CODEWITHSUNDEEP
c#
Bohn
Bohn

Reputation: 26919

Is this a bad practice in overloading a method?

I had a method like this that its consumers are calling it:

static public void DisplayOrderComments(param1, param2, param3, param4)

Now I added an overload for it like this:

static public void DisplayOrderComments(param1, param2, param3, param4, param5)
{
    DisplayOrderComments(param1, param2, param3, param4);
    param5.Foo();
}

Is it a bad practice? Are there better ways of doing it?

Upvotes: 20

Views: 3718

Answers (6)

jmoreno
jmoreno

Reputation: 13561

I would say that depends upon what DisplayOrderComments and param5.Foo() does. If it does param.Foo() for params 1 to 4, then absolutely, if it doesn't then you're doing something extra, which is possibly worth it's own name.

Are you changing what DisplayOrerComments does or the side-effects, or just the conditions under which it runs? If you're introducing new side effects, that may be worth having as it's own function/name that then calls DisplayOrderComments.

Upvotes: 1

Alexei Levenkov
Alexei Levenkov

Reputation: 100527

This is also commons practice for providing easier to use API to an interface. Keeps interface small with method with multiple parameters, but uses multiple extensions methods (sometimes with same name) that provide easier to use API without polluting interface:

interface ILog
{
   void Log(string message, int someNumber, float anotherParam, object moreParams);
}

public static class LogExtensions
{
   public void Log(ILog this log, message)
   {
      log.Log(message, 42, 0, null); 
   }
// more methods using ILog.Log like LogFormat that takes format string...
}

Upvotes: 2

Oded
Oded

Reputation: 498992

This is absolutely fine - it keeps code DRY and avoids unnecessary duplication.

Not only is it not a bad practice, it is a good practice.

If you are using C# 4.0 and above (VS 2010+), you can use an optional argument for your param5 instead of overloading, as Mikey Mouse mentions in this answer.

Upvotes: 28

Mikey Mouse
Mikey Mouse

Reputation: 3098

Yeah, I woudln't say it's bad but if you're using C# 4.0 I'd recommend making the last parameter optional.

You can read all about em here http://msdn.microsoft.com/en-us/library/dd264739.aspx

Upvotes: 7

George Mauer
George Mauer

Reputation: 122062

Not at all. This is perfectly acceptable but there are two problems.

  1. param5 might be passed in as Null - you might want to write some code to check for that condition and do the appropriate thing.
  2. This will prevent you from using optional parameters later. But you might not care about that.

By the way codereview.stackexchange.com might be a better place for this sort of question.

Upvotes: 4

Aliostad
Aliostad

Reputation: 81660

Nice question.

I would say not, it is normal overloading. but I will change it as such (always implement in the one with most parameters):

static public void DisplayOrderComments(param1, param2, param3, param4)
{
   DisplayOrderComments(param1, param2, param3, param4, null);
}

static public void DisplayOrderComments(param1, param2, param3, param4, param5)
{
  ... // do the work
  if(param5!=null)
     param5.Foo();
}

Upvotes: 21

Related Questions