Tag Archives: Design

TDD: your insurance policy

Every now and again I read a post or comment around the topic of TDD and how it constitutes a somewhat futile effort. Forgetting for a moment TDD, let me say a few words around unit testing.

I’m not going to argue or layout the benefits of why putting in place an automated test system for your code is a good idea. I think we are well beyond that discussion and most of us accept it. As such, let us assume that we have unit tests.

Test First or Test After

In order to get these unit tests in place, we have to write them. Here we have two different approaches: test first or test after. In other words, we can either write code and then test or write a test and then implement the code.

With the first approach, that is, writing tests after writing the code, we not only face the challenge of writing a test for something that we perceive as ‘working’, but we also run the risk of creating code that is somewhat hard to test. As the project progresses and the deadlines approach, we tend to drop or ‘defer’ some tests and start to rush more into getting code out the door. After all, what is a perfectly tested codebase if it is not shipped?

What potentially ends up happening, which I’ve seen far too many times, is that we never get round to implementing the missing tests, much like we do not normally go back and pay for technical debt. And of course, this means that our code is at a higher risk of malfunctioning and ultimately costing us money.

If we take a test first approach, we are forced to write a test prior to writing the code. As such we have the test in place. We don’t need to come back later after writing the code to write the test. In addition, since we write the tests first, we can immediately solve any issues that lead to untestable code, after all, we’re writing the code we want to test, so why write it in a way that is not testable if we know beforehand? It’s not like we have written all the code, then come to test it and realized, oops! Yet this is something that does occur in test after approaches and often requires even more effort and thus easier to ‘justify’ skipping a test or two.

Taking the TDD route, we have less chances of ending up with code that is not tested. We have less risks of leaving a test for later, a moment that never arrives.

You could say that it’s a matter of discipline, that just like we commit to TDD and always writing tests first, we can also commit to writing tests after. And I agree, we can. But reality shows that we don’t. Psychologically, there is less chances of us writing a test for something that is ‘working’ than for something that is not.

TDD as an insurance you took out

Now the reality is that TDD isn’t actually about testing as much as it is about design. It is not about test first versus test after. It is about trying to better understand a system, about trying to break things down into pieces our mind can cope with (divide and conquer). It is about specifications. And while I dislike using the word side-effect, the unit tests that emerge from TDD not only guarantee the correct implementation (different to understanding correctly the specification) but also, and more importantly, serve as a regression test. You could therefore think of a unit test in TDD as having three different life forms.

This regression testing is where the analogy to insurance policies comes in, cause even f you don’t believe in the benefits of TDD as a whole, at least consider that it is like that insurance policy which you did take out on the first day, as opposed to the one you kept delaying until it was too late.

Dealing with the “Too many dependencies” problem

17 Replies

Earlier I asked whether people thought the issue of passing in too many dependencies into a class was an actual problem. This was in response to another post I did about test smells. A few people had asked me how the problem of a controller having way too many dependencies should be solved. My response to them was to apply Single Responsibility Principle, which is pretty much what others also said.

The problem

Here is the problem:

In this case it is a controller, but it can well be any other type of class.

Dependency Injection does not solve tight coupling

Why is this a problem? After all, we are applying Dependency Injection.

We are, but we are not decoupling classes. All we are doing is allowing a specific implementation to be swapped out easily. Whether we’re doing it because we want to change behaviors in tests or at runtime, all we’re doing is avoiding depending on a specific implementation.

What we are not doing is automagically making our code clean and sustainable.

The more dependencies our class takes in, the higher its efferent coupling to other classes, meaning that it knows a little bit too much. If it knows too much, it is most likely because it is also doing too much. The class is no longer focused on one thing, and as a consequence we are creating somewhat brittle software that is too tightly coupled, even if we are passing in our dependencies, even if we are not using new anymore.

How did we get here?

We all agree that DI is good and so is Composition over Inheritance. And this causes us to end up with classes that act as coordinators or require information or perform actions on these different classes. Other times we end up here because of:

Framework impositions

