Try / Catch in Constructor - Recommended Practice?

Question

Something I've always been curious of

public class FileDataValidator {

private String[] lineData;

public FileDataValidator(String[] lineData){

    this.lineData = lineData;
    removeLeadingAndTrailingQuotes();

    try
    {
        validateName();
        validateAge();
        validateTown();
    }
    catch(InvalidFormatException e)
    {
        e.printStackTrace();
    }

}

//validation methods below all throwing InvalidFormatException

Is is not advisable to include the try/catch block within my Constructor? I know I could have the Constructor throw the Exception back to the caller. What do you guys prefer in calling methods like I have done in Constructor? In the calling class would you prefer creating an instance of FileDataValidator and calling the methods there on that instance? Just interested to hear some feedback!

Answer 1

In the code you show, the validation problems don't communicate back to the code that is creating this object instance. That's probably not a GOOD THING.

Variation 1:

If you catch the exception inside the method/constructor, be sure to pass something back to the caller. You could put a field isValid that gets set to true if all works. That would look like this:

private boolean isValid = false;

public FileDataValidator(String[] lineData){

    this.lineData = lineData;
    removeLeadingAndTrailingQuotes();

    try
    {
        validateName();
        validateAge();
        validateTown();
        isValid = true;
    }
    catch(InvalidFormatException e)
    {
        isValid = false;
    }
}

public boolean isValid() {
    return isValid;
}

Variation 2:

Or you could let the exception or some other exception propagate to the caller. I have shown it as a non-checked exception but do whatever works according to your exception handling religion:

public FileDataValidator(String[] lineData){

    this.lineData = lineData;
    removeLeadingAndTrailingQuotes();

    try
    {
        validateName();
        validateAge();
        validateTown();
    }
    catch(InvalidFormatException e)
    {
        throw new com.myco.myapp.errors.InvalidDataException(e.getMessage());
    }

}

Variation 3:

The third method I want to mention has code like this. In the calling code you have to call the constructor and then call the build() function which will either work or not.

String[] lineData = readLineData();
FileDataValidator onePerson = new FileDataValidator();
try {
    onePerson.build(lineData);
} catch (InvalidDataException e) {
    // What to do it its bad?
}

Here is the class code:

public FileDataValidator() {
    // maybe you need some code in here, maybe not
}

public void build(String[] lineData){

    this.lineData = lineData;
    removeLeadingAndTrailingQuotes();

    try
    {
        validateName();
        validateAge();
        validateTown();
    }
    catch(InvalidFormatException e)
    {
        throw new com.myco.myapp.errors.InvalidDataException(e.getMessage());
    }

}

Of course, the build() function could use a isValid() method that you call to see if its right but an exception seems the right way to me for the build function.

Variation 4:

The fourth method I want to mention is what I like best. It has code like this. In the calling code you have to call the constructor and then call the build() function which will either work or not.

This sort of follows the way JaxB and JaxRS work, which is a similar situation to what you have.

1. An external source of data - you have a file, they have an incoming message in XML or JSON format.
2. Code to build the objects - you have your code, they have their libraries of code working according the specifications in the various JSRs.
3. Validation is not tied to the building of the objects.

The calling code:

String[] lineData = readLineData();
Person onePerson = new Person();
FileDataUtilities util = new FileDataUtilities();
try {
    util.build(onePerson, lineData);
    util.validate(onePerson);
} catch (InvalidDataException e) {
    // What to do it its bad?
}

Here is the class code where the data lives:

public class Person {
    private Name name;
    private Age age;
    private Town town;
... lots more stuff here ...
}

And the utility code to build and validate:

public FileDataValidator() {
    // maybe you need some code in here, maybe not
}

public void build(Person person, String[] lineData){

    this.lineData = lineData;
    removeLeadingAndTrailingQuotes();
    setNameFromData(person);
    setAgeFromData(person);
    setTownFromData(person);
}

public boolean validate(Person person) {

    try
    {
        validateName(person);
        validateAge(person);
        validateTown(person);
        return true;
    }
    catch(InvalidFormatException e)
    {
        throw new com.myco.myapp.errors.InvalidDataException(e.getMessage());
    }

}

Answer 2

You should consider the static factory pattern. Make your all-arguments constructor private. Provide a static FileDataValidator(args...) method. This accepts and validates all the arguments. If everything is fine, it can call the private constructor and return the newly created object. If anything fails, throw an Exception to inform the caller that it provided bad values.

I must also mention that this: catch (Exception e) { printSomeThing(e); }

Is the deadliest antipattern you could do with Exceptions. Yes, you can read some error values on the command line, and then? The caller (who provided the bad values) doesn't get informed of the bad values, the program execution will continue.

Answer 3

My preference is for exceptions to be dealt with by the bit of code that knows how to deal with them. In this case I would assume that the bit of code creating a FileDataValidator knows what should happen if the file data is not valid, and the exceptions should be dealt with there (I am advocating propagating to the caller).

Whilst discussing best practice - the class name FileDataValidator smells to me. If the object you're creating stores file data then I would call it FileData - perhaps with a validate method? If you only want to validate your file data then a static method would suffice.