current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I have this class containing two constructors with different signatures but they do the same thing:

public Person(Dictionary dictionary, string someString)
    : base(dictionary, someString)
{
    base.GetProperty("FirstName");
    base.GetProperty("LastName");
}

public Person(Dictionary dictionary, string[] someStringArray)
    : base(dictionary, someStringArray)
{
    base.GetProperty("FirstName");
    base.GetProperty("LastName");
}

The type of the second parameter in each of these constructors determines the behavior of the 'GetProperty' method.

I've read about casting and the bad behaviors of it, but would it be appropriate to do something like this:

public Person(Dictionary dictionary, Object something)
{
}

and then cast something to either a string or string[] depending on whichever is appropriate?

share|improve this question
    
Why must there be 2 constructors? Why can't there be only one: public Person(Dictionary dct, string[] something). Simpler, better for the client. –  radarbob Oct 13 '13 at 2:11
    
In theory yes, I could just use one string[] and if its string just pick string[0] But the behavior between processing string and string[] are different, also the determination of string and string[] is based elsewhere. The object itself does not determine if the parameter is a string or string[]. I guess this is very vague. Im trying to add in a better example. –  Rhs Oct 14 '13 at 13:12
    
behavior between processing string and string[] are different. So string[0] is not the same thing/object/property as string in the other contractor, yes? How about optional parameters: public Person(Dictionary dict, string[] otherStuff, string Fname = null). –  radarbob Oct 14 '13 at 21:50
    
@radarbob I like the idea of optional parameter. I'll see if it makes sense to incorporate that in the scope of my project. –  Rhs Oct 15 '13 at 12:58

1 Answer 1

up vote 7 down vote accepted

Two things your could do:

  1. Refactor the common code into a method (like Initialize) and call that:

    private void Initialize()
{
    base.GetProperty("FirstName");
    base.GetProperty("LastName");
}

public Person(Dictionary dictionary, string someString)
    : base(dictionary, someString)
{
    Initialize();
}

public Person(Dictionary dictionary, string[] someStringArray)
    : base(dictionary, someStringArray)
{
    Initialize();
}

  2. Not sure whether that's an option as I don't know how the behaviour changes but you could reduce one case to the other:

    public Person(Dictionary dictionary, string someString)
    : this(dictionary, new [] { someString })
{
}

public Person(Dictionary dictionary, string[] someStringArray)
    : base(dictionary, someStringArray)
{
    base.GetProperty("FirstName");
    base.GetProperty("LastName");
}

Apart from that:

  • I'd avoid the casting. Apparently only strings or arrays of strings make sense to be passed in. If you change the parameter to object then it is no longer clear to the caller what he can and cannot pass in and would have to write a test to make sure that what he is passing in will be accepted. It reduces the clarity of the interface.

  • GetProperty seems to be a strange thing to call in a constructor. I'd expect it to return a property value yet you do nothing with the return value.

  • Consider making "FirstName" and "LastName" (and any other property string you use) string constants rather than literals - especially if you use them in more than one place. Lambda expressions might be an option if they are actual properties on the object.

share|improve this answer
    
In this case, both suggestions are OK, but generally I prefer the second one, because it allows me to use readonly modifier on fields. Further more, if the literals FirstName and LastName are used more then once in code, they should become as private static const string fields. Last but not least, if FirstName is the name of a property in base type, try to uses lambda expressions (type safe) to select them. –  user27811 Oct 9 '13 at 8:05
    
@hichaeretaqua: Good points, although I suspect the properties are supplied via the dictionary in which case lambda expressions won't be an option. Just a guess though. –  ChrisWue Oct 9 '13 at 8:20
    
@ChrisWue Exactly!. The dictionary and the string or the string[] determines what GetProperty does. –  Rhs Oct 14 '13 at 13:14

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.