Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

For reference - http://en.wikipedia.org/wiki/Single_responsibility_principle

I have a test scenario where in one module of application is responsible for creating ledger entries. There are three basic tasks which could be carried out -

  • View existing ledger entries in table format.
  • Create new ledger entry using create button.
  • Click on a ledger entry in the table (mentioned in first pointer) and view its details in next page. You could nullify a ledger entry in this page.

(There are couple more operation/validations in each page but fore sake of brevity I will limit it to these)

So I decided to create three different classes -

  • LedgerLandingPage
  • CreateNewLedgerEntryPage
  • ViewLedgerEntryPage

These classes offer the services which could be carried out in those pages and Selenium tests use these classes to bring application to a state where I could make certain assertion.

When I was having it reviewed with on of my colleague then he was over whelmed and asked me to make one single class for all. Though I yet feel my design is much clean I am doubtful if I am overusing Single Responsibility principle

share|improve this question
2  
IMHO there is no point trying to answer your question in general, without knowing two things: How big are these classes (by method count and LOC)? And how much bigger / more complex they are expected to grow in the foreseeable future? –  Péter Török Nov 30 '11 at 8:35
2  
10 methods each is IMHO a clear indication not to put the code into one class with 30 methods. –  Doc Brown Nov 30 '11 at 9:25
6  
This is borderline territory: a class of 100 LOC and 10 methods is not too small, and one of 300 LOC is not too big. However, 30 methods in a single class sound too much to me. Overall, I tend to agree with @Doc in that it is less risk to have many tiny classes than to have a single overweight class. Especially taking into account that classes naturally tend to gain weight over time... –  Péter Török Nov 30 '11 at 9:28
1  
@PéterTörök - I agree unless the code could be refactored into one class that can be used intuitively that reuses code instead of duplicating functionality like I expect that the OP has. –  Chad Nov 30 '11 at 15:42
1  
Perhaps applying MVC would clear this all up: three views, one model (Ledger), and probably three controllers. –  kevin cline Dec 2 '11 at 0:29
show 3 more comments

8 Answers

up vote 17 down vote accepted

Citing @YannisRizos here:

That's an instinctual approach and it's probably correct, as what's more probable to change is the business logic of managing ledgers. If that happens, it'd be a lot easier to maintain one class than three.

I disagree, at least now, after about 30 years of programming experience. Smaller classes IMHO are better to maintain in almost every case I can think of in cases like this one. The more functions you put into one class, the less structure you will have, and the quality of your code degrades - each day a little bit. When I understood Tarun correctly, each of these 3 operations

LedgerLandingPage

CreateNewLedgerEntryPage

ViewLedgerEntryPage

is a use case, each of these classes consist of more than one function to perform the related task, and each one can be developed and tested separately. So creating classes for each of them groups things together which belong together, making it much easier to identify the part of the code to be changed if something is to change.

If you have common functionalities between those 3 classes, they belong either in a common base class, the Ledger class itself (I assume you have one, at least, for the CRUD operations) or in a separate helper class.

If you need more arguments for creating lots of smaller classes, I recommend to have a look into Robert Martin's book "Clean Code".

share|improve this answer
    
that's the whole reason I came up with three diff classes instead of pushing everything in one class. Since there is no common functionality among any of these I don't have any common class. Thank for link. –  Tarun Nov 30 '11 at 7:29
1  
"Smaller classes are better to maintain in almost every case I can think of." I don't think even Clean Code would go that far. Would you really argue that, in an MVC pattern, there should be one controller per page (or use-case, as you put it), for example? In Refactoring, Fowler has two code-smells together: one referring to having many reasons to change a class (SRP), and one referring to having to edit many classes for one change. –  pdr Nov 30 '11 at 8:56
    
@pdr, the OP states there is no common functionality between these classes, so (for me) it is hard to imagine a situation when you need to touch more than one to make a single change. –  Péter Török Nov 30 '11 at 9:22
    