Often, frameworks impose restrictions that take us down the wrong path. Such is the case of ASP.NET MVC when using the default routing conventions. We mostly encapsulate functionality under a specific controller because the routing suites us. In other words, from a user’s perspective it makes sense to have many things under the /checkout/{action} path, even if that leads us to bloated controllers that take many dependencies which have little to do with each other.

Badly named classes

XYZService or XYZManager named classes are yet another example of ending up with too many dependencies. When we start to encapsulate everything under a service or manager, we find that despite some of the dependencies only being used by one or two methods, still have to be passed in.

IoC Containers

As developers we no longer worry about wiring up all the different required dependencies as Containers now do this for us. This allows us to inject and inject and inject.

Helping solve the problem

First and foremost, and despite it being beaten to death, we need to apply Single Responsibility Principle. I’ve written in the past techniques that have often helped me accomplish this. Taking this into account, there’s also a series of Do’s and Don’ts that can help:

What you can do

Work around framework limitations. For instance, in the case of ASP.NET MVC, you can override the routing behavior to not tie you down to using one class just because you need to keep the same URL. This can be done in various ways. Google Controllerless Actions for some examples (just a starting point).
Create small focused classes. Why create a CustomerServices that takes in five dependencies as opposed to creating MakeCustomerPreferredCommand and InvalidateCustomerCommand class that takes one dependency and has a single method to do what is required?
Extract infrastructure. Don’t pass in dependencies that have to do with infrastructure, specially if these are repetitive. Use techniques such as AOP to apply these at the level where they belong.
Think about Patterns. Often patterns can help solve the problem of too many dependencies. For instance, many times, what’s deemed as a large collaborative class can instead be turned into a series of classes using the Chain of Responsibility pattern.
Events. Event driven architecture is a great way to provide higher decoupling and independence between objects.
CQRS. Well I just had to mention this one because it’s fashionable. In all seriousness though, this also allows for lower of dependencies since it separates responsibilities into smaller commands and separate queries, which again is our goal.

What you shouldn’t do

You shouldn’t switch to using Service Locator as opposed to Dependency Injection. All you’re doing is hiding the problem. Dependency Injection is actually beneficial in that it is showing us that we are breaking SRP and have a high coupling.
You shouldn’t stop using an IoC Container. There are other ways to prevent the Inject Happy Developer. Testing your code is one of them. As you write tests, you’ll suffer the pain of having to set things up over and over again (and refrain from encapsulating that).

These are just some simple guidelines. They are not set in stone.

Injection Happy Detector

Finally, while writing the initial post, I came up with an idea of writing a ReSharper plugin that could help detect classes that have too many dependencies. The result is
the InjectionHappy Detector. Once installed, it gives you a warning if it finds a constructor taking in too many interfaces. This number by default is 3 (just some random value, you can change it yourself under Options). It’s a very simple plugin and not full-proof. More of an experiment. You’re free to fork it and enhance it anyway you wish.

Too many dependencies?

16 Replies

Over the weekend I blogged about a recurrent issue I encounter when looking at tests, which is the encapsulation of the Arrange part of the Arrange-Act-Assert pattern in either setup methods or base classes. Although there are justified use-cases for it, the majority of times it’s a sign of a code smell.

The example I put was one of a Controller having too many dependencies, and I was asked a few times how to avoid this problem. The purpose of the blog post was not around this issue itself but around test setups. However, it is a valid question, and one that has been raised over the past months questioning the usage of IoC Containers.

So I wonder, do you think this is a problem? If so, how many dependencies is too many? How would you avoid it?

Test setups and design smells

7 Replies

Reviewing some code today, I keep stumbling across private methods that are encapsulating test setups. A probable reason for this is that the authors want to prevent having to repeat the same code over and over again. It seems like common sense. I mean after all, that’s one way of complying with DRY.

The problem however is that often this kind of encapsulation is hiding an underlying design problem.

As an example, I’m going to use one of my all time favorite code-bases that I reference in talks I give on MVC and design in general:

Look at the Authenticated_Shipping_With_EmtpyCart test. It’s short and straightforward. However, notice how the test class has a TestBase:

