This section is where the detailed code review resides. If a system has been in production for a period the code base will start diverging from the originally envisaged enterprise architectural pattern. Common issues include:

• In a layered architecture some new components might bypass layers and connect straight to the database
• A system that used to be solely messaging based might start using files or direct database access instead
• Multiple systems might connect directly to the database as opposed to going via a shared service

As for the detailed code review there are simple checks to do up front:

• Use a code duplication tool to determine the amount of “Copying and Pasting” has been used
• Use Resharper to see how many issues it can find
• Determine if there are any unit and integration tests
• If the tests are excessively long
• Is “Arrange” step is overly complex
• Are tests focussed on one problem or do they contain dozens of “Asserts”
• Are there an excessive number of solution files and projects
• Team might be confusing logical and physical separation
• Design doesn’t include Aggregate roots and trees of interrelated objects and instead is modelling at the database table level
• May include unnecessary abstractions and layers that could be collapsed
• Run NDepend or another tool that checks for cyclomatic complexity
• Review the 10 largest methods in the application
• Do they look reasonable?
• Are they factories that populate Data Transfer Objects and could be replaced with AutoMapper?
• Are they mixing data access with rending code?
• Is the code generated?

Transactions

How are transactions handled in the application?

• Enterprise Services/COM+/XA
• ADO.NET/JDBC
• In stored procs

Is it consistent?

• All service methods create a TransactionScope
• AOP injected
• All components hosted in COM+

Is data integrity ensured?

• NOLOCK scattered throughout the code base
• NoSQL database utilised and service writes to multiple documents without mutual exclusion strategy
• Summary tables aren’t consistently populated and thus can drift out of sync

Logging

Logging is consistent across application?

• All service method write parameters to a log
• All use same logging framework
• Write to the same logging repository

Is application support team able to resolve issues by utilising the logs?

Will the application fail in production if logging sink goes down?

• Logs are written to database table and if that is down application cant start
• Audit Web Service fails and orders can’t be processed

System provides data changes auditing?

Security

Role based security is used?

• Users are assigned roles in the application
• Accounts that run the Web Services and Application Servers are assigned roles in the database and not given rights directly

Authorisation checks are made in a consistent manner?

• MVC custom attributes used
• AOP injects authorisation check into all Web Service calls
• Security Context is assigned to user at login and is then passed into each subsequent request

Active Directory or LDAP utilised

• Internal enterprise applications should not develop a custom user authentication module
• Integrated security preferred to connection strings including username/password

Presentation Layer

What presentation patterns are used?

• MVC, MVP, MVVM
• Where does rendering occur
• On server
• On client
• Two way data binding used

Client code follows best practices?

• CSS and Javascript in files so that they can be cached on client
• CSS used over inline styles
• ViewModels used over events

Performance

Is data movements minimised?

• Sorting, paging and filtering occurs in the database and not in the application server
• Aggregation operations are performed only once
• Session data is minimised to reduce copying to and from repository
• SELECT N + 1 anti-pattern does occur

Caching

• Is cache centralised?
• If local and distributed caches are used does invalidation logic work correctly?
• Is it possible to invalidate cache items easily?
• Are inputs cached?
• Are results cached?
• Is cache utilised to protect application from external component failures?
• HR system contains staff home addresses and when it goes down application stops
• Each hour all addresses are loaded into cache if HR system not contactable leave stale items in cache

Does the system utilise asynchronous operations?

• The majority of business operations should be performed asynchronously using a message queues
• Under heavy load queues can be throttled to reduce work on application servers
• If external system is down application can continue working by pushing commands onto local queues

Load balancing

• System is stateless and doesn’t require sticky sessions
• Load balancer is intelligent and allocates work to the least busy server as opposed to simply using Round Robin scheduling
• Load balancer doesn’t route traffic to web server if it is off i.e. should not use a ping check
• SSL encryption is done on Load Balancer to reduce load on the web servers

High Availability

• Multiple servers in each physical tier in the application
• Physical tiers are known by Virtual IPs and not by machine names
• Each servers installation within a physical tier is identical
• Manual intervention is not required when a server fails
• Not Highly Available
• Multiple application servers but some clients only connect to a single machine
• File shares not redundant
• Licensing issues meant implementation is Hot/Cold (Active/Passive) as opposed to Hot/Hot (Active/Active)

Resource Management

Are resources cleaned up correctly?

• Database connections released to pool programmatically and not by garbage collector
• Events are detached so that views are able to be collected when no longer used
• IDisposable/using statement used throughout the code
• LINQ statements that connect to the database are evaluated immediately and not by the caller
• Code is exception safe i.e. if exception is thrown in middle of method the data structure isn’t left in a corrupted inconsistent state

Locking strategy is consistent and modern?

• Active Object pattern and producer consumer queues used over lock statements
• If Optimistic locking strategy is utilised are summary tables correctly written to

Comments

• Comments should not replace source control
• Defects or changes should not be marked with developers name
• Changes should not be dated
• Prefer informative variable, method and class names over comments
• Prefer “Extract Method” refactoring to comments
• XML comments
• Are they used?
• If so are they populated or empty?
• Prefer no XML comments to empty comments