Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Should callers check validity of arguments before calling constructor?

I have been reading a lot about TDD and clean code recently so I start working on a simple project that puts these to use and I have come up against something that I am really not sure what the best approach to take is.

I have a class that takes a Java File object as a parameter, the expectation is that this File object must be a directory and must begin with a certain prefix. My first pass through involved doing checks on the File object before calling the constructor, i.e. checking that it is a directory and checking that the name is valid. But I don't like that it is the caller that is specifying what makes it valid and particularly what the valid prefix is, I think this logic should be placed in the class itself.

I could do this checking in the constructor and throw an exception if it is not valid, but given the nature of the problem, if I am iterating over a list of Files then it is fully expected that some of them won't be 'valid' (i.e. they will be files rather than directories) so is throwing an Exception really warranted?

public MyObject(File directory) {
    if (!directory.isDirectory()) {
        throw new IllegalArgumentException("Must be a directory");
    }
    if (!directory.getName().startsWith("Prefix")) {
        throw new IllegalArgumentException("Must start with Prefix");
    }
    ....
}

I thought about maybe adding a Factory method to create the objects and returning null if the File is invalid.

public static MyObject createMyObject(File directory) {
    if (!directory.isDirectory() || !directory.getName().startsWith("Prefix")) {
        return null;
    }
    return new MyObject(directory);
}

Alternatively I thought about adding a static method to the class that validates the File for the caller before calling the constructor.

public static boolean isValid(File directory) {
    return directory.isDirectory() && directory.getName().startsWith("Prefix");
}

if (MyObject.isValid(directory)) {
    MyObject object = new MyObject(directory);
}

So in terms of clean code and all the OOP principles (such as single responsibility, coupling etc.) which would be the preferred way of doing this?

UPDATE:

Having read some of the answers that have been posted already I started thinking about another possibility that would be applicable to only my current situation rather than generically as my question was really about.

As part of my calling code I have a path from the filesystem and I am listing all files in that directory and it is each file that I am then passing to the MyObject constructor whether it is valid or not. I could pass a FileFilter to the method listFiles that ensures that listFiles only returns valid directories. The FileFilter could be declared within MyObject:

public static FileFilter getFilter() {
    return new FileFilter() {
        public boolean accept(File path) {
            return path.isDirectory() && path.getName().startsWith("Prefix");
        }
    };
}

If my constructor threw an exception then it would really be an exceptional situation because the expectation is that it is only being passed valid directories. Doing this would mean that I could remove the need for a checked exception from constructor/factory since any exception would indicate a bug somewhere rather than expected behaviour. But it still leaves the question of whether to put it in a constructor or a factory method.

like image 321
DaveJohnston Avatar asked Mar 11 '13 11:03

DaveJohnston


1 Answers

public static MyObject createMyObject(File directory) throws IllegalArgumentException{
    if (!directory.isDirectory() || !directory.getName().startsWith("Prefix")) {
        return throw new IllegalArgumentException("invalid parameters")";
    }
    return new MyObject(directory);
}

This is one of the option which is a mix of two of your suggested options. Uses Factory method and also validates the pre-conditions which is what factory methods are good at doing. So IMO this can be a nice option.

Why not option 2 as it is: Because returning nulls is a bad option when you can throw exceptions to warn the user that some pre-conditions were not met.

UPDATE: This strategy gives guarantee that only an object in valid state will be created. Also if you want to mock out the instance of MyObject you can make it implement some interface for using Runtime Polymorphism and pass out mock objects around. Hope that makes sense.

like image 54
Narendra Pathai Avatar answered Sep 19 '22 02:09

Narendra Pathai