I chopped off the remaining part as it wouldn’t fit on the screen. You can see the entire class here. It’s just wrong on so many levels.

If you look at the controller this is testing (CheckoutController), you’ll see:

If I were to write that out every time I would need to test the controller, alarm signals would raise that I’m doing something wrong. If I’m hiding it away in a base class or a method, it’s harder to notice.

So next time you feel the urge to encapsulate some setup in a method or base class, ask yourself why. If you don’t come up with a good reason, re-think your design.

Side-note: Some people have lately been complaining that IoC Containers lead to these kind of design smells also, allowing developers to by injection-happy when dealing with dependencies. It might be true, although I believe that if you are correctly writing the necessary tests, you’d notice this problem early on. The code above also uses a container in the tests (and doesn’t even use conventions for setting it up). That’s another reason I’m against using containers in unit tests.

With HTTP, your application is your API

15 Replies

As of ASP.NET MVC 4.0, the once-named WCF WebAPI project is now part of MVC. To provide a quick summary for those not familiar with either of the two:

ASP.NET MVC is a framework that allows you to create applications based on HTTP.
WebAPI is a framework that allows you to create applications based on HTTP.

So now, under one roof (the roof being ASP.NET MVC not ASP.NET which would have made a bit more sense) you can use two distinct technologies to create HTTP-based applications. Note that I’m talking about HTTP, not necessarily what one would call RESTful systems, but plain HTTP.

Two choices, what to do?

Which one to choose from though? Well, the problem with that question (or maybe a somewhat answer) is that neither of the two technologies is complete. Each have their own missing features and benefits.

With ASP.NET MVC, out of the box, you don’t get things like Content Negotiation, Support for Conditional GETs or PUTs, Complete HTTP Response codes, HTTP Authentication, among other things. And in case you’re wanting to take a resource approach, well you’re out of luck there too since MVC is focused around operations not resources. You do get nice support for View Engines though.

On the other hand, with WebAPI, you do get some of the previous things, mainly Content Negotiation and a more Resource orientated approach (or so it has been said). You also get self-hosting capabilities and are more “exposed” to the raw HTTP. But, you don’t get a View Engine. What this means is that you can’t easily throw together some template in Razor and have it display to in a browser as a user interface.

The Microsoft Guidance

The general guidance coming out of Microsoft, be it from blog posts, tutorials or generally what members of the ASP.NET team are recommending, is to use the best of both worlds. That is, use MVC to create your user interface and use WebAPI to create your API. You can have a CustomerController to handle the HTML views and interaction with real humans, and have an CustomerAPIController (or whatever is may be called) to deal with non-human software interacting with your system. There are wizards in the product to create the different types of controllers and the main debate going on seems to focus more around what folder each type of controller should reside in. It is also mostly being defended under the Separation of Concerns Act.

To be clear, it is recommended to:

Create your user interface with MVC
Create your API with WebAPI

There is no division

When you’re creating an application based on HTTP, your application protocol, that is the language that is used to communicate between your application and the consumer of your application is HTTP. HTTP is not a mere conduit to transfer information. It is a fully fledged application protocol. It has operations, it has request headers, it has responses and response code. It knows how to deal with stale data, it knows how to deal with authentication. It knows what resources are and how to represent these in different way. It is your API.

If you take this into account, your User Interface, is nothing more than another consumer of your API. When you request to see a Customer, if you are a human being, you will be presented with an HTML page to see the customer record. When you are a machine you can request a JSON or XML variant of that customer. However, these are merely two different representations of the same thing. When you update a customer, be it via a user inputing data into a form, or a machine submitting a POST request, there should be no distinction in the behavior of your application. Why would there by? And if there isn’t, why add an extra overhead of separating them out? It makes no sense whatsoever to try and artificially separate these two concepts out into UI and an API. The only UI thing you need to do on the server is forms and HTML pages. Not separate controllers.
Well there are two potential reasons you might want to do this:

Political, to somehow justify having two ways to implement the same thing
Technical. Shortcomings of one technology prohibiting you from doing it

