Method that reads an input file of a particular format and creates objects corresponding to the file

Question

This is essentially a best practices question about reading a file and making sure it follows a specific format. My current implementation works, but it's a while loop with a bunch of if/else statements. I think it wouldn't be too readable for other developers.

/**
 * Request the server to add the theatre details found in the file indicated by the path.
 * The details include the theatre ID (unique across all theatres), the seating dimension, and
 * the floor area (square metres). All theatres have as many seats in a row as they do rows, and the number of
 * rows is indicated by the seating dimension.
 * <p>
 * <p>The file format is, each line consists of
 * "THEATRE" "\t" theatre ID; "\t" seating dimension; "\t" floor area;
 * <p>
 * <p>If there is no file associated with the path, or the file format is incorrect, then
 * the request fails.
 *
 * @param path The path indicating the file to use for the initialisation.
 * @return An empty string if the initialisation is successful, otherwise a
 * message explaining what went wrong beginning with ERROR (e.g. "ERROR incorrect format").
 */
public String initialise(String path)
{
 //IO
 File theatreFile = new File(path);
 Scanner fileScanner = null;
 try
 {
 fileScanner = new Scanner(theatreFile);
 }
 catch (FileNotFoundException e)
 {
 return ResponseMessages.FILE_NOT_FOUND_ERR_MSG.getDescription();
 }
 String currentID = null;
 int numRows;
 int floorSpace;
 /*Loop through input file tokens, check if format is correct and if so, create a theatre object
 that corresponds with the given data. */
 while(fileScanner.hasNext())
 {
 if(fileScanner.hasNext(THEATRE_NAME_MARKER))
 {
 fileScanner.next();
 if(fileScanner.hasNext(THEATRE_CODE_MARKER))
 {
 currentID = fileScanner.next();
 if (getTheatreIDs().contains(currentID))
 {
 return ResponseMessages.DUPLICATE_CODE_ERR_MSG.getDescription();
 }
 if (fileScanner.hasNextInt())
 {
 numRows = fileScanner.nextInt();
 if (fileScanner.hasNextInt())
 {
 floorSpace = fileScanner.nextInt();
 }
 else
 {
 return ResponseMessages.FILE_FORMAT_ERR_MSG.getDescription();
 }
 }
 else
 {
 return ResponseMessages.FILE_FORMAT_ERR_MSG.getDescription();
 }
 }
 else
 {
 return ResponseMessages.FILE_FORMAT_ERR_MSG.getDescription();
 }
 }
 else
 {
 return ResponseMessages.FILE_FORMAT_ERR_MSG.getDescription();
 }
 Theatre newTheatre = new Theatre(currentID, numRows, floorSpace);
 theatres.add(newTheatre);
 }
 fileScanner.close();
 return ResponseMessages.FILE_FOUND_SUCCESS_MSG.getDescription();
}

Additionally, here's an example of what an input file may look like:

THEATRE T2 10 400
THEATRE T1 7 200
THEATRE T3 12 600
THEATRE 215 21 1200

• Mark Jeronimus: Invert the if conditions. You then don't need the 'else' as te main body returns. So you don't keep stepping into deeper levels
• James Lubin: The main body returns a success message though, while the else statements return error messages. This would create objects even though the file format doesn't match which isn't what I want.

Answer 1

As you are asking for best practices: methods and exceptions.

First of all, your code does everything in one method, where you could (and should) clearly divide the parts of

• looping over the file
• parsing a single line
• checking uniqueness of the theatres

As you return with a failure anyway if a single line fails, you might as well throw an exception and stop the complete process. If you want to stick to the interface that returns an error/success condition as a result value, you may well catch and return in the outermost method.

Rough sketch:

private static class MyFileParsingException extends RuntimeException {
 ... add a constructor with a message here
}
public String initialise(String pathString) {
 try {
 Path filePath = Paths.get(pathString);
 List<Theater> theatres = Files.lines(filePath)
 .map(this::parseLineToTheater)
 .collect(Collectors.toList());
 checkUniqueness(theatres );
 this.theatres = theatres; // only set if successful
 return ... // success
 }
 catch(IOException e) {
 return ResponseMessages.FILE_NOT_FOUND_ERR_MSG.getDescription();
 }
 catch(MyFileParsingException e) {
 return e.getMessage();
 }
}
private Theater parseLineToTheater(String inputLine) throws MyFileParsingException {
 // parse single line, throw MyFileParsingException with the
 // appropriate message, return the theater
}
private void checkUniqueness(List<Theater> allReadTheaters) throws MyFileParsingException {
 // check, whether the ids are unique, throw MyFileParsingException
 // with DUPLICATE_CODE_ERR_MSG if you find a duplicate
}

Here, you have a trivial outer loop (or in fact a stream), which uses the nio.Files methods (which have been around since java 8, i.e. more than 4 years now - time to start using them), and two more methods which can trivially be unit-tested without even creating a file.

A little twist is extending RuntimeException instead of Exception for the hommade business exception, so that it can be used in a lambda expression. As it extends RuntimeException, the throws declaration is technically not really necessary on the methods, but I like to have them there so that the reader immediately sees that this exception type is to be expected.

• James Lubin: I'm confused by this line: .map(this::parseLineToTheater) what does it do? I'm unfamiliar with that syntax.
• James Lubin: Ah, it's related to lambdas. I'm not familiar with lambdas yet, so I rewrote that part with out it. I also made other tweaks for performance reasons. I've combined elements of Eric's answer as well.

Answer 2

• Always close resources in a try-with-resources or try-finally block. Otherwise if something bad happens before you manually close it, the resource will remain open.

• All three variables can be defined inside your loop.

• It would be a lot easier to read the code if you used guard clauses (if not condition return error string) rather than making your code look like a giant greater-than sign.

• Putting the new theatre into a variable doesn't buy you much.

• You can clean up a couple of the checks for a valid int by catching the relevant exception rather than rechecking for hasNextInt. The clarity this adds is arguable, because now the reader has to track the exception as well as the proactive checks in their head, but it makes the non-error case easier to read.

With those changes, your code might look something like:

public String initialise(final String path) {
 final File theatreFile = new File(path);
 try (final Scanner fileScanner = new Scanner(theatreFile)) {
 while (fileScanner.hasNext()) {
 if (!fileScanner.hasNext(THEATRE_NAME_MARKER)) {
 return ResponseMessages.FILE_FORMAT_ERR_MSG.getDescription();
 }
 fileScanner.next();
 if (!fileScanner.hasNext(THEATRE_CODE_MARKER)) {
 return ResponseMessages.FILE_FORMAT_ERR_MSG.getDescription();
 }
 final String currentID = fileScanner.next();
 if (this.getTheatreIDs().contains(currentID)) {
 return ResponseMessages.DUPLICATE_CODE_ERR_MSG.getDescription();
 }
 final int numRows = fileScanner.nextInt();
 final int floorSpace = fileScanner.nextInt();
 this.theatres.add(new Theatre(currentID, numRows, floorSpace));
 }
 } catch (final InputMismatchException e) {
 return ResponseMessages.FILE_FORMAT_ERR_MSG.getDescription();
 } catch (final FileNotFoundException e) {
 return ResponseMessages.FILE_NOT_FOUND_ERR_MSG.getDescription();
 }
 return ResponseMessages.FILE_FOUND_SUCCESS_MSG.getDescription();
}