Fundamentals of OOD Part 3: Method Cohesion

Single Responsibility Principle (SRP) helps us write more cohesive types and methods.  Cohesion is the relatedness of the members of a type to each other and the relatedness parts of a method’s code to other parts.

Method cohesion
Often times a method is not very cohesive, meaning the code that it executes relates to more than one thing.  This can often be seen with a method that contains a large switch statement.  For any invocation of the method only one case statement may be executed; meaning that blocks of code within the method don’t relate to all the other blocks.  Switch statements are often an indication that the design should be changed to be more polymorphic or introduce a pattern like the Strategy or Template Method patterns.  Likely a concept of the design is implicit instead of explicit (at least no more explicit than an enum).  For example:

    public class Set {

        public enum Operation {

            Unknown,

            Union,

            Intersection,

            RelativeComplement,

            And,

            Conjunction = And

        }

 

        public static IList<int> PerformOperation(IList<int> one, IList<int> two, Operation operation) {

            List<int> result = new List<int>();

            switch (operation) {

                case Operation.Union:

                    result.AddRange(one);

                    result.AddRange(two);

                    break;

                case Operation.Intersection:

                    foreach (int x in one) {

                        if (two.Contains(x)) {

                            result.Add(x);

                        }

                    }

                    break;

                case Operation.RelativeComplement:

                    foreach (int x in one) {

                        if (!two.Contains(x)) {

                            result.Add(x);

                        }

                    }

                    break;

                case Operation.And:

                    foreach (int x in one) {

                        if (two.Contains(x)) {

                            result.Add(x);

                        }

                    }

                    break;

                default:

                    throw new ArgumentOutOfRangeException(“operation”);

            }

            return result;

        }

    }


This code works; but it’s far from cohesive.  There are many combinations of execution paths this method can take, and each path is unrelated to the other paths.  For example, the Union case has no relation to any of the other cases.  Methods like this are also hard to maintain and prone to errors.  Obviously if another member were added to Operation PerformOperation would have to change–making PerformOperation tightly coupled to Operation–not very object oriented.

This can be made more object-oriented by through Dependency Inversion and the Strategy Pattern:

    public class Set2 {

        public abstract class Operation {

            public abstract IList<int> Execute(IList<int> left, IList<int> right);

        }

 

        public class UnionOperation : Operation {

            public override IList<int> Execute(IList<int> left, IList<int> right) {

                List<int> result = new List<int>();

                result.AddRange(left);

                result.AddRange(right);

                return result;

            }

        }

 

        public class IntersectionOperation : Operation {

            public override IList<int> Execute(IList<int> left, IList<int> right) {

                List<int> result = new List<int>();

                foreach (int x in left) {

                    if (right.Contains(x)) {

                        result.Add(x);

                    }

                }

                return result;

            }

        }

 

        public class RelativeComplementOperation : Operation {

            public override IList<int> Execute(IList<int> left, IList<int> right) {

                List<int> result = new List<int>();

                foreach (int x in left) {

                    if (!right.Contains(x)) {

                        result.Add(x);

                    }

                }

                return result;

            }

        }

 

        public class AndOperation : Operation {

            public override IList<int> Execute(IList<int> left, IList<int> right) {

                List<int> result = new List<int>();

                foreach (int x in left) {

                    if (right.Contains(x)) {

                        result.Add(x);

                    }

                }

                return result;

            }

        }

        public static IList<int> PerformOperation(IList<int> left, IList<int> right, Operation operation) {

            return operation.Execute(left, right);

        }

    }

Now each operation is encapsulated, explicit, PerformOperation need not change as new strategies are added, and we’ve completely avoided the InvalidOperationException.

kick it on DotNetKicks.com

12 thoughts on “Fundamentals of OOD Part 3: Method Cohesion”

  1. Great post, but for the AndOperation, isn’t the second foreach unnecessary? Since it has to be in BOTH it’s enough to just check one collection, in other words, there will never be an item that will be added in the second foreach :)

  2. Hello, first I would like to say that you inspire me, your blogs are enjoyable. :)

    The “Operation” type contain no implementations, so I wonder for the reason you used abstract class over an interface, can you enlighten me with reason ? or was it just for the example ? (silly as it may sound).

    Thank you for your time, regards!

  3. Eyal-Shilony. Good question. I chose an abstract class over an interface because I think it’s more clear in this circumstance. It’s more clear that the strategy implementation is expected to do only one thing.

    I this case it does only one thing, we don’t use other classes that might do other things so rather than over-design we use the simplest thing that fulfills our requirements. Should you want to have a class that can fulfill more than one interface contract then use of interface might be more appropriate. I generally think that implementing more than one interface likely means the class is violating SRP. In this case we’re more likely to implement a class that has a single responsibility because it can only have one base (the abstract Operation).

    But, yes, interface could have just as easily been used.

  4. But that means Intersection and And are the same :)

    But that wasn’t the point I know :D

    Great article, I just wish more developers would know this :(

  5. Hello, your blogs always make a dumb like me to understand things in such a simpler way. So i say thanks to you. After reading this i actually questioned my senior and asked him what pattern would he use in the scenario you presented. He said he would use factory pattern(though i dont agree). His stance was that Strategy and Factory patterns are quite similar so they can be use interchangeably. My question is can factory pattern be used for the problem you presented.

  6. Assam, I’m glad they help.

    The factory pattern implements no business logic (the policy) the Strategy pattern does implement business logic. You could use a factory pattern to decouple the code that uses an Operation implementation from the various implementations; but then you’d be using Factory and Strategy.

    No, the Factory pattern and the Strategy pattern are not interchangeable. Factory merely abstracts the creation of objects. Strategy encapsulates and abstracts policy.

  7. Felipe, my example is very simple. I a real-world case after you perform this change you may actually want to refactor to what you’ve suggested. It depends on the project and depends on what you want to accomplish. “PerformOperation” isn’t very descriptive and isn’t very intention-revealing; so, yes, simply calling the Operation-derived class directly may be a valid refactoring. If “PerformOperation” has a more descriptive name–being more intention revealing–then refactoring to just calling the Operation-derived class directly would likely make the code less clear.

  8. Great blog, very informative!

    I have a question, what are your thoughts on creating an extension method of PerformOperation? This way PerformOperation is not ‘physically’ part of the Operation-derived class, however, one can still invoke it as if.

    One may wonder, however, what the added value would be over just calling Execute on the object.

Leave a Reply

Your email address will not be published. Required fields are marked *

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>