But neither of those stand. The political reason here should be irrelevant to you as a consumer of the ASP.NET MVC stack. More importantly however, the technical reason should not distort the reality.

So what should you do?

Personally I am against drawing this artificial non-existing line between the two concepts merely to justify technical shortcomings. The solution I would take is to have one API and use one of the two technologies, or if you’re open to alternatives, use those (OpenRasta, Nancy, FubuMVC), whatever, I don’t care which one you use.

I have been using MVC for some years and the missing pieces such as content negotiation, HTTP authentication, ETag support (conditional requests), are nothing but a bunch of filters that are implemented with a few lines of code. Moving from operations to resources is nothing but a few routing techniques, implemented using some conventions (if you’re interested I have a lot of code I can share).

Whatever you do use though, don’t let its shortcoming dictate your design. You are implementing HTTP systems, embrace HTTP.

Errors: Handle with care

2 Replies

Went to renew my driving license this morning, a process the DGT (Traffic Authority in Spain) has made extremely simple now: you just head over to a private clinic, pay quite a bit more than you actually should and have them deal with the bureaucracy.

It was meant to be a 30 minute process and quite straightforward:

1. Ask for customer’s details
2. Run some medical tests
3. Get a temporary driving authorization until your license arrives in the post

I ended up being there about an hour and not getting it renewed.

The real process went something like this:

1. Was asked for all my details at reception and waited as they were typed in.

2. Taken into examination room. Waited 15 minutes until they could start the computer since the fan was acting up. Thankfully the technician (which was actually a Doctor) came in, kicked the box (literally) and it turned on. As he walked out he said:

“these computers are scared of me…”

3. Asked for my details again. When I questioned why I was being prompted for them a second time, the answer was somewhat expected:

“This computer has nothing to do with the other one. This one runs the DGT program. The other one runs ours. “

Stupid me!

4. Went through some appropriate tests. Again, this was delayed because the second computer failed to boot.

1st Error handling mistake – the human

I waited patiently until my details were submitted to the DGT system. Everything seemed fine until I was suddenly asked:

Operator: “Are you sure you haven’t lost any points on your driving license?”

Me: “Em no. In fact I think I now have 14 as opposed to 12”.

Operator: “Hmm, something is very wrong. It’s giving me Undetermined Error 611”

Me: “OK, so what does that mean?”

Operator: “There’s a problem with your driving license. I think….”

Me: “You think or you’re sure?”

Operator: “I can’t know exactly. As it stands I can’t process your renewal. You’ll have to leave this with me. If I don’t call you back today then you’ll need to go to DGT and do it yourself. But don’t worry, we won’t charge you anything”

Now you have to admit, that’s awfully kind of them not to charge me for something they haven’t managed to do.

2nd error handling mistake – the stupid developer

Up to now, my customer experience, as a user of this clinic has been somewhat lower than par one could say. I’ve witnessed operator incompetency, lack of care of technical equipment and overall no inspiration of confidence.

This clinic has a ER unit, in-house care, pediatricians and various other specialties. Suffice to say, it does not inspire any confidence in me for me to ever return there, specially when dealing with something serious as health issues. They’ve lost a customer.

The second user experience however comes from the developers that created the system. It was that damn undetermined error 611.

As a developer, when you see “Internal Error”, “Undetermined Error” or “Catastrophic Failure” you pretty much know what the developer was thinking at the time:

“I don’t give a shit about controlling errors in this scenario. Too many things can go wrong for me to control”

Now I am right in assuming that this error code is only valid for the developer. Otherwise he/she is really stupid to display an error message to the user making them either have to memorize error codes or look them up in some manual.

As such, that leaves one of two options: 611 is just the number they use to define what an undetermined error is (i.e. 500 Internal Server Error) or 611 provides them some valuable insight into the error, which then begs the question of why it’s undetermined.

Whatever the case though, this is yet another UX fail. Not only does it not provide any kind of valuable information to the user, but it also, as is in my case, when faced with a somewhat incompetent user, causes a certain level of concern (WTF is wrong with my license? Has it been revoked?).

Handle with care

