9

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

  • 2
  • 3
    An object should always be in a valid state; that is, you should not need to have partially instantiated objects. Some information is not part of an object's core state whilst other information is (e.g. a table must have legs but the cloth is optional); optional information can be provided after instantiation, core information must be provided at instantiation. If some information is core but is not known at instantiation then I'd say the classes need refactoring. It's difficult to say more without knowing the context of the class. – Marvin Mar 9 '16 at 17:25
  • 1
    Saying "we might not have the necessary information to set Prop5 at time of object construction" makes me ask "are you saying that there will be some times that you will have that information at the time of construction and other times when you won't have that information?" It makes me wonder if there are actually two different things that this one class is trying to represent, or perhaps whether this class should be broken into smaller classes. But if it's done "just in case" that's also bad, IMO; that says to me that there's not a clear design in place for how objects get created/populated. – Dr. Wily's Apprentice Mar 9 '16 at 18:14
  • 3
    For the private backing fields with no logic in the properties, is there a reason why you aren't using auto properties? – Carson63000 Mar 10 '16 at 6:11
  • 2
    @Kritner the whole point of auto-properties is that if you do need actual logic in there in the future, you can seamlessly add it in place of the auto get or set :-) – Carson63000 Mar 11 '16 at 2:09
10

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.

  • Thanks, yeah I know POCO/POJOs have their place, and my example is quite abstract. I don't think I could get more specific without going into a novel however. – Kritner Mar 9 '16 at 18:11
  • @Kritner: see my edit – Doc Brown Mar 10 '16 at 6:53
  • yeah usually when I'm creating a brand new class I go the auto property route. Having the private backing fields "just because" is (in my mind) unneeded and even causes issues. When you have ~30 properties to a class and 100+ classes, it's not far fetched to say there's going to be some Properties setting or getting from the incorrect backing field... I've already encountered this several times in our refactoring in fact :) – Kritner Mar 10 '16 at 11:39
  • thanks, the default value for string is null is it not? so if one of the properties that is actually used in a .ToUpper(), but a value is never set for it, it would throw a runtime exception. This is another good example why the required data for the class should have to be set during object construction. Not just leave it up to the user. THanks – Kritner Mar 11 '16 at 12:53
  • In addition to too many unset properties, the main problem with this class is that it has zero logic (ZRP) and is the archetype for an Anemic Model. Without a need for this that can be expressed in words, such as a DTO, it is a poor design. – user949300 Mar 11 '16 at 16:47
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.

  • 1
    Thanks - #1 and #4 I definitely feel the most comfortable with making my argument. Not quite sure what you mean in #3. #5 most of the properties in the classes make up the primary key on the db. We use competitive keys, sometimes up to 8 columns - but that's another issue. I was thinking about trying to put the parts that make up the key into its own class/struct, which would eliminate a lot of properties (at least in this class) - but this is a legacy application with thousands of lines of code with multiple callers. So I guess I have a lot to think about :) – Kritner Mar 9 '16 at 18:15
  • Eh. If the class was immutable, there'd be no reason to write any synchronization code inside the class. – RubberDuck Mar 10 '16 at 12:18
  • @RubberDuck Is this class immutable? What do you mean? – Snoop Mar 10 '16 at 12:20
  • 3
    It's definitely not immutable @StevieV. Anything class with property setters is mutable. – RubberDuck Mar 10 '16 at 13:34
  • 1
    @Kritner How do the properties get used as primary keys? That presumably requires some logic (null checks) or rules (e.g. NAME takes priority over AGE). According to OOP, this code belongs in this class. Could you use that as an argument? – user949300 Mar 11 '16 at 16:53
2

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; }
}
2

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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