Improve testability of constructor

Question

I have the following constructor

 /**
 * @param codePath path to player's code. If ends in '/' it assumes the code is in .class'es in
 *            a directory; otherwise assumes a jar
 * @throws PlayerException if there is a problem loading the code
 */
public Player(File codePath) throws PlayerException {
if (!codePath.exists() || !codePath.canRead()) {
    throw new IllegalArgumentException("Path is invalid: " + codePath
        + ". It can't be read or it doesn't exist");
}

if (Thread.currentThread().getPriority() < ROBOTS_MAX_PRIORITY) {
    throw new PlayerException("Robot priority cannot be greater than game's");
}

try {
    loader = new URLClassLoader(new URL[] { codePath.toURI().toURL() });
    InputStream stream = loader.getResourceAsStream(PLAYER_PROPERTIES_FILE);

    if (stream == null) {
    throw new PlayerException("Player Specification File not found: " + codePath + "!"
        + PLAYER_PROPERTIES_FILE);

    } else {
    Properties playerInfo = new Properties();
    playerInfo.load(stream);

    // read parameters
    author = playerInfo.getProperty("Author", "Unknown programmer");

    teamName = playerInfo.getProperty("Team", "Unknown team");

    robotsThreads = new ThreadGroup(PLAYERS_GROUP, teamName + " Threads");
    robotsThreads.setMaxPriority(ROBOTS_MAX_PRIORITY);

    String banksList = playerInfo.getProperty("Banks");

    if (banksList == null) {
        throw new PlayerException("Error loading configuration property: No banks definition found");
    }
    String[] banksClasses = banksList.split(",");

    teamId = getNextTeamId();
    teamIds.add(teamId);

    banks = loadBanks(loader, banksClasses);

    log.info("[Player] Successfully loaded Player: {}. Banks found = {}", this, banksClasses.length);
    }

} catch (ClassCastException | IOException | ClassNotFoundException | InstantiationException
    | IllegalAccessException | IllegalArgumentException | InvocationTargetException | SecurityException exc) {
    closeClassLoader();
    throw new PlayerException("Error loading player's code", exc);

} catch (PlayerException exc) {
    closeClassLoader();
    throw exc;
}

}

The problem is that the fact that it takes a File makes it hard to test, i don't want to create dummy files. My usual technique is to find what the logic does with the File and then create a constructor that takes that actual object and use that constructor in the unit tests with the original constructor calling the new one. In this case it can be seen that what's needed from the File is the loader so I tried creating a package-protected constructor

Player(URLClassLoader pLoader, String codePath) throws PlayerException { .. } //the second parameter is just used for logging

However, the problem is that I obviously can't do something like this

public Player(File codePath) throws PlayerException, MalformedURLException {
  if (!codePath.exists() || !codePath.canRead()) {
    throw new IllegalArgumentException("Path is invalid: " + codePath
        + ". It can't be read or it doesn't exist");
    }
this(new URLClassLoader(new URL[] { codePath.toURI().toURL() }), "test");    

}

i.e. first validate the File and then invoke the other constructor, because this() calls need to be the first in the method. I also tried extracting the initialization code into a separate method that then maybe i could test by itself, but the variables to initialize (author, teamName, etc) are final, so the compiler again complains. I'm trying to leave those variables as final.

Can anyone suggest another way to refactor this so that i can test the logic that populates the player's data at least. For the full class see here

Answer 1

I'm just going to focus on one of your statements:

However, the problem is that I obviously can't do something like this:

public Player(File codePath) throws PlayerException, MalformedURLException {
    if (!codePath.exists() || !codePath.canRead()) {
        throw new IllegalArgumentException("Path is invalid: " + codePath
              + ". It can't be read or it doesn't exist");
    }
    this(new URLClassLoader(new URL[] { codePath.toURI().toURL() }), "test");    
}

You are right, that's not legal in Java, but there is a relatively common trick to doing this sort of thing (and this trick can sometimes make complicated constructors really quite simple):

private static final URLClassLoader[] fileArgumentHandler(File arg) {
    if (!codePath.exists() || !codePath.canRead()) {
        throw new IllegalArgumentException("Path is invalid: " + codePath
                            + ". It can't be read or it doesn't exist");
    }
    return new URLClassLoader(new URL[]{arg.toURI().toURL()});
}

public Player(File codePath) {
    this(fileArgumentHandler(codePath), "test");
}

Often in larger projects this type of handling becomes quite regular, and it is common to centralize some of these validation processes in to a 'Utility' class that is reused. This is especially true with unit-testing, and I commonly see something like:

@Test
public void testSomeFunction() {
    Player p = newPlayer(UnitTestUtils.fileArgumentHandler(new File(...)));
} 

Note that in this case the utility method has moved outside the constructor and is now part of the invocation... but the concept is similar.

Answer 2

I think you are putting way too much logic in your constructor. The other problem is that you do not want to create dummy files, if so, are there are real files you can make a copy of and test on? If there are no real files either you might actually have to create some real/dummy files.

And if I understand your question you want to test "the logic" of the Player class, which is basically invoking and creating a new URLClassLoader, but since this class takes a File as argument you can't invoke it. In that case you could change the argument from a File to a String so the invoke would look something like

public Player(String filePath) {
    this(new URLClassLoader(new URL[] { filePath }), "test");
}