Time and time again I’ve seen applications fail when it comes to handling errors. Most common excuses given when observing bad error management are, that it’s too complicated to know what’s gone wrong. The irony of this is that much of this complexity is caused by the way we design the system, as opposed to the underlying business rules. The only time error handling is too complex to deal with is when a human decision is required. In these scenarios, we need to also deal with the error, but using a different approach.

Providing error codes or ambiguous messages to a user should never be done. Anything that interacts with a user, a consumer of your application, should use language the user understands, not something they need to look up. That’s what computers are good for: looking up information.

Now, while writing this post, I got a call from the clinic. The undetermined error ended up being a user error. The operator had asked for my nationality and I had said Spanish. She had entered Spain as my place of birth. DGT had Iran on their records. There was a mismatch. The operator failed in using the system. But the system failed her even more by not handling errors correctly.

Much of this would have not taken place if instead of

“Undetermined error 611”

it would have displayed

“There is a mismatch between the place of birth entered and that on record”

Something as simple as this would have saved me and the clinic time, annoyance and money.

So never supply error codes?

When switching to the context of developers, it is fine to provide log details and stack traces. In this context you can provide codes. HTTP does it. It’s useful to automate error handling and machine-to-machine interaction. But in addition to these codes, you still need to provide a valuable message so that a developer can understand where to look, what’s gone wrong.

And if everything does go wrong and there’s no way to handle the situation, consider providing a more re-assuring message to the user, something that doesn’t leave him thinking he’s lost all his points on his driving license:

“It’s OK. You didn’t screw up. We did. We’ll sort it out and get back to you”

That dreaded M in ASP.NET MVC

6 Replies

When it comes to working with Models in MVC, I’ve tried many approaches, some good, others not so much. I’ve ended up settling on ViewModels, whereby the Model I submit is dictated by the View I’m working with. This allows me the flexibility of displaying or gathering only the information I need. It also allows me to provide additional information on the view that isn’t necessarily required by my domain.

It works, but it also adds a lot of friction. Be it mapping, be it validation, it’s continuously repeating same processes over and over again. Even automating some of this still requires constant set up. Every time I work with ASP.NET MVC, I dread having to deal with all this. Way too much friction.

I’ve often wondered whether I’m overly complicating myself by trying to add so much flexibility. I mean if the Rails guys can work with ActiveRecord, why can’t I? Granted that maybe much of the drawbacks of ActiveRecord can be remedied in some way in Rails because of Ruby being a dynamic language and allowing for things such as Mixins, but still, what about the other stuff? The mapping to an actual domain model. What happens when they need a list of countries? What happens when they have to update only 2 out of 7 fields of their domain model?

I decided to ask Shay how he works. As a guy who works with both ASP.NET MVC and Rails, and has written a book on IronRuby, I thought he’d be a good candidate. Plus, he’s a nice guy.

I have to say I wasn’t really surprised by his answer. He binds directly to his Domain Model in ASP.NET MVC. I asked how he dealt with additional info: Html Helpers. I asked how he solved partial updates: In the controller. Although, not surprising, it was interesting. As a guy that’s worked heavily in Rails, he seems to cope fine with this approach in C#.

So again I wonder, am I focusing on too much flexibility? I decided to ask others by running a quick survey on Twitter asking how people worked with M in MVC.

1. First the disqualifying question. Are you using Strongly-Typed views?

98% voted they are. I’m guessing more than 2 people were not, but since the Survey was focused on strongly-typed views, they probably didn’t take part.

2. Now the question to see if I am the odd one out using ViewModels

Apparently not. 78% of people use ViewModels.

3. The next question was how one deals with only partially updating some fields of a Domain Model if binding directly to them.

4. What about that damn list of countries?

(I’ve actually found another way to solve this problem, partially based on conventions, and we might even build it in to AutoReST. But that’s for another post).

5. Key question the following. Mappings (read friction). If you use ViewModels, do you manually map information or use something like AutoMapper?

Surprised by the number of people doing manual mapping. You would think that if AutoMapper’s only responsibility is to do mapping for you, why not use it? Could it be again the same issue? Too much friction to setup? NuGet to the rescue? Too much ceremony?

