I recently put this bullet point list together for the team I’m currently working with.
Naming Conventions
- Naming conventions. We will follow Microsoft’s standard C# naming conventions. http://blogs.msdn.com/brada/articles/361363.aspx. Ignore the bit about putting using statements inside the namespace declaration.
General Principles
- The core imperative is to organise complexity.
- Clarity and readability is central. “Intention Revealing”
- Do not prematurely optimise for performance.
- Do not repeat yourself. Never copy-and-paste code.
- Decouple.
- Always try to leave the code you work on in a better state than before you started (the ‘boy scout’ principle)
Keep the source clean
- Always delete unused code. Including variables and using statements
- Don’t comment out code, delete it. We have source control to manage change.
Naming things
- The name should accurately describe what the thing does.
- Do not use shortenings, only use well understood abbreviations.
- If the name looks awkward, the code is probably awkward.
Namespaces
- Namespaces should match the project name + path inside the project. This is what VS will give you by default.
- Classes that together provide similar functions should be grouped in a single namespace.
- Avoid namespace dependency cycles.
Variables
- Use constants where possible. Avoid magic strings.
- Use readonly where possible
- Avoid many temporary variables.
- Never use a single variable for two different puposes.
- Keep scope as narrow as possible. (declaration close to use)
Methods
- The name should accurately describe what the method does.
- It should only do one thing.
- It should be small (more than 10 lines of code is questionable).
- The number of parameters should be small.
- Public methods should validate all parameters.
- Assert expectations and throw an appropriate error if invalid.
- Avoid deep nesting of loops and conditionals. (Cyclomatic complexity).
Classes
- The name should accurately describe what the class does.
- Classes typically represent data or services, be clear which your class is.
- Design your object oriented schema deliberately.
- A class should be small.
- A class should have one responsibility only.
- A class should have a clear contract.
- A class should be decoupled from its dependencies.
- Favour composition over inheritance.
- Avoid static classes and methods.
- Make the class immutable if possible.
Interfaces
- Rely on interfaces rather than concrete classes wherever possible.
- An interface is a contract for interaction.
- An interface should have a single purpose (ISP)
Tests
- All code should have unit tests if possible.
- Test code should have the same quality as production code.
- Write code test-first wherever possible.
Error Handling
- Only wrap code with a try..catch statement if you are expecting it to throw a specific exception.
- Unexpected errors should only be handled at process boundaries.
- Never ‘bury’ exceptions.
Posts
17 comments:
"Always delete unused code. Including variables and using statements".
In Visual Studio 2008 I use Power Commands- http://code.msdn.microsoft.com/PowerCommands to regularly remove and sort usings across entire solutions.
Mike do you use StyleCop to help?
http://code.msdn.microsoft.com/sourceanalysis
Just had another look at PowerCommands- I've noticed it has "Extract Constant" which will be useful for your standards. I didn't know this functionality existed.
This is a good list of things that every developer should be doing in their code, but my problem with it is that apart from naming/formatting conventions which should be agreed by the team so that it follows a uniform pattern, the rest of the points shouldn't need to be formalised in this way. The developers who this list makes sense to will be doing those things anyway, and the ones who aren't doing those things will need more than a list to understand why they should be doing them. Without understanding the why, they will never do it properly. It is a good starting point though for a discussion about each of those points.
With more functional aspects of programming making their way to the .NET platform, it is important to remember that this list describes elements of object-oriented design.
Some guidelines might be different if solving problems in a functional manner, especially "Avoid static classes and methods".
Great post Mike,
It always amuses me how these principles seem to go out of the window in stored procedures.
The project I'm working on seems to keep most if the business logic in stored procedures. ">10 lines is questionable for a method" - how many sprocs have we seen with >400 lines of practically undebuggable Sql!
What to you intend for:
"Unexpected errors should only be handled at process boundaries."
Richard,
I hadn't seen Power Commands or Style Cop, thanks for letting me know about them. My tools of choice are Resharper for refactoring and NDepends for source analysis. I plan to blog about how we're using NDepends to provide quality metrics for our code base at some point.
Anonymous,
You are right. This list was accompanied by a workshop where we talked about what it means to have 'quality code'. Of course simply obeying the items on this list won't automatically give you quality, there's no replacement for peer review and an understanding of object oriented design, but it's a single tool in an attempt to introduce the idea of code quality to a team that previously had none.
Brian,
Yes, I agree, as functional programming becomes more prevalent we are going to have to learn a whole new way of designing software. However, this list was put together for a team that have quality issues here and now with object oriented software development. The 'avoid static classes and methods' item is there because many developers new to OO will often fall back into a procedural style using statics everywhere, and that's something that I wanted to discourage.
Andy,
I feel your pain :) Stored procedures are a curse.
SavSevic,
"Unexpected errors should only be handled at process boundaries." This is a blog post in itself, but in short, any language that provides structured error handling (such as C#) should should not catch Exception in each method. Catch any specific exceptions you may be expecting, but allow unexpected exceptions to bubble up the stack and catch them at the process boundary. In a web application that means implementing a top level exception handler that can display a freindly page to the user explaining that something has gone wrong, and that writes the details to a log.
I agree wholeheartedly with nearly everything you have in your list except the following:
Avoid static classes and methods.
While I agree we are working in an object oriented paradigm and it makes sense to take advantage of that when possible, I can think of numerous scenarios where a static class is ideal, not the least of which is a common utilities class.
For all intents and purposes, Extension Methods are static methods. They're essentially a utility method that we want to add on to a class when we don't want to write a subclass/inherited class.
Am I missing something here?
More than the half of these points can be done by static analysis tools like fxcop, genderme, ndepend, ncover, simian etc. Why would you waste the time of 2 human beings doing things like that?
I don't agree with "Classes typically represent data or services" when your using an object-oriented language. Object-Oriented code is made out of classes containing both data and behaviour.
This is really bad list. It leads to professional-looking, well-designed, well commented but incorrect code.
Top of the list should make
-checking boundary conditions of various methods
-checking that methods don't break class invariants
-make sure that if statements select correct cases and what is going on in else statement if any.
This is more important things to look than naming convention but it is hard and requires to understand code.
@Anonymous : "This is a really bad list " ....
Sorry, I completely disagree. The things you are talking about are handled by Unit Testing. Good practice should also be checking amount of code coverage of unit tests and ensuring proper unit tests.
A code review would take FOREVER if you were analyzing inputs/outputs for boundary conditions, etc. That's not the point of a code review.
More than 10 lines of code a method is "questionable"?!
I think you're rather misguided here. Please remember that each new method has its own mental overhead and adds lines of code to your project, more documentation, more for people to read.
Real-world business logic often has many cases that really aren't improved by extracting lots of methods that aren't easily described and only used in one place.
And many famous algorithms can't be expressed in less than 10 lines and don't benefit by being broken up.
Your methods should be the right size, doing one clear thing. Short functions are generally better than long functions. But some arbitrary number like "10 lines" is just plain wrong.
NipsMG,
By 'Avoid' I don't mean 'never use'. Of course there's a place for static methods. However it's easy for someone used to a procedural style of programming to fall into the trap of overusing them. I'm probably guilty of overusing extension methods myself. I guess I'm trying to encourage my team to think very hard before writing a static method, and having them defend that thought process in a code review is a way of getting them to do that.
Why? Well if you are interested in doing TDD and component oriented programming (I am), then you are by definition interested in dependency injection and polymorphism. None of these are possible with static methods.
Anonymous 1,
Good point. Tools are an essential help for any review process. I don't think the tools can replace a human code review though.
I agree that OO is all about keeping data and behaviour together, but I would still maintain that most software architectures define more 'data-like' classes (models, DTOs) and more 'service-like' classes (controllers, services, repositories). Understanding the difference makes it easier to understand how those classes should be instantiated and used.
Anonymous 2,
Agreed, those are definately things to look for. However I would also agree with NipsMG that you would expect those kinds of things to be tested for.
Tom,
Of course it's not a hard and fast rule that methods should *never* be more than 10 lines of code. However, in my experience of building business applications, there's rarely a reason to exceed that limit. Usually when I see large methods, it's because they've either got copied and pasted repetition, or they are doing more than one thing. By saying that more than 10 lines is 'questionable', I mean that you should question why your method needs to be large. I disagree that 'real world' business business logic can't be decomposed into smaller logical units. And if your methods can't be 'easily described' then you really do have a serious problem.
2 NipsMG
Problem with unit tests is that they are written by the same guy who wrote code. Meaning that code and tests in sync.
It doesn't imply that code correct because guy may not think about specific case and of course the case didn't make unit test.
This is why code review is a must.
Or you need other guy who doesn't see code to write more tests (or both)
Oh, I meant that if you break up routines too fine then they are often not easily described.
Small is good. However, each extract routine carries a cost. You have to balance the two.
This list sounds like the exact opposite of what a code review should be about. Code reviews are about verifying whether somebody else's solution to a discrete problem solves that problem adequately. And to clarify, since some people seem to have difficulty with the concept, by "adequately" I mean simply this: no input that you can think of produces erroneous output.
If instead your code reviews are merely about imposing your own personal beliefs about how to code on others, or boring them by telling them how you imagine you would have solved the same problem, it becomes a less than useless activity that only serves to stroke the ego of the Reviewer.
If you merely want code to be written exactly as you would have written it yourself, it'd be much more efficient for you just to actually write all the code yourself. It would also be rather less frustrating for those around you that need to work with your ego.
Great post!
Post a Comment