Single Responsibility Principle – discovering violations

Artigo
06/15/2011

A class should have only one reason to change. Responsibilities are reasons to change.

Violations of this principle leave you face to face with fragile design and all the maintenance nightmares it implies. Unfortunately there is no hundred percent way to prevent it from happening – it is just the nature of design.

Common sense often helps to spot multiple responsibilities mixed within a single class. However some cases are tricky and require careful attention. This is where design assessment comes in. An object can be looked at from two perspectives:

Data
- What it knows?
- What connections between objects it maintains?
Behavior
- What it decides?
- What services it performs?

Uncontrolled growth in any of the groups can lead to god class (data or behavior forms respectively).

What it knows?

Keeping related data and behavior in one place is essential to building consistent abstractions. Failing to do so may lead to:

unnecessary details disclosure (by exposing all the data necessary to implement behavior)
duplicated or inconsistent behavior (due to poor discoverability)
uncontrolled growth (as it is harder to see the big picture due to scattered behavior)

Following Information Expert pattern promotes the idea:

Assign a responsibility to the class that has the information needed to fulfill it.

Consider an example below where publicly visible person name could be incorrectly used to distinguish people (as names are not unique). Thus it makes sense to let Person type to define what equality means as it has all the necessary information (social security number). This will prevent introduction of several inconsistent equality behaviors across the code base.

 class Person
{
    private string name;
    // Keep social security number in private
    private string ssn;
    
    // Names are not unique
    public string Name { get { return name; } }

    public override bool Equals(object obj)
    {
        if (obj == null || obj.GetType() != typeof(Person))
            return false;
        // Names cannot be used to identify people unlike 
        // social security number
        return ssn == ((Person) obj).ssn;
    }
}

Keeping related data and behavior in one place is as important as not letting not related sets of data/behavior to be put into the same class. Most of the methods defined on a class should be using most of the data members most of the time.

Many of you started to work still being a student. Employer needs your abilities to work and most likely he isn’t interested in that you still studying.

 // Smart student - works and studies
class Student
{
    private string name;
    private int knowledge;
    private Func<Course, bool> preferences;
    private int experience;

    public string Name
    {
        get { return name;}
    }

    public void Study()
    {
        knowledge++;
    }

    public void Enlist(IEnumerable<Course> courses)
    {
        // Select appropriate courses and enlist
        foreach (var course in courses.Where(preferences))
            course.Enlist(name);
    }

    public void Work()
    {
        experience++;
    }

    public void Sign(Contract contract)
    {
        // sign job contract with your name
        contract.Agree(name);
    }
}

This class clearly has more than one responsibility and the fact that study related methods and work related methods both operate on a subset of data shows that. However separation can solve the problem.

 class Student
{
    private Person person;
    private int knowledge;
    private Func<Course, bool> preferences;

    public Student(Person newStudent)
    {
        person = newStudent;
    }

    public void Study()
    {
        knowledge++;
    }

    public void Enlist(IEnumerable<Course> courses)
    {
        // Select appropriate courses and enlist
        foreach(var course in courses.Where(preferences))
            course.Enlist(person.Name);
    }
}

class Employee
{
    private Person person;
    private int experience;

    public Employee(Person newEmployee)
    {
        person = newEmployee;
    }

    public void Work()
    {
        experience++;
    }

    public void Sign(Contract contract)
    {
        // sign job contract with person's name
        contract.Agree(person.Name);
    }
}

What connections between objects it maintains?

What is connection between objects anyway? For example, a document can be seen a set of glyphs (text, graphics, structural elements like columns and rows). The connection between them is the fact they belong to the same document. So basically the document maintains connection between glyphs. So what must attract our attention in this area to spot Single Responsibility Principle violations?

If the object maintains connections between objects of different abstraction levels it is highly likely that it does someone’s job.