6. Here’s another one. Validation.

So pretty much everyone (79%), some way or other has to deal with decorating their models with attributes.

7. Finally, I was curious how people felt in general with the development process. Did they also encounter friction?

Not surprisingly, 70% find some level of friction in doing web development.

Conclusions

First off, this is not a stab at ASP.NET MVC. If you take it like that, you’re barking up the wrong tree. I’m sure in one way or another, any platform or language has a certain level of friction when it comes to developing applications. No, this is more of a self-stabbing.

I also agree that on the whole, there’s too much friction. However, I’m not sure how much of that friction is caused by the platform vs the mentality of us developers to think big and try to build in so much unneeded flexibility. Are we really applying YAGNI? Are we really applying KISS? Maybe adding so much flexibility in terms of ViewModels because, and I quote, “when dealing with complex scenarios, simple approaches fall apart” only actually solve a 5% edge case that could be remedied in a different way.

Maybe we should stop being afraid of trying to be too strict and not flexible enough. Maybe we should take the concept of conventions more seriously than just what folders our Views, Controllers and Models reside in. Maybe we should push conventions to the limit and see if we actually reduce this friction.

And that’s the next journey I’m going to embark on. It might be time to drop the ViewModel, it might not be. What I do know is that writing good software shouldn’t be so complex.

SRP, as easy as 123…

5 Replies

…of course, you’d need to have the song ABC from the Jackson Five in your head for that title to be remotely amusing.

Single Responsibility Principle is such a simple principle. It states that a class should only have one responsibility. One responsibility. Not two, not three, one. Such a concise and simple definition is hard to get wrong, right? Right? RIGHT??? Hmm…

A conversation between two friends

I’m looking at this Customer class, it does a bit too much no?

Huh? What do you mean? It’s a Customer Class. It deals with Customers. That’s all it does. Not Employees. Customers. I can fetch a customer. I can fetch a lot of customers. I can store a customer. I can calculate their age. I can check if they are VIP customers. Everything there is to do with customers. That’s all it does. Isn’t that what this Single Responsibility Principle states? To only have one responsibility?

Well, yes, but I think you’ve kind of missed the point on what this whole responsibility thing is .

How so? It just deals with Customers right? In fact, isn’t what I’ve just described pretty much the ActiveRecord Pattern?

Indeed it is.

So then WTF is the problem? I don’t get it.

Digging deeper…

Hmm, let’s look at this class. It reads/writes a customer(s) from/to the database. Therefore it has some code that has to do with reading from a database. It calculates age and verifies if a customer is a VIP. So it must have some business logic code in there.

What happens when you need to change the persistence logic?

Well I’ll change the code.

Which code?

The Customer class of course.

Right. What happens when you need to change the business logic of how a customer is considered a VIP?

I’ll change it again…..Sorry. What’s your point?

So again, you’ll touch that class. What happens when Customers need some validation? You’ll again have to touch the same class.

And?

What happens when you touch code? You usually end up breaking things. And it’s not necessarily that particular thing you touch. You break other things that at that point you had no clue were somehow related. The more complicated the code, the more chances you have of breaking something. Of course, if on top of everything, you don’t even have regression tests…well I’m sure you’ve suffered the consequences.

I kind of think of myself a competent developer. I’m sure I can work with enough care to not break things.

Right…. Now while you go and convince yourself of that, let me explain another problem. You see, writing code is easy. Understanding it is hard. The more code in a class, the harder it will be to decipher. The less code, the easier it will be to comprehend. If a class does many things, it will most likely have what? More code. The less it does, you got it! Less code. Forget logic for a moment. It even has psychological impacts. Open a large class and a small one. Which one will depress you more?

I have no problem understanding my own code.

Sure. You’re very smart and you’ve worked on this code base and you know it well. What about other team members? Hell. Forget others. Why don’t you look at code you wrote yourself eight months ago. Do you know what it does?

Well mostly yes. But I guess that’s kind of why comments are useful

