4

I'm working on a legacy code and I have seen a lot of code like this:

public class Person
{
    public Person(PersonData data)
    {
        this.Name = data.Name;
        this.Gender = data.Gender ;
    }

    public String Name { get; private set;}
    public String Gender { get; private set;}
}

public class PersonData
{
    public String Name;
    public String Gender;
}

public static Person ReadPerson(Reader reader)
{
    PersonData data = new PersonData;
    data.Name = reader.ReadString();
    data.Gender = reader.ReadString();

    Person p = new Person(data);
    return p;
}

The PersonData class exists for setting the private fields in Person class in its constructor. Other than that, the PersonData class introduces redundant code, as you can see now you have Name and Sex in both Person and PersonData class.

In my opion, this kind of design doesn't scale: now I have a new field "Age" to read, I have to add the "Age" property in two different places.

Is this a valid design choice (given I have see a lot code like this in the legacy code)?
How can I refactor this?

EDIT:

Those two classes are simplified version of the real code. So please forgive using string instead of enum for gender.

In the real code the PersonData have more than 10 fields so as Person class.

Improve this question
7
  • In some programming models, you sometimes have to define 2 containers for the same property. For instance, in MVVM (the only model I know btw!), often I have to create my property in my "LOB" objects, then I have to create it again in the ViewModel. Then I act on the viewmodel which updates the LOB. I have no idea if this covers your scenario, but that's that. :) Commented Nov 17, 2012 at 1:10
  • Nit: Usually Sex is a boolean value sometimes followed with "please?". Try Gender (or some variation of) which is usually constrained based upon domain (Gender and Preference/Association may be further separated) as to what values are accepted: a simple enum might include Male, Female, Other (but could be more/different and in a simplified form only considers the first two options). Commented Nov 17, 2012 at 1:11
  • (Interesting read: Wikipedia: Third Gender) Commented Nov 17, 2012 at 1:18
  • what version is this legacy code ? if it's 3.0 you don't need any of that and can use object initializers Commented Nov 17, 2012 at 1:27
  • @user1416420 Unfortunately it's in C++/CLI (one assembly) and Managed C++ (another assembly), I cannot even use C# to deal with it... Commented Nov 17, 2012 at 1:38

5 Answers 5

Reset to default
3

Using a parameter object is a valid approach to take when using Constructor Injection and you start getting a large number of parameters in your constructor - however it is unnecessary when there are fewer parameters like you have.

Here is a suggestion:

public class Person
{
    public Person(string name, string sex)
    {
        _name = name;
        _sex = sex;
    }

    public string Name { get {return _name; }}
    public string Sex { get {return _sex; }}

    private readonly string _name, _sex;
}

This makes the the class immutable.

Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

My example is just a simplified version, the real code does have a lot of fields in the PersonData class, so in that case you would suggest using parameter object even introducing duplication?
+1. It could have happened to create consistently looking code: random classes with constructors with many arguments -> large number of arguments in constructors change to use "parameters object". Now 10% of objects look different since they don't have constructors taking single parameter -> change remaining for consistency.
@AZ, yes parameters object is better than constructor with 3+ arguments. The other option is to move reading into each object directly. If you don't need to guarantee that external callers can't modify Person objects consider dropping PersonData altogether and make fields R/W. Also immutable objects are much easier to reason about, so I'd try not to do so.
@AZ. That is a tough question to answer without seeing the actual code, and the answer would probably involve a mix of purity and pragamatism. In this case your parameter object is not vastly different to the class you are initialising. If you have lots of this then the answer could be to rationalise the classes into a better heirarchy, or maybe just re-evaluate what properties need to be read-only.
1

If it would be some sort of externally facing objects (unlike data transfer objects that seem to be in your case) you can consider fluent interfaces to build them out, it not going to decrease number of classes but will let you construct object in more fancy looking way along with better control of what is required and what is optional.

See posts tagged with fluent-interface if interested. I.e. Conditional Builder Method Chaining Fluent Interface :

var person = PersonBuilder
  .CreatePerson()
    .Named(reader.ReadString())
    .Sex(reader.ReadString())
    .Build()
Improve this answer

1 Comment

+1 Ha, I like fluent interface! I recently built one in another project but never thought it can be used in this way.
0

Get rid of PersonData and feed the Reader to the Constructor:

public sealed class Person
{
    public Person(Reader reader)
    {
        this.Name = reader.ReadString();
        this.Sex = reader.ReadString();
    }

    public string Name { get; private set; }

    public string Sex { get; private set; }
}
Improve this answer

8 Comments

What if he wants to create instance from any other input?Does he have to store in a database and then read it?
I don't really like this idea as it pollutes the Person class with Reader. The Person class really shouldn't care about how the reader reads.
Actually, yes. The way the Person class was used in his example looks like it is retrieved from a data repository of some kind. To create a new Person I would add a repository method AddPerson(NewPerson newPerson) where NewPerson is an object created by the frontend. The Person class itself can only be instantiated from the data source. That way you always know whether your object is persisted or not.
@AZ. Person would implement IPerson. And only IPerson would be used outside of the data Adapter.
+1. Good approach in general, but AZ's call for particular project.
|
0

In general, I'd go back to the real-world (or business :)) system you are modelling. If the class matches something in that world, then it is fine. If the class is purely an artifact of the programming system and also appears unnecessary, I'd toss it. Using the "data" class can also hide various problems that using an explicit parameter would catch. For example, when you add "age" how will you detect that all cases are found? If you add it as a constructor parameter, you will get an error for every missing case.

Improve this answer

2 Comments

The thing is then you have 20 parameters in your constructor, not mentioning you have to be careful about the ordering of them...
The fact is you have 20 parameters. That is an element of what you are modelling. Hiding them in a struct doesn't change it. I agree the ordering is touchy. Anyway, the whole question is a bit subjective :)
0

One way would be instead of 

public String Name { get; private set;}
public String Sex { get; private set;}

to expose a property of type PersonData 

public class Person
{
    public PersonData PersonData { get; }
}

Also you can look at deriving Person from PersonData.

Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.