Consider an object model where a man wants to build a house. Man class maintains connection between tasks (got from an architect) and workers to build a house. And this where different abstraction levels (from domain model perspective) meet – a man hired foreman who is supposed to manage construction but it so happens that a man wants to control everything and treats hired foreman as a simple worker giving him tasks to do in not necessarily correct order. Basically a man got foreman’s responsibility (or in other words foreman abstraction isn't consistent).

 abstract class Task
{
    public bool Completed { get; private set; }

    public virtual void Do()
    {
        DoCore();
        Completed = true;
    }

    protected abstract void DoCore();
}

class Worker
{
    public virtual bool CanDo(Task task)
    {
        // Can do everything worker =)
        return true;
    }

    public virtual void Accept(Task task)
    {
        task.Do();
    }
}

class Foreman : Worker
{
    IEnumerable<Worker> workers;
    Func<Task, bool> next;

    public override bool CanDo(Task task)
    {
        // Can do things only on time or otherwise the house 
        // won't stand long
        return next(task);
    }  

    public override void Accept(Task task)
    {
        // Foremen looks for a worker who can do the task
        workers.Where(w => w.CanDo(task)).First().Accept(task);
    }
}

// A man who tries to manage his new house construction
class Man
{
    IEnumerable<Task> construction;
    Foreman foremen;

    public void BuildHouse()
    {
        IEnumerable<Task> tasks;
        // As far as we don't know what to do next we'll ask foreman
        while ((tasks = construction.Where(t => foremen.CanDo(t) && !t.Completed)).Any())
        {
            foreach (var task in tasks)
            {
                foremen.Accept(task);
            }
        }
    }
}

A man must let foreman to manage tasks or become a foreman himself (we’ll take first approach or otherwise the house won't stand long).

 class Foreman
{
    IEnumerable<Worker> workers;
    Func<Task, bool> next;
    
    public void Manage(IEnumerable<Task> construction)
    {
        IEnumerable<Task> tasks;
        // Foreman selects tasks that are not completed and must be done next
        while((tasks = construction.Where(t => !t.Completed && next(t))).Any())
        {
            foreach (var task in tasks)
            {
                workers.Where(w => w.CanDo(task)).First().Accept(task);
            }
        }
    }
}

class Man
{
    IEnumerable<Task> construction;
    Foreman foremen;

    public void BuildHouse()
    {
        // Foreman takes the whole construction to manage
        foremen.Manage(construction);
        // Just check that everything is done
        if (construction.Where(t => !t.Completed).Any())
        {
            throw new InvalidOperationException("Something isn't done!");
        }
    }
}

Now abstractions are leveled - man allows construction to be managed by foreman (foreman abstraction is now consistent).

What it decides?

It is quite uncomfortable when someone asks you for your opinion and then makes his own mind and tells you what and how to do. Why don’t tell what to do in the first place and let me decide how to do it. Isn’t that looking like you are doing my job? This is what Tell Don’t Ask Principle is about:

As the caller, you should not make decisions based on the called object’s state and then change the object’s state.

Here is couple of teenager definitions – which do you think makes more sense?

 class Teenager
{
    private int age;
    private List<string> clothes;

    // Is it age that drives your clothes preferences?
    public int Age { get { return age; } }

    // Do you really wan't your parents to dress you 
    // just based on your age?
    public void TakeNew(string clothing)
    {
        clothes.Add(clothing);
    }
}

// ... or

class Teenager
{
    private List<string> clothes;
    // Preference is something personal
    private Func<string, bool> preference;

    // And you want to able to select clothes based on your 
    // preferences?
    public bool SelectNew(IEnumerable<string> shop)
    {
        var clothing = shop.Where(preference).FirstOrDefault();
        if (clothing != null)
        {
            // Got something you like
            clothes.Add(clothing);
        }
        // Tell your parents whether you need to go to other shop =)
        return clothing != null;
    }
}

By violating Tell Don’t Ask principle caller gains responsibility on make decision instead of the called one. Busted!

What services it performs?

Service provider can do the job by itself or ask collaborators for help. Keeping dependencies on collaborators explicit facilitate controlled class growth. Classes should not contain more objects than a developer can fit in his or her short-term memory (5-7). Otherwise it is possible to introduce responsibilities into class which are either duplicated or not related. Whenever number of collaborators growth more than 5-7 you should consider whether all collaborators still help you to form single abstraction or a new one was introduced.

On the other hand it makes sense to look at consumers and in particular on how they consume supplied services. If consumers use different subsets of provided services it may be a sign that class captures more than one responsibility.

Recall the student example. Original class has two consumers: employer and university (for example). Both consumers were interested in a subset of provided methods (working and studying related respectively). And as we discovered the class captured two abstractions: employee and student.

Summary:

DO pay careful attention to responsibilities assignment
CONSIDER decomposing class into areas (what it knows, maintains, does and decides) and performing assessment in each area

Partilhar via

Single Responsibility Principle – discovering violations

Recursos adicionais