Return to Answer

• 1

• 25.5k
• 7
• 59
• 141

###Convention violations

• Java conventions generally impose "egyptian bracing style", with the opening brace on the same line as the block start. Yes, also for classes.

• Java convention governs that fields should be named camelCase. This means that FileDetails should have the fields url, port, userName and password

• It's convention to restrict the visibility of fields as much as possible. All fields in FileDetails should be private.

###Constructor responsibilities

So... let me get this straight... You're using a constructor of a separate object to set the properties of an object you pass into the constructor, that is of a supertype and ...

This is wrong in more ways than I imagined one could get constructors wrong ...

Let me break down, what constructors are for:

1. Constructors construct objects
2. Constructors construct objects
3. NOTHING ELSE

What the FileService constructor does is the following:

1. It takes a FileDetails object.
2. It loads a FileDetails object from a (hardcoded) json file.
3. It copies over the loaded object's values to the passed object.

At no point it does initialize the object it's responsible for. Neither does it perform validation or any other useful constructor things.

What this constructor basically is is called "factory method". You're using the constructor of a separate class to initialize the properties of a class you control.

Where did the separate class come from???

Instead of doing it this way, it would make much more sense to have the FileDetails constructor be responsible for the work you do there...

Consider the following:

public class FileDetails {
 private String url;
 private String port;
 private String userName;
 private String password;
 
 public FileDetails(String url, String port, String userName, String password) {
 this.url = url;
 this.port = port;
 this.userName = userName;
 this.password = password;
 }
 public FileDetails(Path detailsFile) {
 try (InputStream in = Files.newInputStream(detailsFile, StandardOpenOption.READ)) {
 ObjectMapper mapper = new ObjectMapper();
 mapper.configure(DeserializationConfig.Feature.FAIL_ON_UNKNOWN_PROPERTIES, false);
 FileDetails details = mapper.readValue(in, FileDetails.class);
 
 this.url = details.url;
 this.port = details.port;
 this.userName = details.userName;
 this.password = details.password;
 } catch (IOException e) {
 e.printStackTrace(System.err);
 }
 }
 // getters and setters omitted
}

Along the side this resolves the following additional problems in the code:

• Overlarge scope of objectMapper.
• Overuse of getters and setters inside a class.
• Missing lifetime control of Resources.
• Hardcoded path value for the file with resources in it.

Now here comes the fun part. The second constructor I wrote is really unnecessary. It's a "factory method" (as I mentioned above). What that means is, it can be rewritten like so:

public static FileDetails fromFile(Path detailsFile) {
 try (InputStream in = Files.newInputStream(detailsFile, StandardOpenOption.READ)) {
 ObjectMapper mapper = new ObjectMapper();
 mapper.configure(DeserializationConfig.Feature.FAIL_ON_UNKNOWN_PROPERTIES, false);
 return mapper.readValue(in, FileDetails.class);
 } catch (IOException e) {
 throw new UncheckedIOException(e);
 }
}