Why Too Many Overloads Can Be a Bad Thing

April 1, 2008

tags:
7 comments

At work we’re using code generation to create our data access layer. So for the users table we have a UsersDalBase with many overloads of methods such as Add(User) and Add(IList<User>). Thing is, many time we need to override the default behavior of a method. For instance we might want to change Add(User) to notify the user by mail, write something to a log, use caching, etc. We do that in a UsersDal class that inherits from UsersDalBase.

But hmm, that other overload that accepts a list of users is still there. No one uses it for now, but they might try to in the future. Still, it probably wouldn’t have been written if it hadn’t been for code-generation. So should we also update Add(IList<User>) with all the changes (mails, caching…)? The implementation is more difficult than the single entity case and we would have to add tests for that too.

I had a bitter argument with a member of my team on this very subject the other day. He claimed that we must update both methods since they’re there and might be used, and if we don’t, we won’t get notification and caching in that case. I claimed that it is foolish to waste time on methods that will probably never be used. No where in the system do we add more than one user at the time. The purpose of the many overloads the code-generation gives us is to help us by giving us many options, not hinder us.

At the end we agreed to throw a NotSupportedException when someone tries to add more than user at a time. This way we don’t have to write unneeded code, but we are also not concerned that someone in the future will ill-use the class.

Add comment
facebook linkedin twitter email

Leave a Reply

Your email address will not be published.

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <s> <strike> <strong>

*

7 comments

  1. ekampfApril 1, 2008 ב 23:46

    I don’t agree with your argument.
    From experience, anything that is public WILL eventually be used by some developer using your class.

    Make that function usable, or hide it. Throwing a NotSupportedException is a crappy solution…

    Reply
  2. VinniePApril 2, 2008 ב 1:32

    If you’re not using it, toss it. YAGNI. I don’t see why you’d let an the author of an autogenerater who knows nothing of your business process dictate your object model.

    Reply
  3. doronyApril 4, 2008 ב 15:49

    If I could delete that method, I would, but there is no way for me to hide/delete it. If I do that, the next time I regenerate that Dal (if, let’s say, a field is added to the table) it would just come back again. So throwing an exception is the next closest thing.

    Reply
  4. HPApril 4, 2008 ב 18:51

    If in the base class Add(IList) is calling Add(User) for adding users.

    If U override Add(User) in your class and then call Add(IList) from an instance of your class the method that is been called for adding users its the one in your class.

    U should also call base.Add(User) in the overridden method to add the user.

    Reply
  5. ekampfApril 4, 2008 ב 23:52

    You can always encapsulate your base DAL class rather than deriving from it.

    Keep your object model simple don’t expose anything you don’t want to be used to the outside world.

    Reply
  6. doronyApril 5, 2008 ב 14:07

    HP – Actually Add(User) calls Add(IList).

    ekampf – This kind of architecture change is not really possible at this stage. Also, If I encapsulate the base DAL it would mean that it will stop being Abstract (as I will need to have an instance of it). So anyone will be able to create this object and use its public methods… Which me brings me back to exactly the same issue I started with.

    Reply
  7. ...March 3, 2009 ב 12:23

    Interessante Informationen.

    Reply