Rules to Better Architecture and Code Review

RuleSummaryIntro

Since 1990, SSW has supported the developer community by publishing all our best practices and rules for everyone to see. If you still need help, visit SSW Consulting Services and book in a consultant.​

Hold on a second! How would you like to view this content?

Just the title!A brief blurb!Gimme everything!

Page Content

​For any project that is critical to the business, it’s important to do ‘Modern Architecture Reviews’. Being an architect is fun, you get to design the system, do ongoing code reviews, and play the bad ass. It is even more fun when using modern cool tools.

Follow these steps to achieve a 'Modern Architecture Review'. See how performing an architecture review on a big project, no longer needs to be daunting. Read about the 1st steps, then check to see if you’re following SOLID principles, then drill in and find patterns and anti-patterns. Use Visual Studio 2012 (aka VS 11) to help the process with architecture tools, code smell tools, and the great Visual Studio 2012 Code Review tools.

These steps enable you to attend to the code that needs the most attention. Finally, create TFS work items to make sure they get fixed in the next sprint.

Often times incorrect process is the main source of problems. Developers should be able to focus on what is important for the project rather than getting stuck on things that cause them to spin their wheels.

Note: Anyway keep this brief since it is out of scope. If this step is problematic, there are likely other things you may need to discuss with the developers about improving their process. For example, are they using Test Driven Development, or are they checking in regularly, but all this and more should be saved for the Team & Process Review.

Level 1 - Old School

This team style does a lot of upfront documentation and planning, is very comfortable with Waterfall, and has rarely even heard of Agile :)

Heavy, long documents

Sequence Diagrams

UML

This is a well-established way to do documentation but the issue with it is that it gets out of date.

Figure: Documentation can take the form of Sequence Diagrams

Figure: ...or Use Case Diagrams

Exception: Keep this limited to just enough documentation to cover a couple of sprints, and be committed to keeping it updated. The tool of choice if you're going down this road is Enterprise Architect (an excellent application built by Australians).

Level 2 - Lots of documentation (and the 6 important documents)

​

This team style are all under 30 and have never heard of FoxPro or Access

6 small docs (a couple of pages max + in the order you would read them):

Documentation\Business.docx - Explaining the business purpose of the app

Documentation\Instructions-Compile.docx - Contains instructions on how​​ to get the project to compile (aka the F5 experience)

Level 5: Lots of documentation (and the exact steps to Get Latest and compile with the *database*)

Level 6 : Less documentation (and Get Latest and compile with a PowerShell script)

A perfect solution would need no static documentation. Perfect code would be so self-explanatory that it did not need comments. The same rule applies with instructions on how to get the solution compiling: the best answer would be for the solution to contain scripts that automate the setup.

Example of Level 6: PowerShell Documentation

Recommendation: All manual workstation setup steps should be scripted with powerShell (as per the below example)

Recommendation: You should be able to get latest and compile within 1 minute. Also, a developer machine should not HAVE to be on the domain (to support external consultants)

PS C:\Code\Northwind> .\Setup-Environment.ps1

Problem: Azure environment variable run state directory is not configured (_CSRUN_STATE_DIRECTORY).

Problem: Azure Storage Service is not running. Launch the development fabric by starting the solution.

WARNING: Abandoning remainder of script due to critical failures.

To try and automatically resolve the problems found, re-run the script with a -Fix flag.

Figure: Good example - you see the problems in the devs environment

PS C:\Code\Northwind> .\Setup-Environment.ps1 -fix

Problem: Azure environment variable run state directory is not configured (_CSRUN_STATE_DIRECTORY).

Fixed: _CSRUN_STATE_DIRECTORY user variable set

Problem: Azure Storage Service is not running. Launch the development fabric by starting the solution.

WARNING: No automated fix availab le for 'Azure Storage Service is running'

WARNING: Abandoning remainder of script due to critical failures.

Figure: Good example - when running with -fix this script tries to automatically fix the problem

PS C:\Code\Northwind> .\Setup-Environment.ps1 -fix

Problem: Azure Storage Service is not running. Launch the development fabric by starting the solution.WARNING: No automated fix available for 'Azure Storage Service is running'

