How to avoid "too many parameters" problem in API design?
I have this API function:
public ResultEnum DoSomeAction(string a, string b, DateTime c, OtherEnum d,
string e, string f, out Guid code)
I don't like it. Becaus开发者_开发百科e parameter order becomes unnecessarily significant. It becomes harder to add new fields. It's harder to see what's being passed around. It's harder to refactor method into smaller parts because it creates another overhead of passing all the parameters in sub functions. Code is harder to read.
I came up with the most obvious idea: have an object encapsulating the data and pass it around instead of passing each parameter one by one. Here is what I came up with:
public class DoSomeActionParameters
{
public string A;
public string B;
public DateTime C;
public OtherEnum D;
public string E;
public string F;
}
That reduced my API declaration to:
public ResultEnum DoSomeAction(DoSomeActionParameters parameters, out Guid code)
Nice. Looks very innocent but we actually introduced a huge change: we introduced mutability. Because what we previously had been doing was actually to pass an anonymous immutable object: function parameters on stack. Now we created a new class which is very mutable. We created the ability to manipulate the state of the caller. That sucks. Now I want my object immutable, what do I do?
public class DoSomeActionParameters
{
public string A { get; private set; }
public string B { get; private set; }
public DateTime C { get; private set; }
public OtherEnum D { get; private set; }
public string E { get; private set; }
public string F { get; private set; }
public DoSomeActionParameters(string a, string b, DateTime c, OtherEnum d,
string e, string f)
{
this.A = a;
this.B = b;
// ... tears erased the text here
}
}
As you can see I actually re-created my original problem: too many parameters. It's obvious that that's not the way to go. What am I going to do? The last option to achieve such immutability is to use a "readonly" struct like this:
public struct DoSomeActionParameters
{
public readonly string A;
public readonly string B;
public readonly DateTime C;
public readonly OtherEnum D;
public readonly string E;
public readonly string F;
}
That allows us to avoid constructors with too many parameters and achieve immutability. Actually it fixes all the problems (parameter ordering etc). Yet:
- Everybody (including FXCop & Jon Skeet) agree that exposing public fields are bad.
- Eric Lippert et al say relying on readonly fields for immutability is a lie.
That's when I got confused and decided to write this question: What's the most straightforward way in C# to avoid "too many parameters" problem without introducing mutability? Is it possible to use a readonly struct for that purpose and yet not have a bad API design?
CLARIFICATIONS:
- Please assume there is no violation of single responsibiltiy principle. In my original case the function just writes given parameters to a single DB record.
- I'm not seeking a specific solution to the given function. I'm seeking a generalized approach to such problems. I'm specifically interested in solving "too many parameters" problem without introducing mutability or a terrible design.
UPDATE
The answers provided here have different advantages/disadvantages. Therefore I'd like to convert this to a community wiki. I think each answer with code sample and Pros/Cons would make a good guide for similar problems in the future. I'm now trying to find out how to do it.
Use a combination of builder and domain-specific-language style API--Fluent Interface. The API is a little more verbose but with intellisense it's very quick to type out and easy to understand.
public class Param
{
public string A { get; private set; }
public string B { get; private set; }
public string C { get; private set; }
public class Builder
{
private string a;
private string b;
private string c;
public Builder WithA(string value)
{
a = value;
return this;
}
public Builder WithB(string value)
{
b = value;
return this;
}
public Builder WithC(string value)
{
c = value;
return this;
}
public Param Build()
{
return new Param { A = a, B = b, C = c };
}
}
DoSomeAction(new Param.Builder()
.WithA("a")
.WithB("b")
.WithC("c")
.Build());
One style embraced in the frameworks is usually like grouping related parameters into related classes (but yet again problematic with mutability):
var request = new HttpWebRequest(a, b);
var service = new RestService(request, c, d, e);
var client = new RestClient(service, f, g);
var resource = client.RequestRestResource(); // O params after 3 objects
Just change your parameter data structure from a class to a struct and you’re good to go.
public struct DoSomeActionParameters
{
public string A;
public string B;
public DateTime C;
public OtherEnum D;
public string E;
public string F;
}
public ResultEnum DoSomeAction(DoSomeActionParameters parameters, out Guid code)
The method will now get its own copy of the structure. Changes made to the argument variable cannot be observed by the method, and changes the method makes to the variable can not be observed by the caller. Isolation is achieved without immutability.
Pros:
- Easiest to implement
- Least change of behavior in underlying mechanics
Cons:
- Immutability is not obvious, requires developer attention.
- Unnecessary copying to maintain immutability
- Occupies stack space
What you have there is a pretty sure indication that the class in question is violating the Single Responsibility Principle because it has too many dependencies. Look for ways to refactor those dependencies into clusters of Facade Dependencies.
I'm not a C# programmer but I believe C# supports named arguments: (F# does and C# is largely feature compatable for that sort of thing) It does: http://msdn.microsoft.com/en-us/library/dd264739.aspx#Y342
So calling your original code becomes:
public ResultEnum DoSomeAction(
e:"bar",
a: "foo",
c: today(),
b:"sad",
d: Red,
f:"penguins")
this takes no more space/thought that your object creation and has all the benifits, of the fact that you haven't changed what is happening in the unerlying system at all. You don't even have to recode anything to indicate the arguments are named
Edit: here is a artical i found about it. http://www.globalnerdy.com/2009/03/12/default-and-named-parameters-in-c-40-sith-lord-in-training/ I should mention C# 4.0 supports named arguments, 3.0 did not
Why not just make an interface that enforces immutability (i.e. only getters)?
It's essentially your first solution, but you force the function to use the interface to access the parameter.
public interface IDoSomeActionParameters
{
string A { get; }
string B { get; }
DateTime C { get; }
OtherEnum D { get; }
string E { get; }
string F { get; }
}
public class DoSomeActionParameters: IDoSomeActionParameters
{
public string A { get; set; }
public string B { get; set; }
public DateTime C { get; set; }
public OtherEnum D { get; set; }
public string E { get; set; }
public string F { get; set; }
}
and the function declaration becomes:
public ResultEnum DoSomeAction(IDoSomeActionParameters parameters, out Guid code)
Pros:
- Doesn't have stack space problem like
structsolution - Natural solution using language semantics
- Immutability is obvious
- Flexible (consumer can use a different class if he wants)
Cons:
- Some repetitive work (same declarations in two different entities)
- Developer has to guess that
DoSomeActionParametersis a class that could be mapped toIDoSomeActionParameters
How about creating a builder class inside your data class. The data class will have all the setters as private and only the builder will be able to set them.
public class DoSomeActionParameters
{
public string A { get; private set; }
public string B { get; private set; }
public DateTime C { get; private set; }
public OtherEnum D { get; private set; }
public string E { get; private set; }
public string F { get; private set; }
public class Builder
{
DoSomeActionParameters obj = new DoSomeActionParameters();
public string A
{
set { obj.A = value; }
}
public string B
{
set { obj.B = value; }
}
public DateTime C
{
set { obj.C = value; }
}
public OtherEnum D
{
set { obj.D = value; }
}
public string E
{
set { obj.E = value; }
}
public string F
{
set { obj.F = value; }
}
public DoSomeActionParameters Build()
{
return obj;
}
}
}
public class Example
{
private void DoSth()
{
var data = new DoSomeActionParameters.Builder()
{
A = "",
B = "",
C = DateTime.Now,
D = testc,
E = "",
F = ""
}.Build();
}
}
I know this is an old question but I thought I'd wade in with my suggestion as I've just had to solve the same problem. Now, admittadly my problem was slightly different to yours as I had the additional requirement of not wanting users to be able to construct this object themselves (all hydration of the data came from the database, so I could jail off all construction internally). This allowed me to use a private constructor and the following pattern;
public class ExampleClass
{
//create properties like this...
private readonly int _exampleProperty;
public int ExampleProperty { get { return _exampleProperty; } }
//Private constructor, prohibiting construction outside of this class
private ExampleClass(ExampleClassParams parameters)
{
_exampleProperty = parameters.ExampleProperty;
//and so on...
}
//The object returned from here will be immutable
public ExampleClass GetFromDatabase(DBConnection conn, int id)
{
//do database stuff here (ommitted from example)
ExampleClassParams parameters = new ExampleClassParams()
{
ExampleProperty = 1,
ExampleProperty2 = 2
};
//Danger here as parameters object is mutable
return new ExampleClass(parameters);
//Danger is now over ;)
}
//Private struct representing the parameters, nested within class that uses it.
//This is mutable, but the fact that it is private means that all potential
//"damage" is limited to this class only.
private struct ExampleClassParams
{
public int ExampleProperty { get; set; }
public int AnotherExampleProperty { get; set; }
public int ExampleProperty2 { get; set; }
public int AnotherExampleProperty2 { get; set; }
public int ExampleProperty3 { get; set; }
public int AnotherExampleProperty3 { get; set; }
public int ExampleProperty4 { get; set; }
public int AnotherExampleProperty4 { get; set; }
}
}
You could use a Builder-style approach, though depending on the complexity of your DoSomeAction method, this might be a touch heavyweight. Something along these lines:
public class DoSomeActionParametersBuilder
{
public string A { get; set; }
public string B { get; set; }
public DateTime C { get; set; }
public OtherEnum D { get; set; }
public string E { get; set; }
public string F { get; set; }
public DoSomeActionParameters Build()
{
return new DoSomeActionParameters(A, B, C, D, E, F);
}
}
public class DoSomeActionParameters
{
public string A { get; private set; }
public string B { get; private set; }
public DateTime C { get; private set; }
public OtherEnum D { get; private set; }
public string E { get; private set; }
public string F { get; private set; }
public DoSomeActionParameters(string a, string b, DateTime c, OtherEnum d, string e, string f)
{
A = a;
// etc.
}
}
// usage
var actionParams = new DoSomeActionParametersBuilder
{
A = "value for A",
C = DateTime.Now,
F = "I don't care for B, D and E"
}.Build();
result = foo.DoSomeAction(actionParams, out code);
In addition to manji response - you may also want to split one operation into several smaller ones. Compare:
BOOL WINAPI CreateProcess(
__in_opt LPCTSTR lpApplicationName,
__inout_opt LPTSTR lpCommandLine,
__in_opt LPSECURITY_ATTRIBUTES lpProcessAttributes,
__in_opt LPSECURITY_ATTRIBUTES lpThreadAttributes,
__in BOOL bInheritHandles,
__in DWORD dwCreationFlags,
__in_opt LPVOID lpEnvironment,
__in_opt LPCTSTR lpCurrentDirectory,
__in LPSTARTUPINFO lpStartupInfo,
__out LPPROCESS_INFORMATION lpProcessInformation
);
and
pid_t fork()
int execvpe(const char *file, char *const argv[], char *const envp[])
...
For those who don't know POSIX the creation of child can be as easy as:
pid_t child = fork();
if (child == 0) {
execl("/bin/echo", "Hello world from child", NULL);
} else if (child != 0) {
handle_error();
}
Each design choice represent trade-off over what operations it may do.
PS. Yes - it is similar to builder - only in reverse (i.e. on callee side instead of caller). It may or may not be better then builder in this specific case.
here is a slightly different one from Mikeys but what I am trying to do is make the whole thing as little to write as possible
public class DoSomeActionParameters
{
readonly string _a;
readonly int _b;
public string A { get { return _a; } }
public int B{ get { return _b; } }
DoSomeActionParameters(Initializer data)
{
_a = data.A;
_b = data.B;
}
public class Initializer
{
public Initializer()
{
A = "(unknown)";
B = 88;
}
public string A { get; set; }
public int B { get; set; }
public DoSomeActionParameters Create()
{
return new DoSomeActionParameters(this);
}
}
}
The DoSomeActionParameters is immutable as it can be and cannot be created directly as its default constructor is private
The initializer is not immutable, but only a transport
The usage takes advantage of the initializer on the Initializer (if you get my drift) And I can have defaults in the Initializer default constructor
DoSomeAction(new DoSomeActionParameters.Initializer
{
A = "Hello",
B = 42
}
.Create());
The parameters will be optional here, if you want some to be required you could put them in the Initializer default constructor
And validation could go in the Create method
public class Initializer
{
public Initializer(int b)
{
A = "(unknown)";
B = b;
}
public string A { get; set; }
public int B { get; private set; }
public DoSomeActionParameters Create()
{
if (B < 50) throw new ArgumentOutOfRangeException("B");
return new DoSomeActionParameters(this);
}
}
So now it looks like
DoSomeAction(new DoSomeActionParameters.Initializer
(b: 42)
{
A = "Hello"
}
.Create());
Still a little kooki I know, but going to try it anyway
Edit: moving the create method to a static in the parameters object and adding a delegate which passes the initializer takes some of the kookieness out of the call
public class DoSomeActionParameters
{
readonly string _a;
readonly int _b;
public string A { get { return _a; } }
public int B{ get { return _b; } }
DoSomeActionParameters(Initializer data)
{
_a = data.A;
_b = data.B;
}
public class Initializer
{
public Initializer()
{
A = "(unknown)";
B = 88;
}
public string A { get; set; }
public int B { get; set; }
}
public static DoSomeActionParameters Create(Action<Initializer> assign)
{
var i = new Initializer();
assign(i)
return new DoSomeActionParameters(i);
}
}
So the call now looks like this
DoSomeAction(
DoSomeActionParameters.Create(
i => {
i.A = "Hello";
})
);
Use the structure, but instead of public fields, have public properties:
•Everybody (including FXCop & Jon Skeet) agree that exposing public fields are bad.
Jon and FXCop will be satisified because you are exposing properites not fields.
•Eric Lippert et al say relying on readonly fields for immutability is a lie.
Eric will be satisifed because using properties, you can ensure that the value is only set once.
private bool propC_set=false;
private date pC;
public date C {
get{
return pC;
}
set{
if (!propC_set) {
pC = value;
}
propC_set = true;
}
}
One semi-immutable object (value can be set but not changed). Works for value and Reference types.
A variant of Samuel's answer that I used in my project when I had the same problem:
class MagicPerformer
{
public int Param1 { get; set; }
public string Param2 { get; set; }
public DateTime Param3 { get; set; }
public MagicPerformer SetParam1(int value) { this.Param1 = value; return this; }
public MagicPerformer SetParam2(string value) { this.Param2 = value; return this; }
public MagicPerformer SetParam4(DateTime value) { this.Param3 = value; return this; }
public void DoMagic() // Uses all the parameters and does the magic
{
}
}
And to use:
new MagicPerformer().SeParam1(10).SetParam2("Yo!").DoMagic();
In my case the parameters were intentionally modifiable, because the setter methods didn't allow for all possible combinations, and just exposed common combinations of them. That's because some of my parameters were pretty complex and writing methods for all possible cases would have been difficult and unnecessary (crazy combinations are rarely used).
加载中,请稍侯......
精彩评论