views:

99

answers:

5

I have a simple application that loads data from an XML file to a database.

public class EmployeeLoader()  
{ 
   public void LoadEmpoyees()
   {...}

   public void LoadSalaries()
   {...}

   public void LoadRegistrationData()
   {...}    

   public void LoadTaxData()
   {...}
}

Is it a good idea to have multiple "Load" methods as this looks like a code smell as I have about tweney Load methods? If so, how do I make my code more readable?
Each Load method loads data to the corresponding table in the database via a repository?

+3  A: 

Actually, having it broken down into well-named steps like this is very readable.

See this article: http://www.codinghorror.com/blog/2008/07/coding-without-comments.html

Nissan Fan
A: 

How about a single exposure point to the user called "Load()" this method could take an enum argument in it indicating what they want to load, and then under the covers you can have as many/few methods as you need/want to implement this outer exposed call.

NSA
+3  A: 

The big question is whether these methods all have to be public. Ideally you would want something like a single public Load method that loads everything from the database by calling all of your private Load* methods.

Gabe
+1  A: 

Having them separate make it much more readable than a load method with lots of boilerplate to manage the different scenarios

You'd have something like

public void Load() {
  if (condition1 that makes me know I'm loading an employee) {
    //whatever applies to this condition
  }
  if (condition2 that makes me know I'm loading salaries) {
    //whatever applies to this condition
  }
  if (condition3 that makes me know I'm loading registrationData) {
    //whatever applies to this condition
  }
  if (condition4 that makes me know I'm loading taxData) {
    //whatever applies to this condition
  }
}

Ugh.

Even if the methods do very similar stuff, it might be a good idea separating them and calling similar methods. This way, if something changes, it will be an easy refactor =).

Finally, if the class gets too big (too many responsibilities), you might consider breaking into more classes with more specific responsibilities.

Samuel Carrijo
A: 

I'm assuming that the loaded data is going to be saved in local fields/dictionaries, and then used by another method.

if that is the case you could lazy load the values as they are required.

public class EmployeeLoader
{

    private List<String> _Employees = null;
    public List<String> Employees
    {
        get
        {
            if (_Employees == null)
            {
                LoadEmployees();
            }
            return _Employees;
        }
    }

    private void LoadEmployees()
    {
        //Load Data
    }
}

to complement that you could still have a single Load() method to force load the values into those backing fields.

Keivan