WARNING: Abandoning remainder of script due to critical failures.

Figure: Good example - Note that on the 2nd run, issues resolved by the 1st run are not re-reported

Unit Testing

Figure: Nice Unit Tests explain what the code is supposed to be doing.

Figure: Most young developers are happy with good old stepping through code with F11. The good thing is there are no diagrams that become out of date (which they always do after the first couple of sprints) giving you nasty Technical Debt.

Figure: Don't forget that you have the completed requirements which get done and archived and can now serve as free documentation e.g. User Stories (aka PBIs)

Figure: Annotations marry up the code with the PBIs, showing who, what, why and when for each piece of code

Level 3+: The rest of the jigsaw

Scrum Tip: Update your Acceptance Criteria - If you subscribe to this work item check-in policy, then you understand that the PBI is now the documentation. If requirements change during the course of the PBI (based on a conversation with Product Owner of course) then the PBI should be updated.

When updating the acceptance criteria, strike through the altered acceptance criteria and add the new ones. Get the PO to confirm your understanding.

E.g.Enter search text, click ‘Google’, and see the results populate below.[Updated]Enter search text and automatically see the results populate below.

One way is quick but messy - it will make further changes harder in the future (i.e. quick and dirty).

The other way is cleaner – it will make changes easier to do in the future, but will take longer to put in place.

'Technical Debt' is a metaphor to help us think about this problem. In this metaphor (often mentioned during Scrum software projects), doing things the quick and dirty way gives us a 'technical debt', which will have to be fixed later. Like a financial debt, the technical debt incurs interest payments - in the form of the extra effort that we have to do in future development.

We can choose to continue paying the interest, or we can pay the debt up front by redoing the piece of work in the cleaner way.

The same principle is true with documentation. Using the 'old school' method will leave you with a build-up of documentation that you will need to keep up-to-date as the project evolves.

Warning: if you want to follow Scrum and have zero technical debt, then you have to throw away all documentation at the end of each sprint. If you do want to keep it, make sure you add it to your definition of done to keep it updated.

The technologies and design patterns form the architecture that is usually the stuff that is hard to change.

A pattern allows using a few words to a dev and he knows exactly what coding pattern you mean.

ALM is about refining the work processes.​

We are doing this project using C#​

Bad example - you know nothing about how the project will be done

Technologies: MVC4. The DI container is Structure Map. Entity Framework. Typescript. Knockout.Patterns: Repository and Unit of Work (tied to Entity Framework to remove additional abstraction), IOCALM: Scrum with 2 week sprints and a Definition of Done including StyleCop to greenALM: Continuous deployment to staging

Good example - this tells you a lot about the architecture and processes in a few words

The important ones for most web projects:

Technologies: MVC

Patterns: Single responsibly - if it does more than one thing, then split it. Eg. If it checks the weather and gets data out of the database, then split it.

Patterns: Inversion of control / dependency injection Eg. If your controller needs to get data, then you inject the component that gets the data.

Patterns: Repository/Unit of Work - repository has standard methods for getting and saving data. The code calling the repository should not know where the data lives. Eg. A User Repository could be saving to Active Directory or CRM and it should not affect any other code You may or may not choose to have an additional abstraction away from entity framework.

ALM: Scrum - kind of a pattern for your process.E​g. Sprint Review every 2 weeks. Mostly a senior architect should be added for that 1 day each 2 weeks.

To visualize the structure of all your code you need architecture tools that will analyse your whole solution.

They show the dependencies between classes and assemblies in your projects.
You have 2 choices:

Visual Studio's Dependency Graph. This feature is only available in Visual Studio Ultimate. (recommended)

If you want architecture tools for Visual Studio, but don't have Visual Studio Ultimate, then the excellent 3rd party solution nDepend. A bonus is that it can also find issues and highlights them in red for easy discovery.

Figure: VS2012 lets you generate a dependency graph for your solution.Figure: The dependency graph in VS2012 shows you some interesting information about how projects relate to each other

nDepend has a similar diagram that is a little messier, but the latest version also includes a "Queries + Rules Explorer" which is another code analysis tool

