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!

More concerning is what you would do with `e` in the case of an API, `printStackTrace` smells funny. Surely you should let the users of the code encounter the exception so that they may do something about it? That's what exceptions are for. — Grant Thomas, May 28 '13 at 15:40
Why not change the `validateXXX` operations to return booleans and then set a variable called `valid` to be if all three `validateXXX` calls are valid. Then expose that var out with a method `isValid` — cmbaxter, May 28 '13 at 15:43
Doing something with the [Command Pattern](http://en.wikipedia.org/wiki/Command_pattern) may help here; that is, instantiate a method you'll be calling multiple times, passing in the data to validate, and let that method do the exception throwing. — Clockwork-Muse, May 28 '13 at 15:45

Lee Meador · Accepted Answer · 2013-11-25T23:23:47.897

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.

An external source of data - you have a file, they have an incoming message in XML or JSON format.
Code to build the objects - you have your code, they have their libraries of code working according the specifications in the various JSRs.
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());
    }

}

Your first type is broken (methods _within_ the constructor?) and the second is just an exercise in redundancy. — Grant Thomas, May 28 '13 at 15:49
@GrantThomas I would love to hear more. I see no problem calling methods in the constructor, if they serve a useful purpose. Maybe you could explain. OP would have to decide what's useful. Second, are you saying the `catch-rethrow` is redundant? — Lee Meador, May 28 '13 at 15:56
@LeeMeador Thanks for taking so much time writing out several ideas!They provided alot of food for thought. I think I will go with variation 3. Moving out the validation calls from the constructor and propogating the exception to the caller is much more friendly in terms of being able to reuse a single object, and the calling class is much more suited to knowing what should happen if invalid data has been passed. — deanmau5, May 28 '13 at 17:39
@LeeMeador I also think your first example is broken. You are indeed calling methods from within the constructor, which is syntactically OK, but you also declare a method `isValid()` in your constructor. It's a curly brackets issue. — Timmos, Nov 20 '13 at 13:24
@GrantThomas: Catching checked exceptions and rethrowing as unchecked exceptions is often a good idea, and should be done much more often than it actually is (indeed, I would posit that just about the only "problem" with checked exceptions is the lack of a convenient syntax to express that). Checked exceptions should only be allowed to propagate in cases where the caller will perceive the exception as having the same meaning as the code which threw it. If a `ReadSoundFile` method tries to load a CODEC and that attempt throws a FileNotFoundException, letting it propagate would... — supercat, Nov 26 '13 at 01:00
...falsely lead the caller to belive that the `ReadSoundFile` method had trouble loading the file containing the sound (as opposed to the file containing the software necessary to decode it). If Java had a `doesntexpect X` declaration via which it could indicate that it calls methods which are declared as throwing X, but it doesn't expect them to do so, and that if they do throw X it should be considered "unexpected" and rethrown as an unchecked exception, then checked exceptions could actually be quite useful. — supercat, Nov 26 '13 at 01:04

score 4 · Answer 2 · answered May 28 '13 at 15:52

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.

Thankyou for suggesting this patter. Im very much for implementing patterns my code and this is something I'll certainly play around with. Thanks for the anti pattern advice also! To be honest I just have it there for dev purposes until I work out a solution to the initial question! — deanmau5, May 28 '13 at 17:32

score 2 · Answer 3 · answered May 28 '13 at 15:48

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.

Try / Catch in Constructor - Recommended Practice?

3 Answers3

Linked

Related