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 ?
Yes, overloading can easily be overused.
ReplyDeleteI'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.
As far as I can tell, you won't have fewer methods, just fewer names. I generally prefer the overloaded method system of naming, but I don't think it really makes much difference as long as you comment and document your code well (which you should do in either case).
ReplyDeleteCan you overuse it? well, yes, that is true.
ReplyDeleteHowever, the examples you've given are perfect examples of when to use method overloading. They all perform the same function, why give them different names just because you're passing different types to them.
The main rule, is doing the clearest, easiest to understand thing. Don't use overloading just to be slick or clever, do it when it makes sense. Other developers might be working on this code as well. You want to make it as easy as possible for them to pick up and understand the code and be able to implement changes without implementing bugs.
I like overloading my methods so that later on in the intellisense I don't have a million of the same method. And it seems more logical to me to have it just overloaded instead of having it named differently a dozen times.
ReplyDeleteOne thing you may consider is that you can't expose overloaded methods as operation contracts in a WCF Web Service. So if you think you may ever need to do this, it would be an argument for using different method names.
ReplyDeleteAnother argument for different method names is that they may be more easily discoverable using intellisense.
But there are pros and cons for either choice - all design is trade-off.
You probably need some project-wide standards. Personally, I find the overloaded methods much easier to read. If you have the IDE support, go for it.
ReplyDeleteThe point of overloading to to ease the learning curve of someone using your code... and to allow you to use naming schemes that inform the user as to what the method does.
ReplyDeleteIf you have ten different methods that all return a collection of employees, Then generating ten different names, (Especially if they start with different letters!) causes them to appear as multiple entries in your users' intellisense drop down, extending the length of the drop down, and hiding the distinction between the set of ten methods that all return an employee collection, and whatever other methods are in your class...
Think about what is already enforced by the .Net framework for, say constructors, and indexers... They are all forced to have the same name, and you can only create multiples by overloading them...
If you overload them, they will all appear as one, with their disparate signatures and comments off to tthe side.
You should not overload two methods if they perform different or unrelated functions...
As to the confusion that can exist when you want to overload two methods with the same signature by type
as in
public List<Employee> GetEmployees(int supervisorId);
public List<Employee> GetEmployees(int departmentId); // Not Allowed !!
Well you can create separate types as wrappers for the offending core type to distinguish the signatures..
public struct EmployeeId
{
private int empId;
public int EmployeeId { get { return empId; } set { empId = value; } }
public EmployeeId(int employeId) { empId = employeeId; }
}
public struct DepartmentId
{
// analogous content
}
// Now it's fine, as the parameters are defined as distinct types...
public List<Employee> GetEmployees(EmployeeId supervisorId);
public List<Employee> GetEmployees(DepartmentId departmentId);
I have seen overloading overused when you have only subtle differences in the arguments to the method. For example:
ReplyDeletepublic List<Product> GetProduct(int productId) { // return a List }
public List<Product> GetProduct(int productId, int ownerId ) { // return a List }
public List<Product> GetProduct(int productId, int vendorId, boolean printInvoice) { // return a List }
In my small example, it quickly becomes unclear if the second int argument should be the owner or customer id.
A brief glance at the framework should convince you that numerous overloads is an accepted state of affairs. In the face of myriad overloads, the design of overloads for usability is directly addressed by section 5.1.1 of the Microsoft Framework Design Guidelines (Kwalina and Abrams, 2006). Here is a brief prècis of that section:
ReplyDeleteDO try to use descriptive parameter names to indicate the default used by shorter overloads.
AVOID arbitrarily varying parameter names in overloads.
AVOID being inconsistent in the ordering of parameters in overloaded members.
DO make only the longest overload virtual (if extensibility is required). Shorter overloads should simply call through to a longer overload.
DO NOT use ref or out parameters to overload members.
DO allow null to be passed for optional arguments.
DO use member overloading rather than defining members with default arguments.
Yes you can overuse it, however here is another concept which could help keep the usage of it under control ...
ReplyDeleteIf you are using .Net 3.5+ and need to apply multiple filters you are probably better to use IQueryable and chaining i.e.
GetQuery<Type>().ApplyCategoryFilter(category).ApplyProductNameFilter(productName);
That way you can reuse the filtering logic over and over whereever you need it.
public static IQueryable<T> ApplyXYZFilter(this IQueryable<T> query, string filter)
{
return query.Where(XYZ => XYZ == filter);
}
You can use Overloading as much as you want.
ReplyDeleteFrom the best practices point of view as well, it is recommended that you use Overloading if you are trying to perform the same "operation" (holistically) on the data. E.g. getProduct()
Also, if you see Java API, Overloading is everywhere. You wouldn't find a bigger endorsement than that.
Overloading is desirable polymorphic behavior. It helps the human programmer remember the method name. If explicit is redundant with the type parameter, then it is bad. If the type parameter does not imply what the method is doing, then explicit starts to make sense.
ReplyDeleteIn your example, getProductByName is the only case where explicit might make sense, since you might want to get product by some other string. This problem was caused by the ambiguity of primitive types; getProduct(Name n) might be a better overload solution in some cases.
Another option is to use a Query object to build the "WHERE Clause". So you would have only one method like this:
ReplyDeletepublic List<Product> GetProducts(Query query)
The Query object contains the condidition expressed in an Object Oriented way. The GetProducts obtain the query by "parsing" the Query object.
http://martinfowler.com/eaaCatalog/queryObject.html
yes you can overuse it. In your example it would seem like the first and third would probably return a single item, where the second would return several. If that is correct, then I would call the first and third GetProduct and the second GetProducts or GetProductList
ReplyDeleteif this is not the case and all three return several (as in if you pass it productID 5, it returns any items with 5 in the productid, or returns any items with the string parameter in its name) then I would call all three GetProducts or GetProductList and override all of them.
In any event, the name should reflect what the function does, so calling it GetProduct (singular) when it returns a list of Products doesn't make a good function name. IMNSHO
I'm a total fan of the "explicit" way: giving each function a different name. I've even refactored some code which had lots of Add(...) functions in the past, to AddRecord(const Record&), AddCell(const Cell&), etc.
ReplyDeleteI think this helps to avoid some confusions, inadvertent casts (in C++, at least) and compiler warnings, and it improves clarity.
Maybe in some cases you need the other strategy. I haven't encountered one yet.