I'm looking at a class diagram and it shows a method like this:
SavePrintOptions(bool,bool,bool,bool,bool,bool,bool,bool,bool);
where each of the parameters match up with save options (checkboxes) in the UI. How could I refactor this signature to reduce the number of parameters?
-
5\$\begingroup\$ As the answer to this question suggests, collect all of the parameters into a class, and change the method to accept an object of that class. \$\endgroup\$kenrogers– kenrogers2013年06月14日 19:22:22 +00:00Commented Jun 14, 2013 at 19:22
-
\$\begingroup\$ It's also possible that a large number of parameters to a method indicates the code is smelling. Extract and refactor to more methods, possibly even more classes, to embody SRP (Single Responsibility Principle). \$\endgroup\$Jesse C. Slicer– Jesse C. Slicer2013年06月14日 21:05:52 +00:00Commented Jun 14, 2013 at 21:05
3 Answers 3
That is a real code smell and should be refactored. How should any mortal soul unterstand, what this code does?
Why not using a simple object?
public class{
public bool Whateveryouwant { get; set; }
public bool Whateveryouwantelse { get; set; }
}
The advantage:
1) your parameter shrunk to just one
2) if you name the properties (better than above) anybody else knows, what options were chosen.
-
1\$\begingroup\$ You may also wish to use enumerations rather than boolean values. An enum with 2 possibilities is a lot clearer and easier to understand than simply "true/false". \$\endgroup\$MattDavey– MattDavey2013年06月15日 09:53:00 +00:00Commented Jun 15, 2013 at 9:53
-
1\$\begingroup\$ Or blog.barvinograd.com/2012/06/working-with-bitfields-in-c \$\endgroup\$Thomas Junk– Thomas Junk2013年06月15日 10:01:51 +00:00Commented Jun 15, 2013 at 10:01
I would create additional PrintOptions class with immutable fields having meaningful names, it could be easily serialized, passed as method parameter etc.
You also can have several different option sets so it easy to manipulate with several options at once not just one by one if it's required, for example you can have DefaultOptions instance.
public class PrintOptions {
public readonly bool option1;
public readonly bool option2;
public readonly bool optionN;
public PrintOptions(bool option1, bool option2, bool optionN) {
this.option1 = option1;
this.option2 = option2;
this.optionN = optionN;
}
}
public class SomeClass {
public SomeClass(PrintOptions printOptions) {
PrintOptions = printOptions;
}
public PrintOptions PrintOptions {get; set;}
public void Print() {
Print(this.PrintOptions);
}
public void Print(PrintOptions printOptions) {
Print(printOptions);
}
}
So print options is decoupled from class that do some work using this options. PrintOptions could be considered as dependency or passed as an argument to Print method, depending on your requirements
Looks like there are a lot of hidden work is going inside class that is not responsible for saving print options.
It might be even possible that this method doesn't use all of the parameters. I also saw code, where private instance fields were passed into method instead of using them inside calling method:
public Work()
{
...
SaveOptions(_instanceField, whatever)
}
private void SaveOptions(bool option, string parameter)
I'd rather move this parameters as fields into separate class. Then give meaningfull names and provide default values for fields. After all you will have class with single responsibility and method without ANY parameters
var printOptions = new PrintOptions(bool, bool);
printOptions.Orientation = Orientation.Vertical;
printOptions.BlackAndWhite = true;
printOptions.Save();