C# – How to argue against this “completely public” mindset of business object class design

cclassclass-designobject-oriented-design

We're doing a lot of unit testing and refactoring of our business objects, and I seem to have very different opinions on class design than other peers.

An example class that I am not a fan of:

public class Foo
{
    private string field1;
    private string field2;
    private string field3;
    private string field4;
    private string field5;

    public Foo() { }

    public Foo(string in1, string in2)
    {
        field1 = in1;
        field2 = in2;
    }

    public Foo(string in1, string in2, string in3, string in4)
    {
        field1 = in1;
        field2 = in2;
        field3 = in3;
    }

    public Prop1
    { get { return field1; } }
    { set { field1 = value; } }

    public Prop2
    { get { return field2; } }
    { set { field2 = value; } }

    public Prop3
    { get { return field3; } }
    { set { field3 = value; } }

    public Prop4
    { get { return field4; } }
    { set { field4 = value; } }

    public Prop5
    { get { return field5; } }
    { set { field5 = value; } }

}

In the "real" class, they're not all string, but in some cases we have 30 backing fields for completely public properties.

I hate this class, and I don't know if I'm just being picky. A few things of note:

  • Private backing fields with no logic in the properties, seems unnecessary and bloats the class
  • Multiple constructors (somewhat ok) but coupled with
  • all properties having a public setter, I'm not a fan.
  • Potentially no properties would be assigned a value due to the empty constructor, if a caller is unaware, you could potentially get some very unwanted and hard to test for behavior.
  • It's too many properties! (in the 30 case)

I find it much more difficult to really know what state the object Foo is in at any given time, as an implementer. The argument was made "we might not have the necessary information to set Prop5 at time of object construction. Ok, I guess I can understand that, but if that's the case make only Prop5 setter public, not always up to 30 properties in a class.

Am I just being nit-picky and/or crazy for wanting a class that is "easy to use" as opposed to being "easy to write (everything public)"? Classes like the above scream to me, I don't know how this is going to be used, so I'm just going to make everything public just in case.

If I'm not being terribly picky, what are good arguments to combat this type of thinking? I'm not very good at articulating arguments, as I get very frustrated trying to get my point across (not intentionally of course).

Best Answer

Completely public classes have a justification for certain situations, as well as the other extreme, classes with only one public method (and probably lots of private methods). And classes with some public, some private methods as well.

It all depends on the kind of abstraction you are going to model with them, which layers you have in your system, the degree of encapsulation you need in the different layers, and (of course) what school of thought the author of the class comes from. You can find all of these types in SOLID code.

There are entire books written about when to prefer which kind of design, so I am not going to list any rules here about it, the space in this section would not be sufficient. However, if you have a real world example for an abstraction you like to model with a class, I am sure the community here will happily help you to improve the design.

To address your other points:

  • "private backing fields with no logic in the properties": Yes, you are right, for trivial getters and setters this is just unneccessary "noise". To avoid this kind of "bloat", C# has a short-cut syntax for property get/set methods:

So instead of 

   private string field1;
   public string Prop1
   { get { return field1; } }
   { set { field1 = value; } }

write

   public string Prop1 { get;set;}

or

   public string Prop1 { get;private set;}

  • "Multiple constructors": that is not a problem in itself. It gets a problem when there is unnecessary code duplication in there, like shown in your example, or the calling hierarchy is convoluted. This can be easily solved by refactoring common parts into a separate function, and by organizing the constructor chain in a unidirectional manner

  • "Potentially no properties would be assigned a value due to the empty constructor": in C#, every datatype has a clearly defined default value. If properties are not initialized explicitly in a constructor, they get this default value assigned. If this is used intentionally, it is perfectly ok - so an empty constructor might be ok if the author knows what he is doing.

  • "It's too many properties! (in the 30 case)": yes, if you are free to design such a class in a greenfield manner, 30 are too many, I agree. However, not everyone of us has this luxury (did you not write in the comment below it is a legacy system?). Sometimes you have to map records from an existing database, or file, or data from a third party API to your system. So for these cases, 30 attributes might be something one has to live with.

Related Topic