1
\$\begingroup\$

I have a class that reads key-value pairs from an Excel file. I want to store the values in variables and access them from the class instance.

I did this by putting in the constructor the method that reads the Excel file. Is this OK? AFAIK it's not a good practice to put methods in constructors.

public class ExcelReader{
    private Map<String, String> map = null;
    private String firstname;
    private String lastname;

    public ExcelReader(){
        map = new HashMap<String, String>();
        getExcelData();
        firstname = map.get("Firstname");
        lastname= map.get("Lastname");
    }


    private void getExcelData() { //reads data from excel and puts it in map
       //...
       map.put(key,value);
    }

}

This is how I want to get the values:

ExcelReader reader = new ExcelReader();
reader.firstname;
\$\endgroup\$
3
\$\begingroup\$

You do have to be careful when calling methods from the constructor though. See the answer to the same question here. But as long as you don't call overridable methods (and don't pass a reference to this) there is no problem.

The biggest issue I have with your method is the naming. By convention a method starting with getXXX() implies that it actually returns that XXX. If you would rename your method to something like initFromExcelData() for example it would be a lot clearer what the method does.

I personally would also do the assignments for first and last name inside this method. Especially if these are the only things you want to do with the excel. That way you can make the map local to that init method.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the review Imus. I will add your suggestions to my code. \$\endgroup\$ – return Aug 4 '17 at 9:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.