Got that bad feeling ....
-
Hey all, I was reviewing some C# code today that really made me wince. Although I didn't like it, I struggled to think of a better solution, so I thought I would throw it open here and learn something.
class ABC
{
int doSomething(int a, String b, bool c)
{
return doSomething(a,b,c,-1);
}
int doSomething(int a, String b, bool c, int d)
{
return doSomething(a,b,c,-1,-1);
}
int doSomething(int a, String b, bool c, int d, int e)
{
//do whatever
}
}Now examining the code in the last doSomething, I could make plenty of comments, it was appalling, but I kept getting drawn back to the nasty overloaded doSomething. In C++ I would have used defaults instead and had one method instead of three but this wasn't possible in C#. Under the current circumstances the third doSomething function has ended up with lots of ugly tests like if(d=-1) doDefault() etc, that execute logic that look like it belongs in the parent doSomething. I feel the functionality of the doSomething would better delegated out of the class altogether, into another class that is perhaps instanced each time, setup for each different circumstance and then executed ... the Command pattern. Any better ideas ?
Regards Ray "Je Suis Mort De Rire" Blogging @ Keratoconus Watch
-
Hey all, I was reviewing some C# code today that really made me wince. Although I didn't like it, I struggled to think of a better solution, so I thought I would throw it open here and learn something.
class ABC
{
int doSomething(int a, String b, bool c)
{
return doSomething(a,b,c,-1);
}
int doSomething(int a, String b, bool c, int d)
{
return doSomething(a,b,c,-1,-1);
}
int doSomething(int a, String b, bool c, int d, int e)
{
//do whatever
}
}Now examining the code in the last doSomething, I could make plenty of comments, it was appalling, but I kept getting drawn back to the nasty overloaded doSomething. In C++ I would have used defaults instead and had one method instead of three but this wasn't possible in C#. Under the current circumstances the third doSomething function has ended up with lots of ugly tests like if(d=-1) doDefault() etc, that execute logic that look like it belongs in the parent doSomething. I feel the functionality of the doSomething would better delegated out of the class altogether, into another class that is perhaps instanced each time, setup for each different circumstance and then executed ... the Command pattern. Any better ideas ?
Regards Ray "Je Suis Mort De Rire" Blogging @ Keratoconus Watch
Ray Kinsella wrote:
Any better ideas ?
yes, you can still chain the methods together but in the opposite direction. In the DoSomething (by the way, that should really be Pascal Cased rather than camel Cased) with the least parameters do everything you can that only needs those parameters. Then with the DoSomething with the extra parameters call the first DoSomething and add the additional functionality that needs the extra parameter. Then for the DoSomething with the most parameters add the extra functionality that requires all parameters then call the middle DoSomething, which in turn will call the first DoSomething. Of course, it really depends on what you are doing overall. If it is sufficiently complex it may be better to abstract some of the functionality out in to private methods and have each of the DoSomething methods call the approprate private methods. However, that may cost slighly more to maintain as you'd have to duplicate the call to the private methods in upto three places. But, if the overall code is easier to read it might be worth it.
Upcoming events: * Edinburgh: Web Security Conference Day for Windows Developers (12th April) * Glasgow: Introduction to AJAX (2nd May), SQL Server, Mock Objects My website
-
Ray Kinsella wrote:
Any better ideas ?
yes, you can still chain the methods together but in the opposite direction. In the DoSomething (by the way, that should really be Pascal Cased rather than camel Cased) with the least parameters do everything you can that only needs those parameters. Then with the DoSomething with the extra parameters call the first DoSomething and add the additional functionality that needs the extra parameter. Then for the DoSomething with the most parameters add the extra functionality that requires all parameters then call the middle DoSomething, which in turn will call the first DoSomething. Of course, it really depends on what you are doing overall. If it is sufficiently complex it may be better to abstract some of the functionality out in to private methods and have each of the DoSomething methods call the approprate private methods. However, that may cost slighly more to maintain as you'd have to duplicate the call to the private methods in upto three places. But, if the overall code is easier to read it might be worth it.
Upcoming events: * Edinburgh: Web Security Conference Day for Windows Developers (12th April) * Glasgow: Introduction to AJAX (2nd May), SQL Server, Mock Objects My website
Yeah!, your right, should be the other way around. It was on the tip of my tongue, it just didn't feel right before ... must be loosing my edge. Thanks for that.
Regards Ray "Je Suis Mort De Rire" Blogging @ Keratoconus Watch
-
Hey all, I was reviewing some C# code today that really made me wince. Although I didn't like it, I struggled to think of a better solution, so I thought I would throw it open here and learn something.
class ABC
{
int doSomething(int a, String b, bool c)
{
return doSomething(a,b,c,-1);
}
int doSomething(int a, String b, bool c, int d)
{
return doSomething(a,b,c,-1,-1);
}
int doSomething(int a, String b, bool c, int d, int e)
{
//do whatever
}
}Now examining the code in the last doSomething, I could make plenty of comments, it was appalling, but I kept getting drawn back to the nasty overloaded doSomething. In C++ I would have used defaults instead and had one method instead of three but this wasn't possible in C#. Under the current circumstances the third doSomething function has ended up with lots of ugly tests like if(d=-1) doDefault() etc, that execute logic that look like it belongs in the parent doSomething. I feel the functionality of the doSomething would better delegated out of the class altogether, into another class that is perhaps instanced each time, setup for each different circumstance and then executed ... the Command pattern. Any better ideas ?
Regards Ray "Je Suis Mort De Rire" Blogging @ Keratoconus Watch
Ray Kinsella wrote:
In C++ I would have used defaults instead
Well, C# does not have default parameters, but prefered way to archive same results is with overloads, just like you are doing. Make one DoSomething with paramers a, b, c, d, e that does it all, and provide overloads DoSomething(a,b,c,d) that just calls DoSomething(a,b,c,d,e) with meaningfull "default" value of e, if possible. And the same with DoSomething(a,b,c), etc... If it is not possible then I don't know what you wanted to do with C++ defaults.
"Throughout human history, we have been dependent on machines to survive. Fate, it seems, is not without a sense of irony. " - Morpheus "Real men use mspaint for writing code and notepad for designing graphics." - Anna-Jayne Metcalfe
-
Ray Kinsella wrote:
Any better ideas ?
yes, you can still chain the methods together but in the opposite direction. In the DoSomething (by the way, that should really be Pascal Cased rather than camel Cased) with the least parameters do everything you can that only needs those parameters. Then with the DoSomething with the extra parameters call the first DoSomething and add the additional functionality that needs the extra parameter. Then for the DoSomething with the most parameters add the extra functionality that requires all parameters then call the middle DoSomething, which in turn will call the first DoSomething. Of course, it really depends on what you are doing overall. If it is sufficiently complex it may be better to abstract some of the functionality out in to private methods and have each of the DoSomething methods call the approprate private methods. However, that may cost slighly more to maintain as you'd have to duplicate the call to the private methods in upto three places. But, if the overall code is easier to read it might be worth it.
Upcoming events: * Edinburgh: Web Security Conference Day for Windows Developers (12th April) * Glasgow: Introduction to AJAX (2nd May), SQL Server, Mock Objects My website
Interesting, what you are saying seems to be exact opposite of what I though is how to do default parameter values in C#, what am I missing?
"Throughout human history, we have been dependent on machines to survive. Fate, it seems, is not without a sense of irony. " - Morpheus "Real men use mspaint for writing code and notepad for designing graphics." - Anna-Jayne Metcalfe
-
Interesting, what you are saying seems to be exact opposite of what I though is how to do default parameter values in C#, what am I missing?
"Throughout human history, we have been dependent on machines to survive. Fate, it seems, is not without a sense of irony. " - Morpheus "Real men use mspaint for writing code and notepad for designing graphics." - Anna-Jayne Metcalfe
Colin's recommending a way that gets around having to put ugly kludges in the code, e.g. using -1 to indicate a missing value.
Deja View - the feeling that you've seen this post before.
-
Colin's recommending a way that gets around having to put ugly kludges in the code, e.g. using -1 to indicate a missing value.
Deja View - the feeling that you've seen this post before.
To me it looks like if you can't find meaningfull default value (and that is different from "using -1 to indicate a missing value") indicates that they do different things and you should probably have two different methods rather then overloads.
"Throughout human history, we have been dependent on machines to survive. Fate, it seems, is not without a sense of irony. " - Morpheus "Real men use mspaint for writing code and notepad for designing graphics." - Anna-Jayne Metcalfe
-
To me it looks like if you can't find meaningfull default value (and that is different from "using -1 to indicate a missing value") indicates that they do different things and you should probably have two different methods rather then overloads.
"Throughout human history, we have been dependent on machines to survive. Fate, it seems, is not without a sense of irony. " - Morpheus "Real men use mspaint for writing code and notepad for designing graphics." - Anna-Jayne Metcalfe
Here's a coded example of what I mean: This is the first example where the methods are chained from most parameters to least parameters. This assumes that the methods with additional parameters are doing more work.
public void DoSomething(int a, int b, int c)
{
// Do stuff with a, b and c
}public void DoSomething(int a, int b, int c, int d)
{
// Do some preliminary stuff with d - optional
DoSomething(a, b, c);
// Do additional stuff with d - optional.
// Note: You must do something with d
}public void DoSomething(int a, int b, int c, int d, int e)
{
// Do some preliminary stuff with e - optional
DoSomething(a, b, c, d);
// Do additional stuff with e - optional
// Note: You must do something with e
}NOTE: It is sometimes the case where the version with the most parameters is actually going to do all the work, and the other overloads just provide default values into the method that does the work. Think MessageBox.Show (A lot of its overloads just provide default values for the buttons, icon and so on.) However, if the work is incrementally increased with the number of parameters then the code I showed above is likely to be better.
Upcoming events: * Edinburgh: Web Security Conference Day for Windows Developers (12th April) * Glasgow: Introduction to AJAX (2nd May), SQL Server, Mock Objects Never write for other people. Write for yourself, because you have a passion for it. -- Marc Clifton My website