Rules to Better Architecture and Code Review

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!

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.

  1. Do you evaluate the processes?

    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.

    1. Are devs getting bogged down in the UI?
    2. Do you have continuous integration and deployment?
    3. Do you have a Schema Master?
    4. Do you have a TFS Master?
    5. Do you have a Scrum Master?

    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.

  2. Do you make sure you get latest and compile?

    It's amazing how often you can't simply do a "Get Latest" and compile.

    ​A good developer makes it clear how to get a new project and compile it.

    Check they have instruction files in their solution as per the Do you make instructions at the beginning of a project and improve them gradually? rule.

  3. Do you review the Solution and Project names?

    The name of your solution and the names of the projects in your solution should be consistent.

    Follow the rule: Do you have a consistent .Net Solution Structure?

    solutionlayout.png
    Figure: Good Example - The Solution and Projects are named consistently
  4. Do you conduct an architecture review after every sprint?

    There are 2 main parts to any application. The UI which is what the customer can see and provide feedback on, and the underlying code which they really can't know if it is healthy or not.

    Therefore it is important to conduct a 'test please' on the internal code and architecture of the application.

    Ideally conduct a small 'Code + Architecture Review' for every sprint. Assuming a 2 week sprint, schedule a 4 hour (2 architects x 2 hours) review during all sprints.

    The following are items that are addressed in an architecture/code review:

    Background information/overview of the project

    • Current system
    • Objectives of the system
    • Number of users (internal, external, edit, read only)
    • Current technologies
    • Current environment (SOA, SOE)

    Points for discussion

    • Rich client
    • Web client
    • Smart client (any disconnected users?)
    • Technology choices
      • Persistence layer (SQL Server, Access, SQL Express, LINQ, netTiers)
      • Business layer
      • UI
      • Communications
      • Workflow
      • Integration - external systems
    • Requirements for 'package' software
      • PerformancePoint
      • Reporting Services
      • Accounting packages
      • SharePoint
    • Data migrations
    • Data reporting
    • User experience
    • Network
    • Responsibilities/players
    • Infrastructure
      • Network
      • Hardware
    • Deployment
      • Staged implementation
      • In parallel
      • Development/Test/Staging/Production
    • Disaster recovery
    • Change control/source control
    • Build Server
    • Performance
    • Scalability
    • Extensibility
    • Design patterns
    • Maintainability
    • Reliability (failover servers?)
    • 'Sellability' i.e. is the solution appropriate for the client?

    Note: If you are using Enterprise Architect, be aware of technical debt:

    • Add a datetime of the last time the diagram was modified so we have an indication of when it is out of date
    • On your diagrams, be aware that some parts are done and some are not.
  5. Do you review the documentation?

    There are 2 styles of documentation:

    Style #1: Old School:

    IwS2

    ​This style of team 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.

    enterprisearchitect1
    Figure: Documentation can take the form of Sequence Diagrams
    enterprisearchitectusecases.png
    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).

    Style #2: New School:

    Mark Zuckerberg​This style of team 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):
    • Business.docx - Explaining the business purpose of the app
    • Instructions_Compile.docx - Contains instructions on how to get the project to compile (aka the F5 experience)
    • Instructions_Deployment.docx - Describes the deployment process
    • Patterns_And_Technologies.docx - Explaining the technical overview e.g. Broad architecture decisions, 3rd party utilities, patterns followed etc. (ie. SSW Data Onion)
    • Definition_Of_Done.docx – Ensures that your team maintains a high level of quality with a Definition of Done
    • Definition of Ready.docx – Ensure that your PBIs are well defined before adding them to a sprint by specifying a Definition of Ready
    • Unit Tests​
    • Code and Work Items (Via the magic of Annotation)
    ProjectDocumentation.jpg
    Figure: 4 small docs explain most of what you need to know very briefly.
    UnitTestExplorer.png
    Figure: Nice Unit Tests explain what the code is supposed to be doing.
    vs11debug.png
    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.
    tfspreviewbacklog.png
    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)
    Annotation and Comment
    Figure: Annotations marry up the code with the PBIs, showing who, what, why and when for each piece of code

    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.

    This should be added to the definition of done.

    Technical Debt

    What's "Technical Debt"?

    During a project, when you add functionality, you have a choice:

    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.

  6. Do you document the technologies, design patterns and ALM processes?

    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), IOC
    ALM: Scrum with 2 week sprints and a Definition of Done including StyleCop to green
    ALM: 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:

    1. Technologies: MVC
    2. 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.
    3. Patterns: Inversion of control / dependency injection
      Eg. If your controller needs to get data, then you inject the component that gets the data.
    4. 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.
    5. 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.

    The decisions the team makes regarding these 3 areas, should be documented in _Technologies.docx as per http://rules.ssw.com.au/SoftwareDevelopment/RulestobetterArchitectureandCodeReview/Pages/DoYouReviewTheDocumentation.aspx​.​

  7. Do you look for native code that’s missing dispose? Unpublished

    ​​a. Suggestion to MS: make a tool 

  8. Do you look at the architecture?

    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.
    architecturetools_vs11.pngFigure: VS2012 lets you generate a dependency graph for your solution. sqldeploy_dependencies.png 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

    nDepend.pngFigure:nDepend Dependency Graph. Issues are highlighted in red for easy discovery.

    Read more about nDepend: http://www.ndepend.com/

    Warning: nDepend doesn't yet support Visual Studio 2012 (Aka VS11) with a plugin.

  9. Do you know how to laser in on the smelliest code?

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

    467510-lotto-balls.jpeg ​Figure: The bad was is to browse the code Run Code Metrics Figure: Run Code Metrics in VS2012 Red dots indicate the code that is hard to maintain Figure: 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.

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

    Tip: To learn how to use Annotate, see Do you know the benefits of Source Control?.

    Suggestion to MS: allow us to visualize the developers responsible for the bad code (currently and historically)

  10. Do you know the common Design Principles? (Part 1)

    SRP The Single Responsibility Principle
    A class should have one, and only one reason to change.

    OCP The Open Closed Principle 
    You should be able to extend a classes behavior without modifying it.

    LSP The Liskov Substitution Principle 
    Derived classes must be substitutable for their base classes.

    ISP The Interface Seg regation Principle 
    Make fine-grained interfaces that are client-specific.

    DIP The Dependency Inversion Principle 
    Depend on abstractions, not on concretions.

    Figure: Your code should be using SOLID principles​​

    It is assumed knowledge that you know all 5 SOLID principles. If you don't, read about them on Uncle Bob's site above, or watch the SOLID Pluralsight videos by Steve Smith.

    What order?

    1. 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.
    2. Liskov Substitution and Dependency Inversion are the next most common violations, so keep an eye out for them next.
    3. 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.

  11. Do you know the common Design Principles? (Part 2 - Example)

    The hot spots identified in your solution often indicate violations of common design principles.

    Check Address Figure: Check Address.Save() and Customer.LoadCustomer() looking for SOLID refactor opportunities

    The most common problem encountered will be code that violates the Single Responsibility Principle (SRP). Addressing SRP issues will see a reduction in the following 3 metrics:

    1. "Cyclomatic Complexity" which indicates that your methods are complex, then
    2. "High Coupling" indicates that your class/method relies on many other classes, then
    3. "Number of Lines" indicates code structures that are long and unwieldy.

    Let's just look at one example.

    This code does more than one thing, and therefore breaks the Single Responsibility Principle.

    public class PrintServer 
    {
        public string CreateJob(PrintJob data) { //...
        }
        public int GetStatus(string jobId) { //...
        }
        public void Print(string jobId, int startPage, int endPage) { //...
        }
        public List GetPrinterList() { //...
        }
        public bool AddPrinter(Printer printer) { //...
        }
        public event EventHandler PrintPreviewPageComputed;
        public event EventHandler PrintPreviewReady;
        // ...
    }
    
    Figure: Bad example - This class does two distinct jobs. It creates print jobs and manages printers.
    public class Printers {
        public string CreateJob(PrintJob data) { //...
        }
        public int GetStatus(string jobId) { //...
        }
        public void Print(string jobId, int startPage, int endPage) { //...
        }
    }
    public class PrinterManager {
        public List GetPrinterList() { //...
        }
        public bool AddPrinter(Printer printer) { //...
        }
    }
    
    Figure: Good Example - Each class has a single responsibility.

    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.

    TODO: Replace example with TailSpin

    TODO: Updated Code Metrics diagram after fix

  12. Do you know the common Design Patterns? (Part 1)

    Design patterns are useful for ensuring common design principles are being followed.  They help make your code consistent, predictable, and easy to maintain.

    ​There are a very large number of Design Patterns, but here are a few important ones.

    ​IOC ​Inversion of Control Control of the object coupling is the responsibility of the caller, not the class.
    ​DI Dependency Injection Dependencies are "injected" into the dependent object rather than the object depending on concretions.
    ​Factory ​Factory Pattern ​Object creation is handled by a "factory" that can provide different concretions based on an abstraction.
    ​Singleton ​Singleton Pattern ​Instantiation of an object is limited to one instance to be shared across the system.
    ​Repository ​Repository Pattern ​A repository is used to handle the data mapping details of CRUD operations on domain objects.
    ​Unit of Work ​Unit of Work Pattern ​A way of handling multiple database operations that need to be done as part of a piece of work.
    ​MVC ​Model View Controller ​An architectural pattern separating domain logic (Controller) from how domain objects (Models) are presented (View).
    ​MVP ​Model View Presenter ​An architectural pattern deriving from MVC where the View handles UI events instead of the Controller.
    Figure: Choose patterns wisely to improve your solution architecture

    It is assumed knowledge that you know these design patterns. If you don't, read about them on the sites above or watch the PluralSight videos on Software Patterns.

  13. Do you know the common Design Patterns? (Part 2 - Example)

    ​Appropriate use of design patterns can ensure your code is maintainable.

    Always code against an interface rather than a concrete implementation. Use dependency injection to control which implementation the interface uses.

    For example, we could implement Inversion of Control by using the Dependency Injection pattern to decrease the coupling of our classes.

    In this code, our controller is tightly coupled to the ExampleService and as a result, there is no way to unit test the controller.

    [This example is from the blog: http://www.devtrends.co.uk/blog/how-not-to-do-dependency-injection-the-static-or-singleton-container]

    public class HomeController
    {
        private readonly IExampleService _service;
        public HomeController()
        {
          _service = new ExampleService();
        }    
        public ActionResult Index()
        {
            return View(_service.GetSomething());
        }
    }​
    Figure: Bad example - Controller coupled with ExampleService
    public class HomeController
    {
        private readonly IExampleService _service;
         
        public HomeController()
        {
          _service = Container.Instance.Resolve<IExampleService>();
        }
        
        public ActionResult Index()
        {
            return View(_service.GetSomething());
        }
    }
    Figure: Bad example - 2nd attempt using an Inversion of Control container but *not* using dependency injection. A dependency now exists on the Container.

    This is bad code because we removed one coupling but added another one (the container).

    public class HomeController
    {
        private readonly IExampleService _service;
         
        public HomeController(IExampleService service)
        {
          _service = service;
        }
         
        public ActionResult Index()
        {
            return View(_service.GetSomething());
        }
    }​
    Figure: Good example - code showing using dependency injection. No static dependencies.

    Even better, use Annotate so you can enlighten the developer.

    bad.png
    Figure: Bad Example - Referencing the concrete EF context


    Figure: Good Example - Programming against the interface

    It is important to know when the use of a pattern is appropriate.  Patterns can be useful, but they can also be harmful if used incorrectly.

  14. Do you look for GRASP 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.

    architecture_responsibility_bad.png Figure: Bad Example - Calls are going in both directions which hints at a poor architecture architecture_responsibility_good.png Figure: Good Example - Calls are flowing in one direction hinting at a more sensible arrangement of responsibilities
  15. Code - Can you read code down across

    Reading down should show you the what (all the intend)

    Reading across should show you the how (F12)​

  16. Do you start reading code?

    “Aim for simplicity. I want to code to read like poetry”
    - Terje Sandstrom

    Good code:

    • Is clear and easy to read
    • Has consistent and meaningful names for everything
    • Has no repeated or redundant code
    • Has neat formatting
    • Explains "why" when you read down, and "how" when you read left to right
    public IEnumerable<Customer> GetSupplierCustomersWithMoreThanZeroOrders(int supplierId)
    {
        var supplier = _repository.Suppliers.Single(s => s.Id == supplierId);

        if (supplier == null)
        {
            return Enumerable.Empty<Customer>();
        }
        var customers = supplier.Customers
            .Where(c => c.Orders > 0);

        return customers;
    }
    Figure: This code explains what it is doing as you read left to right, and why it is doing it when you read top to bottom.

    Tip: Read the book Clean Code: A Handbook of Agile Software Craftsmanship by Robert. C. Martin.

    Good code is declarative:

    For example, I want to show all the products where the unit price less than 20, and also how many products are in each category.

    Dictionary<string, ProductGroup> groups = new Dictionary<string, ProductGroup>();
    foreach (var product in products)
    {
        if (product.UnitPrice >= 20)
        {
            if (!groups.ContainsKey(product.CategoryName))
            {
                ProductGroup productGroup = new ProductGroup();
                productGroup.CategoryName = product.CategoryName;
                productGroup.ProductCount = 0;
                groups[product.CategoryName] = productgroup;
            }
            groups[p.CategoryName].ProductCount++;
        }
    }var result = new List<ProductGroup>(groups.Values);
    result.Sort(delegate(ProductGroup groupX, ProductGroup groupY)
    {
        return
            groupX.ProductCount > groupY.ProductCount ? -1 :
            groupX.ProductCount < groupY.ProductCount ? 1 :
            0;
    });
    Figure: Bad example - Not using LINQ. The yellow gives it away.

    Tip: Resharper can automatically convert this code.

    result = products
        .Where(product => product.UnitPrice >= 20)
        .GroupBy(product => product.CategoryName)
        .OrderByDescending(group => group.Count())
        .Select(group => new { CategoryName = group.Key, ProductCount = group.Count() });
    Figure: Good example - using LINQ

    Tip: For more information on why declarative programming (aka LINQ, SQL, HTML) is great, watch the TechDays 2010 Keynote by Anders Hejlsberg.Anders explains why it's better to have code "tell what, not how".

    Clean front-end code - HTML (This one is questionable as HTML is generally a designer issue)

    Anyone who creates their own HTML pages today should aim to make their markup semantically correct. For more information on semantic markup, see http://www.webdesignfromscratch.com/html-css/semantic-html/.

    For example:

    • <p> is for a paragraph, not for defining a section.
    • <b> is for bolding, not for emphasizing (<strong> and <em>) do that.

    Clean front-end code - JavaScript

    Clean code and consistent coding standards is not just for server-side code.  It is important that you apply your coding standards to your client-side JavaScript code as well.

    You should use a framework like jQuery to make development easier.  jQuery also contributes to clean and consistent code as you don't need to explicitly code for different browsers and standards.

    var ajax;
    
    
    }
    ajax.open("GET"
    , uri, true); 
    Figure: Bad Example - This code doesn't use jQuery
    
    
    $.ajax({
        t
    ype: 
    "GET",
        url: uri
    }).done(function (html) {
        $('results').append(html)
    ;
    });
    Figure: Good Example - using jQuery​, the code is much cleaner and easier to read

    Even better code usese Knockout.js for View Model binding with jQuery.

    Read more about Do you start reading code?
  17. Do you use a ‘Precision Mocker’ like Moq instead of a ‘Monster Mocker’ like Microsoft Fakes?

    Using a precision mocking framework (such as Moq or NSubstitute) encourages developers to write maintainable, loosely coupled code.

    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 Precision Mocker (e.g. Moq)

    This mocking framework takes advantage of well written, loosely coupled code.

    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
  18. Do you have opportunities to convert use Linq to entities?

    Look for inline SQL​ to see whether you can replace it with Linq to Entities.

    Speed camera Figure: SQL Injection for Speed Cameras :-)
  19. Do you look for opportunities to use Linq?

    Linq is a fantastic addition to .Net which lets you write clear and beautiful declarative code. Linq allows you to focus more on the what and less on the how.

    You should look for opportunities to replace your existing code with Linq.

    ​For example, replace your foreach loops with Linq.

    var lucrativeCustomers = new List<Customer>();
    foreach (var customer in Customers)
    {
        if (customer.Orders.Count > 0)
        {
            lucrativeCustomers.Add(customer);
        }
    }
    Figure: Bad Example - imperative programming using a foreach loop
    var lucrativeCustomers = Customers.Where(c => c.Orders.Count > 0).ToList();
    Figure: Good Example - declarative programming using Linq
  20. Do you use the repository pattern for data access?

    The repository pattern is a great way to handle your data access layer and should be used wherever you have a need to retrieve data and turn it into domain objects.

    The advantages of using a repository pattern are:

    • Abstraction away from the detail of how objects are retrieved and saved
    • Domain objects are ignorant of persistence - persistence is handled completely by the repository
    • Testability of your code without having to hit the database (you can just mock the repository)
    • Reusability of data access code without having to worry about consistency

    Even better, by providing a consistent repository base class, you can get all your CRUD operations while avoiding any plumbing code.

    See the "A Generic CRUD Repository for Entity Framework" blog post by Damian Brady for an example of how to implement a repository pattern.

  21. Do you look for large strings in code?

    Long hard-coded strings in a codebase can be a sign of poor architecture.

    ​To make hard-coded strings easier to find, consider highlighting them in your IDE.

    longstringbadexample.png
    Figure: Bad Example - The connection string is hard-coded and isn't easy to see in the IDE. longstringbadexample2.png Figure: Better Example - The connection string is still hard-coded, but at least it's very visible to the developers. longstringgood.png Figure: Good Example - The connection string is now stored in configuration and we don't have a long hard-coded string in the code.
  22. Do you know to replace reflection with MEF?

    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:

    1. It needs reflection at all - can the component be directly referenced? OR
    2. Can the pure reflection code be simplified with MEF?

    [TODO - Damian: Bad example - using reflection]

    [TODO - Damian Good Example - using MEF]

  23. MEF: Do you know not to go overboard with dynamic dependencies?

    ​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:

    1. You lose your Code Analysis. Only static references can be analysed by code analysis tools.
    2. You lose your traceability. Visual Studio can no longer show you what concrete method is being called at design time.
    More details on MEF can be foud here: http://msdn.microsoft.com/en-us/library/dd460648(v=vs.110).aspx
  24. Do you look for memory leaks?Unpublished

    a. These could be in the UI and middle level tiers

    b. There are common patterns for memory leak, e.g. proxy to WCF [code]

    e.g RIA service [code]

    e.g. Load +=     (9 out of 10 people will forget to remove their statement)

    ​Google: Common Memory Leak

    [bad code]

    [good code]

  25. Do you look for call back over event handlers?Unpublished

    [bad code]

    [good code]

  26. Do you look for Console.WriteLine and change it to Trace.WriteLine ?

    Unless you are writing a Console application, any output that uses Console.WriteLine will almost certainly be lost.  You're much better off using Trace.WriteLine.

    You can create a trace listener to send your Trace.WriteLine statements to the Console if you wish, but you can also redirect them elsewhere if required.  See this MSDN article for more information.

    Console.WriteLine("Service started"); Figure: Bad Example - Using Console.WriteLine to write debug information

    Trace.WriteLine("Service started"); Figure: Good Example - Using Trace.WriteLine to write debug informatio

  27. Do you look for duplicate code?Unpublished

    Code duplication is a big "code smell" that harms maintainability.  You should keep an eye out for repeated code and make sure you refactor it into a single place.

    For example, have a look at these two Action methods in an MVC 4 controller.

    //
    // GET: /Person/
    [Authorize]
    public ActionResult Index()
    {
        // get company this user can view
        Company company = null;
        var currentUser = Session["CurrentUser"] as User;
        if (currentUser != null)
        {
            company = currentUser.Company;
        }
    
        // show people in that company
        if (company != null)
        {
            var people = db.People.Where(p => p.Company == company);
            return View(people);
        }
        else
        {
            return View(new List());
        }
    }
    
    //
    // GET: /Person/Details/5
    [Authorize]
    public ActionResult Details(int id = 0)
    {
        // get company this user can view
        Company company = null;
        var currentUser = Session["CurrentUser"] as User;
        if (currentUser != null)
        {
            company = currentUser.Company;
        }
    
        // get matching person
        Person person = db.People.Find(id);
        if (person == null || person.Company == company)
        {
            return HttpNotFound();
        }
        return View(person);
    }
    
    Figure: Bad Example - The highlighted code is repeated and represents a potential maintenance issue.

    We can refactor this code to make sure the repeated lines are only in one place.

    private Company GetCurrentUserCompany()
    {
        // get company this user can view
        Company company = null;
        var currentUser = Session["CurrentUser"] as User;
        if (currentUser != null)
        {
            company = currentUser.Company;
        }
        return company;
    }
    
    //
    // GET: /Person/
    [Authorize]
    public ActionResult Index()
    {
        // get company this user can view
        Company company = GetCurrentUserCompany();
    
        // show people in that company
        if (company != null)
        {
            var people = db.People.Where(p => p.Company == company);
            return View(people);
        }
        else
        {
            return View(new List());
        }
    }
    
    
    // GET: /Person/Details/5
    [Authorize]
    public ActionResult Details(int id = 0)
    {
        // get company this user can view
        Company company = GetCurrentUserCompany();
    
        // get matching person
        Person person = db.People.Find(id);
        if (person == null || person.Company == company)
        {
            return HttpNotFound();
        }
        return View(person);
    }
    
    Figure: Good Example - The repeated code has been refactored into its own method.

    Tip: The Refactor menu in Visual Studio 11 can do this refactoring for you.

    vs_refactor_extract.png Figure: The Extract Method function in Visual Studio's Refactor menu.
  28. Do you look for Code Coverage?

    Code Coverage shows how much of your code is covered by tests and can be a useful tool for showing how effective your unit testing strategy is.  However, it should be looked at with caution.​​

    • You should focus on *quality* not *quantity* of tests.
    • You should write tests for fragile code first and not waste time testing trivial methods
    • Remember the 80-20 rule - a very high test coverage is a noble goal but there are diminishing returns.
    • If you're modifying code, write the test first, then change the code, then run the test to make sure it passes (AKA red-green-refactor).
    • You should run your tests regularly (see Do you follow a Test Driven Process). Ideally, they'll be part of your build (see Do you know the minimum builds to create on any branch)
    CodeCoverage_blurred.png
    F igure: Code Coverage metrics in VS2010. This solution has a very high code coverage percentage (around 80% on average)

    Hot Tip: Do you use Live Unit Testing to see code coverage?

  29. Do you use the Web API (REST)?Unpublished

          RIA services – RIP

    WCF – RIP(Now shipped in Asp.Net MVC4)

    SOAP - RIP

  30. Do you review the Builds?Unpublished

                Quote: don’t use gated check-ins because…. thanks Peter Provost

  31. Do you read “Timeless way of building” ( has relevance to software)?Unpublished

    This field should not be null (Remove me when you edit this field).
  32. Do you use the Kent Beck philosophy?

    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:

    1. Write code as it spews out of your brain
    2. Do lots of small refactoring rather than big architectural rewrites
    3. If you are going to change code, add a test first (AKA red-green-refactor)

    Tip: Read Michael Feather’s book, "Working Effectively with Legacy Code" for some insights into effective unit testing.

    Tip: Don't focus on the percentage of code coverage, focus on whether tests will touch the lines of code you care about​.

  33. Do you know the best dependency injection container? (aka Do not waste days evaluating IOC containers)

    You can waste days evaluating IOC containers. The top ones are quite similar. There is not much in this, but the best ones are StructureMap and AutoFac. At SSW we use Autofac on most projects.

    Other excellent DI containers are Ninject and Castle Winsdor. They have weaknesses, some are listed below.

    Dependency Injection is an essential ingredient to having maintainable solutions. IOC containers make doing dependency injection easier.

    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.

    dic-bad.png
    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
    dic-good.png alt=
    Figure: Good Example - Autofac has a great combination of performance and features a nd is actively developed

    Note: Autofac's support for child lifetime containers maybe significant for some:
    http://nblumhardt.com/2011/01/an-autofac-lifetime-primer

    StructureMap does also support a kind of child container:
    http://codebetter.com/jeremymiller/2010/02/10/nested-containers-in-structuremap-2-6-1/

    Autofac_web.png

    Figure: Good Example - the web / mvc integration package layer for Autofac is developed by the same core Autofac team. Some containers (such as Structure Map) require third-party integration layers  

    Further Reading:

  34. Do you decide on the level of the verboseness? E.g. ternary operators

    Do you believe in being verbose in your code (don't compress code and don't use too many ternary operators)? 

    ​Different developers have different opinions.  It is important your developers work as a team and decide together how verbose their code should be.

    What is your opinion on this?  Contribute to the discussion on Stack Overflow.

  35. Do you generate the VS Dependency Graph?

    ​Dependency graphs are important because they give you an indication of the coupling between the different components within your application.

    A well architected application (ie. one that correctly follows the Onion Architecture) will be easy to maintain because it is loosely coupled.

    ​​
    Figure: Bad Example- The Visual Studio Dependency Graph is hard to read
    TimePRODependence-good.png
    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:

  36. Do you review the code comments?

    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:

    1. Comments aren't aways 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.
    2. 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.
    3. 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 name
    public 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
  37. Do you use part of sprint review to drill into the next most important number in this list? Unpublished

    a. Use the retro to come up with better work practises
    b. Image of our “Scrum 8 steps” 

    Google: Tips for Retrospective

  38. Do you use the best Code Analysis tools?

    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:

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

    Level 1

    Get ReSharper to green on each file you touch. You want the files you work on to be left better than when you started. See Do you follow the boyscout rule?

    Tip: You can run through a file and tidy it very quickly if you know two great keyboard shortcuts:

    • Alt + [Page Down/Page Up] : Next/Previous Resharper Error / Warning
    • Alt + Enter: Smart refactoring suggestions
    Image 01
    Figure: ReSharper will show Orange when it detects that there is code that could be improved
    image002.png
    Figure: ReSharper will show green when all code is tidy

    Level 2

    Is to use Code Auditor.

    stylecop.png
    Figure: Code Auditor shows a lot of warnings in this test project

    Note: Document any rules you've turned off.

    Level 3

    Is to use Link Auditor.

    Note: Document any rules you've turned off.

    Level 4

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

    stylecop.png
    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

    runcodeanalysisvs11.png
    Figure: Run Code Analysis in VS2015
    Code Analysis
    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'

    image003.png
    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.

    More Information: Do you make instructions at the beginning of a project and improve the m gradually?

    stylecop_removed_rules.png
    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
    2016-06-08_12-59-38.png
    Figure:  SonarQube workflow with Visual Studio and TFS
    2016-06-08_12-59-53.png
    Figure: SonarQube gives you the changes in code analysis results between each check-in
  39. Do you know what to do about ASP.NET core (aka ASP.NET 5) default dependency injection?

    ​​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 )


    You can quckly flag this error and any more by using the SSW Code Auditor​.

    Here is an example of rewiring the default code to AutoFac with the SSW's Music Store​  app:

    SSW-DependencyInjection-Example-Default-Bad.png ​​​​
    Figure: Bad Example - ​​The default dependency injection for ASP.NET 5​
    ​​SSW-DependencyInjection-Example-Default-Good.png
    ​​Figure: Good Example - The bad example rewired to utilize​ AutoFac. Red boxes outline the modified code.

    Further Reading:​


  40. Do you use subdomains instead of virtual directories?

    Using subdomains over directories has 2 benefits:

    1. it is easier to host different sections of your website on different platforms, and

    2. in different geographic locations.

     

    ​​​​http://ssw.myservice.com/
    http://northwind.myservice.com/

    Figure: ​Good Example - A subdomain is used to distinguish ​organisations

    ​​​http://www.myservice.com/ssw/
    http://www.myservice.com/northwind/

    ​​Figure: Bad Example - Virtual directories are used to distinguish organisations
  41. Do you use the best middle tier .Net libraries?

    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.

    ​At SSW, we use:

  42. Do you use the best Web UI libraries?

    ​Don't waste time evaluating which Web UI libraries to use. Most of the commonly used libraries are very similar in functionality . The recommend  library 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

    zurb.png
    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.​​ 

    In VS 2015 these packages were moved from N​uget to Bower. 

    bootstrap.png
    Figure: Good example - leader among frameworks today, Bootstrap toolkit is recommended to build​ successful websites

    The 3 things a developer need to know to get up and running quickly with ASP.NET MVC

     

    Twitter Bootstrap & ASP.NET MVC ​​​​- Intro / Quickstart

     

    ​Other useful frameworks​

    Now that you saved a lot of UI develo​pment time by using Bootstrap, you can play around with other useful frameworks.​

    • KendoUI for enhanced HTML and jQuery controls
    • KnockoutJS for view-model data binding
    • SignalR for real-time web functionality​
  43. Do you use your IOC container to Inject Dependencies – and not as a singleton container

    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.

    IOC_badexample.png

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


    IOC_GoodExample.png

    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. 

     

    For more information and insight on IOC usage, read the following: ​http://www.devtrends.co.uk/blog/how-not-to-do-dependency-injection-the-static-or-singleton-container