Method that reads an input file of a particular format and creates objects corresponding to the file
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
-
\$\begingroup\$ 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 \$\endgroup\$Mark Jeronimus– Mark Jeronimus2018年07月04日 13:43:24 +00:00Commented Jul 4, 2018 at 13:43
-
\$\begingroup\$ 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. \$\endgroup\$James Lubin– James Lubin2018年07月04日 15:02:33 +00:00Commented Jul 4, 2018 at 15:02
2 Answers 2
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.
-
\$\begingroup\$ I'm confused by this line: .map(this::parseLineToTheater) what does it do? I'm unfamiliar with that syntax. \$\endgroup\$James Lubin– James Lubin2018年07月05日 15:54:32 +00:00Commented Jul 5, 2018 at 15:54
-
\$\begingroup\$ 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. \$\endgroup\$James Lubin– James Lubin2018年07月05日 17:53:47 +00:00Commented Jul 5, 2018 at 17:53
Always close resources in a
try-with-resources
ortry-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 forhasNextInt
. 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();
}