Rather than randomly browsing for dodgy code, use Visual Studio's Code Metrics feature to identify "Hot Spots" that require investigation.

​Figure: The bad was is to browse the codeFigure: Run Code Metrics in VS2012Figure: Red dots indicate the code that is hard to maintain. E.g. Save() and LoadCustomer()

Identifying the problem areas is only the start of the process. From here, you should speak to the developers responsible for this dodgy code. There might be good reasons why they haven't invested time on this.

Figure: Find out who the devs are by using the Annotate tool, and start a conversation.

What order?

Look for Single Responsibility Principle violations. These are the most common and are the source of many other issues. Reducing the size and complexity of your classes and methods will often resolve other problems.

Liskov Substitution and Dependency Inversion are the next most common violations, so keep an eye out for them next.

When teams first begin implementing Dependency Injection, it is common for them to generate bloated interfaces that violate the Interface Segregation Principle.

After you have identified and corrected the most obvious broad principle violations, you can start drilling into the code and looking for localized code breaches. ReSharper from JetBrains o​r JustCode from Telerik are invaluable tools once you get to this level.

Once you understand common design principles, look at common design patterns to help you follow them in your projects.

Additionally, code that has high coupling violates the Dependency Inversion principle. This makes code difficult to change, but can be resolved by implementing the Inversion of Control *and* Dependency Injection patterns.

GRASP stands for General Responsibility Assignment Software Patterns and describes guidelines for working out what objects are responsible for what areas of the application.

​The fundamentals of GRASP are the building blocks of Object Oriented design. It is important that responsibilities in your application are assigned predictably and sensibly to achieve maximum extensibility and maintainability.

GRASP consists of a set of patterns and principles that describe different ways of constructing relationships between classes and objects.

Creator

A specific class is responsible for creating instances of specific other classes (e.g. a Factory Pattern)

​Information Expert

Responsibilities are delegated to the class that holds the information required to handle that responsibility​

​Controller

​System events are handled by a single "controller" class that delegates to other objects the work that needs to be done

​Low Coupling

Classes should have a low dependency on each other, have low impact if changed, and ​have high potential for reuse

​High Cohesion

​Objects should be created for a single set of focused responsibilities

​Polymorphism

​The variation in behaviour of a type of object is the responsibility of that type's implementation

​Pure Fabrication

​Any class that does not represent a concept in the problem domain

​Indirection

​The responsibility of mediation between two classes is handled by an intermediate object (e.g. a Controller in the MVC pattern)

​Protected Variations

​Variations in the behaviour of other objects is abstracted away from the dependent object by means of an interface and polymorphism

Tip: Visual Studio's Architecture tools can help you visualise your dependencies. A good structure will show calls flowing in one direction.

Figure: Bad Example - Calls are going in both directions which hints at a poor architectureFigure: Good Example - Calls are flowing in one direction hinting at a more sensible arrangement of responsibilities

Mocking frameworks allow you to replace a section of the code you are about to test, with an alternative piece of code.
For example, this allows you to test a method that performs a calculation and saves to the database, without actually requiring a database.

There are two types of mocking framework.

The Monster Mocker (e.g. Microsoft Fakes or TypeMock)

This type of mocking framework is very powerful and allows replacing code that wasn’t designed to be replaced.
This is great for testing legacy code (tightly coupled code with lots of static dependencies) and SharePoint.

Figure: Bad Example – Our class is tightly coupled to our authentication provider, and as we add each test we are adding *more* dependencies on this provider. This makes our codebase less and less maintainable. If we ever want to change our authentication provider “OAuthWebSecurity”, it will need to be changed in the controller, and every test that calls it

The mocking framework creates substitute items to inject into the code under test.

Figure: Good Example - An interface describes the methods available on the provider

Figure: Good Example - The authentication provider is injected into the class under test (preferably via the constructor)

Figure: Good Example - The code is loosely coupled. The controller is dependent on an interface, which is injected into the controller via its constructor. The unit test can easily create a mock object and substitute it for the dependency. Examples of this type of framework are Moq and NSubstitute

