Reading values from a file and assigning to another class

Question

I'm assigning values to an object by passing it to another class. Basically, I'm reading values from a file and assigning it by passing it to another class.

This is the POJO class for the details in a file.

public class FileDetails
{
 String URL;
 String Port;
 String UserName;
 String Pwd;
 public String getUserName() {
 return UserName;
 }
 public String getPort() {
 return Port;
 }
 public String getPwd() {
 return Pwd;
 }
 public String getURL() {
 return URL;
 }
 public void setPort(String port) {
 this.Port = port;
 }
 public void setPwd(String pwd) {
 this.Pwd = pwd;
 }
 public void setURL(String URL) {
 this.URL = URL;
 }
 public void setUserName(String userName) {
 this.UserName = userName;
 }
}

This is the class which I am using to set the values into that an instance of FileDetails.

public class FileService extends FileDetails
{
 public FileService(FileDetails fileDetails)
 {
 FileDetails fileDet = null;
 ObjectMapper objectMapper = new ObjectMapper();
 objectMapper.configure(DeserializationConfig.Feature.FAIL_ON_UNKNOWN_PROPERTIES, false);
 try
 {
 fileDet = objectMapper.readValue(new File("res.json"), fileDetails.class);
 }
 catch(IOException e)
 {
 e.printStackTrace();
 }
 if (fileDet != null) {
 fileDetails.setUserName(fileDet.getUserName());
 fileDetails.setPwd(fileDet.getPwd());
 fileDetails.setURL(fileDet.getURL());
 fileDetails.setPort(fileDet.getPort());
 } else {
 System.out.println("Didn't set details");
 }
 }
}

This works. Everywhere I call this code, I am using two lines of code, like

FileDetails fileDetails = new FileDetails();
FileService fileService = new FileService(fileDetails);

I want to know if this is the optimum way to get things done, or if there are any better ways to do this.

• yuri: I'm by no means a Java professional but couldn't you collapse this into one line e.g. FileService fileService = new FileService(new FileDetails());?
• ayrusme: I could, but that wouldn't enable me to use the fileDetails object elsewhere. The FileService class acts as a setter to any FileDetails object I pass to it.
• Nobody: Bro, first of all there's no point having getter setter methods if the attributes in the class are not private and remember the attributes are not private in Java by default
• ayrusme: @Nobody, The default visibility mode is package-private, unless specified otherwise. Read: docs.oracle.com/javase/tutorial/java/javaOO/accesscontrol.html Anyway, do you find any modifications I can do with this code? Any optimizations?
• Nobody: Yes and both aren't same, private is visible only to the class and nope I just thought I let you know something I saw unusual! @thatrockbottomprogrammer

Answer 1

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);
 }
}

• ayrusme: Thanks for this feedback! I am looking to code better everyday. I'll read up the best practices and try and implement them :)