How to argue against this "completely public" mindset of business object class design

Question

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).

Answer 1

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.

Answer 2

1. By saying that "Private backing fields with no logic in the properties, seems unnecessary and bloats the class" you already have a decent argument.

2. It doesn't seem like multiple constructors are the issue here.

3. As for having all properties as public... Maybe think of it this way, if you ever wanted to synchronize the access to all of these properties among threads you would have a tough time because you might have to write synchronization code everywhere they're used. However, if all of the properties were enclosed by getters/setters then you could easily build the synchronization into the class.

4. I think when you say "hard to test behavior" the argument speaks for itself. Your test might have to check if it's not null or something like that (every single place the property is used). I don't really know because I don't know what your test/application looks like.

5. You're right way too many properties, could you try using inheritance (if the properties are similar enough) and then have a list/array using generics? Then you could write getters/setters to access the information and simplify the way you'll interact will all the properties.

Answer 3

As you told me, Foo is a business object. It should encapsulate some behaviour in methods according to your domain and its data must remain as encapsulated as possible.

In the example you show, Foo looks more like a DTO and goes against all OOP principles (see Anemic Domain Model) !

As improvements I would suggest:

• Make this class as immutable as possible. As you said, you want to make some unit testing. One mandatory capability for unit testing is determinism, and determinism can be achieved via immutability because it solves the side-effects problems.
• Break this god class into multiple classes that do only 1 thing each (see SRP). Unit test a 30 attributes class in really a PITA.
• Remove constructor redundancy by having only 1 main constructor and the others calling the main constructor.
• Remove unnecessary getters, I seriously doubt all getters are really useful.
• Bring back business logic in business classes, this is where it belongs !

This could be a potential refactored class with these remarks:

public sealed class Foo
{
    private readonly string field1;
    private readonly string field2;
    private readonly string field3;
    private readonly string field4;

    public Foo() : this("default1", "default2")
    { }

    public Foo(string in1, string in2) : this(in1, in2, "default3", "default4")
    { }

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

    //Methods with business logic

    //Really needed ?
    public Prop1
    { get; }

    //Really needed ?
    public Prop2
    { get; }

    //Really needed ?
    public Prop3
    { get; }

    //Really needed ?
    public Prop4
    { get; }

    //Really needed ?
    public Prop5
    { get; }
}

Answer 4

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

• "There are several methods scattered in the client class(es) doing more than 'mere DTO duty'. They belong together."
• "It may be a 'DTO' but it has business identity - we need to override Equals."
• "We need to sort these - we need to implement IComparable"
• "We're stringing together several of these properties. Let's override ToString so every client doesn't have to write this code."
• "We have multiple clients doing the same thing to this class. We need to DRY up the code."
• "The client is manipulating a collection of these things. It's obvious it should be a custom collection class; and many of the earlier points will make that custom collection more functional than currently."
• "Look at all the tedious, exposed, string manipulation! Encapsulating that in business-relevant methods will increase our coding productivity."
• "Pulling these methods together into the class will make them testable because we don't have to deal with the more complex client class."

• "We can now unit test those refactored methods. As it is, for reasons x,y,z the client is not testable.

• To the extent any of the above arguments can be made, in aggregate:

  • The functionality becomes re-usable.
  • It's DRY
  • It's decoupled from the clients.
  • coding against the class is faster and less error prone.
• "A well done class hides state and exposes functionality. This is the starting proposition for designing OO classes."
• "The final result does a far better job of expressing the business domain; the distinct entities and their explicit interaction."
• "This 'overhead' functionality (i.e. not explicit functional requirements, but it's gotta be done) clearly stands out and adheres to Single Responsibility Principle.