1

Having a class such as this, with two getters for two instance variables:

class A
{
   _fieldA;
   _fieldB;

  GetA()
  GetB()

  GetSpecialNumber(int a)
  {
      //calculation not requiring any fields
  }
}

The class will be classified as lacking cohesion completely. However, I believe in some cases such a stateless object is desired and thus cohesion metric should not apply. Or is that a wrong approach/thinking? Truth is, I have never read about low cohesion being good, except for a few cases mentioned at the end of this material: http://www.aivosto.com/project/help/pm-oo-cohesion.html

John V
  • 4,855
  • 15
  • 39
  • 63
  • What I see is a DTO with read-only properties. What is wrong with a DTO? – Hasan Emrah Süngü Feb 05 '18 at 08:34
  • What you've got here are not really methods but read-only properties. So, no. Cohesion metrics don't apply here, IMHO. ("Not really methods" = They don't contain any logic that works with the fields.) – Fildor Feb 05 '18 at 08:35
  • @Fidor, right, then lets say those are methods that do something with the respective fields. As they do not work with the other one, cohesion is 0. – John V Feb 05 '18 at 08:36
  • 1
    @user970696 Do not get too fixated on metrics. They are not written in stone. They should give you a rough idea what is going on in your solutions. If you must keep your cohesion high, then only thing you can do is separate that class `A` into two classes `X` and `Y`, which only has either of the fields – Hasan Emrah Süngü Feb 05 '18 at 08:41
  • @EmrahSüngü Yes. I am rather trying to understand whether the examples in the link I provided are actually valid and that low cohesion can be really justified in some cases. – John V Feb 05 '18 at 08:42
  • You'll almost always find that one special case where it is reasonable to break a specific "clean code" guideline. The thing is: You need to be aware that it *is* that one special case when you encounter it and if you do don't be too dogmatic about it. Just make it stay a special case and not your new "normal". – Fildor Feb 05 '18 at 08:46

2 Answers2

2

No, low cohesion is not good, at least not if you are aiming for object-orientation.

Now, to be fair and to warn you, this is probably not a majority opinion, but DTOs, Beans, Properties, or whatever you call them are not well designed (as the linked article seems to suggest). Again, only if you care about object-orientation. If you don't care that much, then of course you may decide what you want to do.

Obviously there are subtleties, like is a given metric really correct, or whether outside forces (requirements) pull you toward low coupling. However, the reason we want to avoid low coupling is that we want to have things that change together in one place for maintainability. Low coupled things will probably not change together, therefore less maintainability.

Low cohesion sometimes leads to high coupling. For example DTOs (low cohesion objects) lead to high coupling by design. Every time anything in the DTO changes, all the places where that DTO is used need to be examined. That is, those places are highly coupled to the DTO.

Robert Bräutigam
  • 7,514
  • 1
  • 20
  • 38
  • Thank you, very interesting. Also what would you think of the cases mentioned at the end of the article I referenced in the question? – John V Feb 05 '18 at 10:36
  • 1
    I do not agree that those cases represent good design. You can perhaps find very specific cases where those designs might make sense, but not in general. Grouping data or procedures for convenience is not how good designs are made. – Robert Bräutigam Feb 05 '18 at 13:52
  • I don't agree with this, using DTOs helps you organizing your code in a proper way, making each object responsible of its contents. With more abstraction (dtos), maintanability INCREASES as there are more customization points in the code. Everyone can write shitty code using one approach or another, but if you compare 2 proper designs, the one with SOLID principes is the best one in customization points, even if it has low cohesion – Ivan Sanz Carasa Feb 05 '18 at 17:35
  • @IvanSanz-Carasa You can *not* be responsible for things that are public because you simply can't control how they are used. Everything in DTOs is public, therefore DTOs have no responsibilities. Also, in my thinking that also means they shouldn't exist. – Robert Bräutigam Jul 02 '19 at 09:38
1

For this case, I would use properties instead of fields, this helps some tools to understand that this is a DTO (which is a correct thing) so they stop complaining about cohesion and code quality.

struct X
{
    public int A { get; private set; }
    public int B { get; private set; }
}

In case GetSpecialNumber(int a) doesn't use any of the fields/properties, it can be a static method:

public static int GetSpecialNumber(int a)

I would also move it to a helper class if it will be used from somewhere else.

public static class SpecialNumberHelper
{
    public static int GetSpecialNumber(int a)
    {
        // calculation not requiring any fields
    }
}
Ivan Sanz Carasa
  • 1,141
  • 8
  • 16
  • Understood. I am mainly interested if low cohesion can be ever good with actual methods. I modified the code in the question. Now there is another method not working with any fields, i.e. cohesion still 0. – John V Feb 05 '18 at 08:41
  • in case GetSpecialNumber doesn't require any of the fields, it should be moved as static method to some generic helper – Ivan Sanz Carasa Feb 05 '18 at 08:48
  • Well but if this is that helper? Like Math class or so, counting square or factorial, where all you need is an input. I think in that case cohesion will be low too. – John V Feb 05 '18 at 08:50
  • @IvanSanz not necessarily. It really depends. Imagine: A class that has a method to compute some ID for you. It also provides 2 const values: "EmptyID" and "PoisonID". I'd say it is totally reasonable to have them in one (utilitiy) class. – Fildor Feb 05 '18 at 08:50
  • @user970696 helpers are static classes with static methods and no state (well... they can have static concurrent stuff inside for caching purposes etc) – Ivan Sanz Carasa Feb 05 '18 at 08:53
  • So the examples the link provided are not valid? – John V Feb 05 '18 at 08:53
  • "So the examples the link provided are not valid?" - What examples specifically? And why should the answer make those invalid? – Fildor Feb 05 '18 at 09:01
  • At the end of the artcle there are examples. Your conclusion seems to be different than theirs, in terms of separating methods of helper classes etc. – John V Feb 05 '18 at 09:50