c# - Method Overloading. Can you overuse it?

Question

What's better practice when defining several methods that return the same shape of data with different filters? Explicit method names or overloaded methods?

For example. If I have some Products and I'm pulling from a database

explicit way:

public List<Product> GetProduct(int productId) {    // return a List    }
public List<Product> GetProductByCategory(Category category) {    // return a List    }
public List<Product> GetProductByName(string Name ) {    // return a List    }

overloaded way:

public List<Product> GetProducts() {    // return a List of all products    }
public List<Product> GetProducts(Category category) { // return a List by Category }
public List<Product> GetProducts(string searchString ) { // return a List by search string }

I realize you may get into a problem with similar signatures, but if you're passing objects instead of base types (string, int, char, DateTime, etc) this will be less of an issue. So... is it a good idea to overload a method to reduce the number of methods you have and for clarity, or should each method that filters the data a different way have a different method name?

Answer 1

Yes, overloading can easily be overused.

I've found that the key to working out whether an overload is warranted or not is to consider the audience - not the compiler, but the maintenance programmer who will be coming along in weeks/months/years and has to understand what the code is trying to achieve.

A simple method name like GetProducts() is clear and understandable, but it does leave a lot unsaid.

In many cases, if the parameter passed to GetProducts() are well named, the maintenance guy will be able to work out what the overload does - but that's relying on good naming discipline at the point of use, which you can't enforce. What you can enforce is the name of the method they're calling.

The guideline that I follow is to only overload methods if they are interchangable - if they do the same thing. That way, I don't mind which version the consumer of my class invokes, as they're equivalent.

To illustrate, I'd happily use overloads for a DeleteFile() method:

void DeleteFile(string filePath);
void DeleteFile(FileInfo file);
void DeleteFile(DirectoryInfo directory, string fileName);

However, for your examples, I'd use separate names:

public IList<Product> GetProductById(int productId) {...}
public IList<Product> GetProductByCategory(Category category) {...}
public IList<Product> GetProductByName(string Name ) {...}

Having the full names makes the code more explicit for the maintenance guy (who might well be me). It avoids issues with having signature collisions:

// No collisions, even though both methods take int parameters
public IList<Employee> GetEmployeesBySupervisor(int supervisorId);
public IList<Employee> GetEmployeesByDepartment(int departmentId);

There is also the opportunity to introduce overloading for each purpose:

// Examples for GetEmployees

public IList<Employee> GetEmployeesBySupervisor(int supervisorId);
public IList<Employee> GetEmployeesBySupervisor(Supervisor supervisor);
public IList<Employee> GetEmployeesBySupervisor(Person supervisor);

public IList<Employee> GetEmployeesByDepartment(int departmentId);
public IList<Employee> GetEmployeesByDepartment(Department department);

// Examples for GetProduct

public IList<Product> GetProductById(int productId) {...}
public IList<Product> GetProductById(params int[] productId) {...}

public IList<Product> GetProductByCategory(Category category) {...}
public IList<Product> GetProductByCategory(IEnumerable<Category> category) {...}
public IList<Product> GetProductByCategory(params Category[] category) {...}

Code is read a lot more than it is written - even if you never come back to the code after the initial check in to source control, you're still going to be reading that line of code a couple of dozen times while you write the code that follows.

Lastly, unless you're writing throwaway code, you need to allow for other people calling your code from other languages. It seems that most business systems end up staying in production well past their use by date. It may be that the code that consumes your class in 2016 ends up being written in VB.NET, C# 6.0, F# or something completely new that's not been invented yet. It may be that the language doesn't support overloads.