Why do you need comments if your code is clear enough to understand? And you know why your code is hard to understand? Because you’re trying to solve too many problems at the same time. Think about it. You’re doing Customer listings, storing, reading and some business logic. It might be the case that for this particular example it’s not too hard to understand, but that’s because both of us have beaten Customer management to death (along with Authentication) so we’re experts in it. But imagine being thrown into some code that you have little knowledge of the domain. Is it easier to understand a class if it does one thing or five?

So what are you proposing?

It’s actually very simple. Divide and Conquer. Remember that? Back from our College days? Divide up a problem into smaller problems and solve the smaller issues. Dealing with mini-problems is easier than dealing with mega ones.

When we take a class and divide it up into smaller classes, it will be easier to deal with it. And dealing can mean both understanding it and modifying it. But if your customer class is doing all that, it’s bound to be harder to understand and maintain. That’s why it’s important to get these responsibilities right.

I see. But where have I gone wrong then? I kind of thought I understood the Single Responsibility Principle. Going back to my Customer class, how do I define what the responsibility is if it’s not “dealing with customers”?

Getting a grip on responsibilities

The main problem in complying with SRP is defining the responsibilities. Where do we draw the line? A few things that have helped me along the way have been…

Change

Have you ever heard people say:

“A class should only have one reason to change”

when talking about Single Responsibility?

Yes, actually I have.

Well think about it. If a class changes for more than one reason, it must be because it’s doing more than one thing. Going back to your customer class, think how many reasons there is for it to change. If we have to touch the same class for changing different things, the risk of breaking something is higher, which leads to higher costs in maintainability.

One way therefore of trying to figure out the responsibilities of a class is by asking the question of how many reasons are there for it to change.

Naming

Naming is a good way to discover the responsibilities of a class. What does your class do? Give it a name describing what it does. Does it use And or Or? Is it hard to explain what it does? Do you need to use suffixes like Manager, Processor or Admin because you can’t pinpoint the exact word describing what it does? Maybe it’s because it does two things that one word can’t describe. These are all clues that your class is doing more than one thing.

Cohesion

Take your class. Look at your methods. Do they have parameters or are they using instance fields? If they are using parameters, remove them. Make them instance fields. Do you end up with methods that only use one of the five instances? That most likely is a warning of the low cohesion that exists between that method and your class. Cohesion is a indication of how well related lines of code are, how well related methods are to a class.

And you use ReSharper right. Next time ReSharper prompts you if you want to make a method static, it’s telling you it does not use instance fields. That’s where you need to decide if that method actually belongs in that class. Sometimes it might, many times it might not.

OK, that’s kind of making sense.

Wait though. Many look at Single Responsibility Principle as something that applies only to classes. Wrong. It applies to methods too. The more things your method does, the more lines of code. The more lines of code. The harder to understand. The harder to debug.

Use the same process to identify responsibilities in methods. How many reasons do I have to change the method? What do I name it? If I can’t name it on a single line, bad! If I can’t name it without using And/Or , bad!

Refactor your methods to smaller ones. Give each smaller method a good name. Make it descriptive. Don’t waste time trying to comment your methods and the 20 parameters it takes. If it’s well named and has one or two parameters at most, it should be self-describing. Remember, parameters are used to operate on values, not decide what path to take in the method. For that, you create two methods.

(Beer enters the scene…)

Hahahhaha…

What?

I remember back when I first started using non-locking source control, I was concerned about running into conflicts when merging. I was right. I did run into issues. I bitched at the SCM. But now that I think of it, the problem wasn’t the SCM. It was the way I was working. It was having too much code in the same class.

Yep, and that would lead to contention since many people would need to touch the same code.

Yep. I was fighting the wrong problem. Damn, it seems such a simple principle, but hard to get right then….

Yes. It’s about asking the right questions. Single Responsibility Principle is about making code maintainable and understandable. That’s all there is to it really.

Right. Well dude, I guess it’s time for me to Refactor out that Class.

Yep. Oh and do yourself a favor. If you’ve not read the book Clean Code by Robert C. Martin (@unclebobmartin), get it. It teaches you much of what we’ve been talking about.