Figure: Bad Example - The connection string is hard-coded and isn't easy to see in the IDE.Figure: Better Example - The connection string is still hard-coded, but at least it's very visible to the developers.Figure: Good Example - The connection string is now stored in configuration and we don't have a long hard-coded string in the code.

Reflection code is often used to implement a "plugin" architecture where components are loaded at runtime rather than referenced directly in the code. This is done instead of statically referencing classes to make the solution more flexible.

The Managed Extensibility Framework (MEF) is an Inversion of Control (IoC) framework build on Reflection that simplifies and standardises this plugin methodology.

You don't need an IoC container or ServiceLocator for everything, but an IoC container WILL help if you have complex dependency graphs to instantiate (in your default constructor) or you have truly pluggable components. For example, if you want to allow a component to be picked up automatically at runtime from some assembly if it’s in a folder.

Any existing Reflection code should be examined to see whether:

It needs reflection at all - can the component be directly referenced? OR

​Using Managed Extensibility Framework to load dynamic dependencies at runtime can be useful, but it's not always required and does have some disadvantages. You shouldn't always look to MEF to implement a dynamic strategy.

​If a reference doesn't need to be dynamically loaded at runtime, it's perfectly fine to have a default constructor that has a hardcoded instantiation of a dependency. If it was never a requirement to make that thing configurable or dynamic, don't invent business requirements just because using an IoC container is "fancier".

There are disadvantages to using dynamic loading of references:

You lose your Code Analysis. Only static references can be analysed by code analysis tools.

You lose your traceability. Visual Studio can no longer show you what concrete method is being called at design time.

Kent Beck is the man credited with "rediscovering" the Test Driven Development methodology. It's a great way to ensure your code works as expected and it will allow you to catch errors that occur down the track.

Based on Kent Beck's principles, you should:

Write code as it spews out of your brain

Do lots of small refactoring rather than big architectural rewrites

If you are going to change code, add a test first (AKA red-green-refactor)

When selecting a Dependency Injection container it is worth considering a number of factors such as:

Ease of use

Configurability: Fluent API and/or XML Configuration

Performance (Unless you have a very high traffic application the difference should be minimal)

NuGet Support (only Ninject is doing a poor job of this) - see image

The top tools all contain comparable functionality. In practice which one you use makes little difference, especially when you consider that your container choice should not leak into your domain model.

Important: Unless a specific shortfall is discovered with the container your team uses, you should continue to use the same container across all of your projects, become an expert with it and invest time on building features rather than learning new container implementations.

Figure: Bad Example - Ninject was a top container but is no longer developed as actively as Autofac and Structuremap. Both Autofac and Structuremap have active communities and contributors that ensure they stay up to date with the latest changes in .Net

Figure: Good Example - Autofac has a great combination of performance and features and is actively developed

Figure: Good Example – The ReSharper Dependency graph groups dependencies based on Solution Folders. By having a
Consistent Solution Structure it is easy to see from your Dependency Graph if there is coupling between your UI and your Dependencies

Further Reading:

​Comments can be useful for documenting code but should be used properly. Some developers like seeing lots of code comments and some don't.

Some tips for including comments in your code are:

Comments aren't always the solution. Sometimes it's better to refactor the comments into the actual method name. If your method needs a comment to tell a developer what it does, then the method name is probably not very clear.

Comments should never say *what* the code does, it should say *why* the code does it. Any decent developer can work out what a piece of code does.

Comments can also be useful when code is missing. For example, why there is no locking code around a thread-unsafe method.

// returns the Id of the first customer with the matching last namepublic int GetResult(string lastname){// get the first matching customer from the repository return repository.Customer.First(c => c.LastName.StartsWith(lastname));} Figure: Bad Example - The first comment is only valuable because the method is poorly named, while the second describes *what* is happening, not *why*.public int GetFirstCustomerWithLastName(string lastname){// we use StartsWith because the legacy system sometimes padded with spaces return repository.Customer.First(c => c.LastName.StartsWith(lastname));}Figure: Good Example - The method has been renamed so no comment is required, and the comment explains *why* the code has been written in that way.

Good code is so nice it doesn't need comments, but when it does:

Includes comments that explain the intent (the "why" rather than the "what")