@pdr: If you have too many classes to edit for one change, it is mostly a sign you did not refactor common functionality out to a separate place. But in the above example, this would probably lead to a fourth class, and not just one. –  Doc Brown Nov 30 '11 at 9:23
2  
@pdr: BTW, did you actually read "Clean Code"? Bob Martin goes very far in breaking down code into smaller units. –  Doc Brown Nov 30 '11 at 9:29
show 9 more comments

There are three reasons to invoke SRP as a reason to split a class:

  • so that future changes in requirements can leave some classes unchanged
  • so that a bug can be isolated more quickly
  • to make the class smaller

There are three reasons to refuse to split a class:

  • so that future changes in requirements won't cause you to make the the same change in multiple classes
  • so that bug-finding won't involve tracing long paths of indirection
  • to resist encapsulation-breaking techniques (properties, friends, whatever) that expose information or methods to everyone when they are only needed by one related class

When I look at your case and imagine changes, some of them will cause changes to multiple classes (eg add a field to ledger, you'll need to change all three) but many will not - for example adding sorting to the list of ledgers or changing the validation rules when adding a new ledger entry. Your colleague's reaction is probably because it's hard to know where to look for a bug. If something looks wrong on the list of ledgers, is it because it was added wrongly, or there's something wrong with the list? If there's a discrepancy between the summary and the details, which is wrong?

It's also possible your colleague thinks that the changes in requirements that mean you'll have to change all three classes are far more likely than that changes where you'd get away with changing only one. That could be a really useful conversation to have.

share|improve this answer
    
good to see a different perspective –  Tarun Dec 13 '11 at 18:49
add comment

IMHO you shouldn't be using classes at all. There are no data abstractions here. The ledgers are a concrete data type. The methods for manipulating and displaying them are functions and procedures. There will certainly be many commonly used functions to be shared such as formatting a date. There is little place for any data to be abstracted and hidden in a class in the display and selection routines.

There is some case to hide state in a class for ledger editing.

Whether or not you're abusing the SRP, you're abusing something more fundamental: you're overusing abstraction. It's a typical problem, engendered by the mythical notion OO is a sane development paradigm.

Programming is 90% concrete: use of data abstraction should be rare and carefully engineered. Functional abstraction, on the other hand should be more common, since language details and choice of algorithms need to be separated from the semantics of the operation.

share|improve this answer
add comment

Let's examine these classes:

  • LedgerLandingPage: The role of this class is to show a list of ledgers. As the application grows, you will probably want to add methods for sorting, filtering, highliting and transparently fuse in data from other data sources.

  • ViewLedgerEntryPage: This page shows the ledger in detail. Seems pretty straight forward. As long as the data is straight forward. You can nullify the ledger here. You could also want to correct it. Or comment it, or attach a file, receipt or external booking number or whatever. And once you allow editing, you definitely want to show a history.

  • CreateLedgerEntryPage: If the ViewLedgerEntryPage has full editing capability, the responsibility of this class can possibly be fulfilled by editing a "blank entry", which may or may not make sense.

Theses examples may seem a little far fetched, but the point is, that from a UX we're talking features here. A single feature is definitely a a distinct, single responsibility. Because the requirements to that feature can change and the requirements of two different features can change into two opposing directions, and then you wish you had stuck with the SRP from the beginning.
But conversely, you can always slap a facade onto three classes, if having a single interface is more convenient for whatever reason you can think of.


There is such a thing as SRP overuse: If you require a dozen classes to read the contents of a file, it's rather safe to assume you're doing just that.

If you look at the SRP from the other side, it actually says, that a single responsibility should be taken care of by a single class. Please note however, that responsibilities exist only within abstraction levels. So it makes perfectly sense for a class from a higher abstraction level to carry out its responsibility by delegating work to lower level component, which is best achieved through dependency inversion.

share|improve this answer
    
I guess even I could not have described the responsibility of these classes better than you did. –  Tarun Dec 1 '11 at 3:49
add comment

Do those classes have only one reason to change?

If you do not know it (yet), then you might have stepped into the speculative design / over engineering trap (too many classes, too complex design patterns applied). You have not satisfied YAGNI. In early stages of the software life cycle (some) requirements might be not clear. Thus the direction of change is currently not visible for you. If you realize that, keep it in one or a minimal amount of classes. However this comes with a liability: As soon as the requirements and the direction of change are clearer you need to rethink the current design and make a refactoring to satisfy the reason of change.

If you already know that there are three different reasons for change and that they map to those three classes, then you just applied the right dose of SRP. Again this comes with some liability: If you are wrong or future requirements change unexpectedly, you need to rethink the current design and make a refactoring to satisfy the reason of change.

The whole point is:

  • Keep an eye on the drivers for change.
  • Chose a flexible design, which can be refactored.
  • Be ready to refactor code.

If you have this mindset, you will shape the code without much fear of speculative design / over engineering.

However there is always the factor time: For many changes you will not have the time you need. Refactoring costs time and effort. I often encountered situations where I knew I had to change the design, but due to time constraints I had to postpone it. This will happen and cannot be avoided. I found that it is easier to refactor under engineered code than over engineered code. YAGNI gives me a good guideline: If in doubt, keep the amount of classes and the application of design patterns to a minimum.

share|improve this answer
add comment

I think that if ledger was a table in db, I suggest put these thress task to one class(e.g DAO).If the ledger has more logic and was not a table in db, I suggest we should create more class to do these tasks(may be two or thress class) and provide a facade class to do simply for customer.

share|improve this answer
    
well, ledger is a feature in UI and there are many related operations to it which could be carried out from different pages. –  Tarun Dec 1 '11 at 5:18
add comment

There isn't a definitive implementation of the Single Responsibility Principle, or any other principle. You should decide based on what's more probable to change.

As @Karpie writes in an earlier answer:

You only need one class, and the single responsibility of that class is to manage ledgers.

That's an instinctual approach and it's probably correct, as what's more probable to change is the business logic of managing ledgers. If that happens, it'd be a lot easier to maintain one class than three.

But that should be examined and decided per case. You shouldn't really care for concerns with minuscule possibility of change and concentrate applying the principle on what's more likely to change. If the logic of managing ledgers is a multilayer process and layers are prone to change independently or are concerns that should be separate in principle (logic and presentation), then you should use separate classes. It's a game of probability.

A similar question on the subject of overusing design principles, that I think you'd find interesting: Design patterns: when to use and when to stop doing everything using patterns

share|improve this answer
1  
SRP, to my understanding, is not really a design pattern. –  Doc Brown Nov 30 '11 at 7:39
    
@DocBrown Of course not a design pattern, but the discussion on the question is extremely relevant... Good catch though, I've updated the answer –  Yannis Rizos Nov 30 '11 at 7:41
add comment

You only need one class, and the single responsibility of that class is to manage ledgers.

share|improve this answer
2  
To me a '...manager' class usually shows that you couldn't be bothered to break it down further. What is the class managing? The presentation, the persistence? –  sebastiangeiger Nov 30 '11 at 6:43
    
-1. Putting 3 or more use cases into 1 class is exactly the thing what the SRP is trying to avoid - and for good reasons. –  Doc Brown Nov 30 '11 at 7:30
    
@sebastiangeiger So what are the OP's three classes doing? Presentation or persistence? –  sevenseacat Nov 30 '11 at 7:36
    
@Karpie I am honestly hoping presentation. Granted, the example is not really good, but I would expect that '...Page' in a website is doing something similar to a view. –  sebastiangeiger Nov 30 '11 at 7:40
2  
In fact, you only need one class for the whole application, with the single responsibility of making users happy ;-) –  Péter Török Nov 30 '11 at 9:34
show 1 more comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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