Cool!

var improves readability

The Principle of Least Surprise

6 Replies

I’m having a discussion on the ASP.NET MVC forums with one of the guys from the ASP.NET team in regard to the Data Annotations in MVC 2 and I’m not sure I agree with him. Here’s an issue Jak and I have run into:

In MVC 2 there’s a new Html Helper named EditorForModel(); that renders out a form based on the properties of your model, along with the validation messages, labels, etc. So something like this:

   <% using (Html.BeginForm()) {%>

        <%=Html.EditorForModel() %>

<p>

                <input type="submit" value="Save" />

            </p>

    <% } %>

.csharpcode, .csharpcode pre
{
font-size: small;
color: black;
font-family: consolas, “Courier New”, courier, monospace;
background-color: #ffffff;
/*white-space: pre;*/
}
.csharpcode pre { margin: 0em; }
.csharpcode .rem { color: #008000; }
.csharpcode .kwrd { color: #0000ff; }
.csharpcode .str { color: #006080; }
.csharpcode .op { color: #0000c0; }
.csharpcode .preproc { color: #cc6633; }
.csharpcode .asp { background-color: #ffff00; }
.csharpcode .html { color: #800000; }
.csharpcode .attr { color: #ff0000; }
.csharpcode .alt
{
background-color: #f4f4f4;
width: 100%;
margin: 0em;
}
.csharpcode .lnum { color: #606060; }

you’d get something like this:

If you want to do individual fields and thus have finer control, you can use the EditorFor helper, passing in a property name. In other words, the previous could be also rendered as:

    <% using (Html.BeginForm()) {%>

   <%=Html.EditorFor( model => model.FirstName) %>

   <%=Html.EditorFor( model => model.LastName) %>

   <%=Html.EditorFor( model => model.Email) %>

<p>

                <input type="submit" value="Save" />

            </p>

    <% } %>

but this time you don’t get the labels:

The problem however is that you lose something else: the Validation messages. If you have client-side validation enabled, the previous ASPX file generates a pretty much useless call to the EnableClientValidation JS function:

EnableClientValidation({"Fields":[],"FormId":"form0"}, null);

as opposed to:

EnableClientValidation({"Fields":[{"FieldName":"FirstName","ValidationRules":[{"ErrorMessage":"First name is required","ValidationParameters":....

// rest omitted for brevity

The result of course is that client-validation doesn’t work (and from what I’ve heard causes JS errors in some browsers).

The solution to this is to explicitly add a Validation Message, like so:

<%=Html.ValidationMessage("FirstName")%>

Now I understand that EditorFor is for fine-tuning and there is a corresponding LabelFor (although a ValidationFor doesn’t exist yet), but my main concern here is that it’s breaking the principle of least surprise from an API perspective.

For me, the only difference between EditorFor and EditorForModel should be that in the first I specify a property name explicitly whereas in the latter it just assumes the whole model. Nothing is telling me by the name of the method that the second does a whole bunch more of magic.

One solution is for EditorFor to be renamed to something else if it’s ONLY going to provide the input box (be it a text area, checkbox, radio group, etc..).

Thoughts?

Hadi Hariri's Blog

Tag Archives: Design

TDD: your insurance policy

Test First or Test After

TDD as an insurance you took out

Dealing with the “Too many dependencies” problem

The problem

Dependency Injection does not solve tight coupling

How did we get here?

Framework impositions

Badly named classes

IoC Containers

Helping solve the problem

What you can do

What you shouldn’t do

Injection Happy Detector

Too many dependencies?

Test setups and design smells

With HTTP, your application is your API

Two choices, what to do?

The Microsoft Guidance

There is no division

So what should you do?

Errors: Handle with care

1st Error handling mistake – the human

2nd error handling mistake – the stupid developer

Handle with care

So never supply error codes?

That dreaded M in ASP.NET MVC

Conclusions

SRP, as easy as 123…

A conversation between two friends

Digging deeper…

Getting a grip on responsibilities

var improves readability

The Principle of Least Surprise

Follow “Hadi Hariri's Blog”