public Customer GetFirstCustomerWithLastName(string lastName){ // we use StartsWith because the legacy system sometimes padded with spaces return _repository.Customer.FirstOrDefault(c => c.LastName.StartsWith(lastName));}Figure: Good comments explain the intent of the code rather than what it is doing

Whenever you are writing code, you should always make sure it conforms to your team's standards. The more pain a developer has when trying to check in the better, because there will be less left up to testers to find.

No matter how good a coder you are, you will always miss some of them some of the time, so it's a really good idea to have a tool that automatically scans your code and reports on what you need to change in order to improve it.

Visual Studio has a great Code Analysis tool to help you look for problems in your code. Combine this with Jetbrain's ReSharper and your code will be smell free.

The levels of protection are:

Figure: You wouldn't play cricket without protective gear and you shouldn't code without protective tools

Level 3

Level 4

Is to use StyleCop to check that your code has consistent style and formatting.

Figure: StyleCop shows a lot of warnings in this test project

Level 5

Run Code Analysis (was FxCop) with the default settings or ReSharper with Code Analysis turned on

Figure: Run Code Analysis in VS2015

Figure: The Code Analysis results indicate there are 17 items that need fixing

Level 6

Ratchet up your Code Analysis Rules until you get to 'Microsoft All Rules'

Figure: Start with the Minimum Recommended Rules, and then ratched up.

Level 7

Is to document any rules you've turned off.

All of these rules allow you to disable rules that you're not concerned about. There's nothing wrong with disabling rules you don't want checked, but you should make it clear to developers why those rules were removed.

Create an _InstructionsCodeAnalysis.doc document in your solution with the rules that have been turned off and why.

Figure: The document _InstructionsCodeAnalysis.doc shows that there were 3 StyleCop rules disabled.

Suggestion to MS: Allow developers to put a comment against any disabled rule when you turn it off

Level 8

The gold standard is to use Sona rQube, which gives you the code analysis that the previous levels give you as wells as the ability to analyze technical debt and to see which code changes had the most impact to technical debt

Figure: SonarQube workflow with Visual Studio and TFS

Figure: SonarQube gives you the changes in code analysis results between each check-in

​​We already know what the best IOC container is, but how does ASP.NET core's default dependency injection compare​?

ASP.NET 5 includes default dependency injection for new Web Apps in the Startup.cs file. This is adequate for simple projects, but not designed to compete with the features of alternatives containers (like AutoFac's convention based registration).

"The default services container provided by ASP.NET 5 provides a minimal feature set and is not intended to replace other containers.​​" - Steve Smith, (ASP.NET Dependency Injection )

Don't waste time evaluating which middle tier .Net libraries to use. Most of the commonly used libraries are very similar in fuctionality. By sticking to a library, you can also increase your expertise in it, reducing development time in the future.

​Don't waste time evaluating which Web UI libraries to use. Most of the commonly used libraries are very similar in functionality . The recommendlibrary is Twitter Bootstrap.

It's the most popular available framework today, which means more people involved in the project, more tutorials and articles from the community, more real-world examples/websites, more third-party extensions, and better integration with other​ web development products

Figure: Bad example - Zurb Foundation is the second most popular, but it uses Sass files and​ has the worst name

In VS 2013 Zurb Foundation was always out of date on Nuget. So if you use it (which is not recommended) download it direct from ​Github.​​

A common practice we see when developers start to use IOC containers is that the IOC container becomes a central service and configuration repository that all the components across the project become dependent upon.

​Using an IOC container in this manner can bring advantages such as centralised configuration and dependency lifecycle and scope managment. If implemented correctly, however, your classes can benefit from the above without any direct dependency on the IOC container itself.

Figure: Bad Example - the dependency is manually fetched from the IOC container, This class now has a hard dependency on your IOC container

Figure: Good example - The dependency is enforced via a constuctor parameter. The class does not need to know anything about the IOC container being used and can potentially be reused in different contexts and with different IOC containers.

Help and improve these rules

Nothing great is easy. The SSW rules are a great resource for developers all around the world.
However it’s hard to keep rules current and correct. If you spot a rule that is out of date, please email or if you are cool tweet me.