Transcript A Modern Architecture Review Using Code Review Tools @AdamCogan | #vsalm #tee12 #dev324 Delivering Awesome Web Applications.

A Modern Architecture Review
Using Code Review Tools
@AdamCogan | #vsalm #tee12 #dev324
Delivering Awesome Web Applications
Agenda
•
#1 The 1st things to look out for
•
•
•
•
#2 High Level tools
•
•
•
•
Processes
Does it work?
Documentation
Architecture
Code Analysis
Code Metrics
#3 Manual Checking
•
•
SOLID Design Principles
Code Review tools
About Adam

Chief Architect at SSW

Developing custom solutions for
businesses across a range of
industries such as Government,
banking, insurance

Microsoft Gold Partner

Microsoft Regional Director

VSTS MVP

@AdamCogan
I Believe
The first things to look out for
Do you evaluate the processes?

Often times this is the source of problems






Are devs getting bogged down in the UI?
Do you have a Scrum Master?
Do you have continuous integration?
Do you have continuous deployment?
Do you have a Schema Master?
Do you have a TFS Master?
http://rules.ssw.com.au/SoftwareDevelopment/RulestobetterArchitectureandCodeReview/Pages/DoYouHaveTheD
esignersFixingUpTheUI.aspx
Are they on the latest version?

…of VS

…of TFS

Alternatively for a ‘Gold Star’ …

Resharper, add-ins, extensions
http://rules.ssw.com.au/SoftwareDevelopment/RulestobetterArchitectureandCodeReview/Pages/CanYouGetLatestAndCompile.aspx
Can you ‘Get latest’ and compile? #L1

It's amazing how often you can't simply do a "Get Latest" and
compile

Add _Instructions_Compile.docx

Then run unit tests…
http://rules.ssw.com.au/SoftwareDevelopment/RulestobetterArchitectureandCodeReview/Pages/CanYouGetLatestAndCompile.aspx
Can you get latest and compile? #L1

See the Integration.Test project fail

Add _Instructions_Compile.docx
http://rules.ssw.com.au/SoftwareDevelopment/RulestobetterArchitectureandCodeReview/Pages/CanYouGetLatestAndCompile.aspx
Can you get latest and compile? #L2

Creation of the database via scripts (incremental)
Tip: use OSQL, SQLCMD or SSW SQL Deploy

and re-run the tests. They will now pass.
Do you want a ‘Gold Star’ ? #L3

Streamline setup of a new development environment

Problems to check for:



Windows 8 not supported
Many things to build
Lots of dependencies

Recommendation: All manual work station setup steps should be
scripted with PowerShell

Recommendation: A get + compile should work within 1 minute,
and work without a dev being 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: You see the problems in the devs environment
Note Prefix eg. _01Setup-Environment.ps1
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 available for 'Azure Storage Service is running'
WARNING: Abandoning remainder of script due to critical failures.
Figure: The script tries to automatically fix the problems
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: Note on 2nd run only 1 script remains – see there is less to fix
Can you ‘Check In’ and Deploy #L1

Add _Instructions_Deploy.docx

Alternatively for a ‘Star’ …
http://rules.ssw.com.au/SoftwareDevelopment/RulestobetterArchitectureandCodeReview/Pages/Default.aspx
Can you ‘Check In’ and Deploy #L2

Use PowerShell scripts as your documentation





build.ps1
deploy_dev.ps1
deploy_test.ps1
deploy_prod.ps1
Alternatively for a ‘Gold Star’ … TFSBuild + Portal
http://rules.ssw.com.au/SoftwareDevelopment/RulestobetterArchitectureandCodeReview/Pages/Default.aspx
Can you ‘Check In’ and Deploy #L2
TFSBuild + Portal
Do you review the Solution and
Project names?

The name of your solution and the names of the project
in your solution should be consistent
http://rules.ssw.com.au/SoftwareDevelopment/RulestobetterArchitectureandCodeReview/Pages/DoYouReviewThe
SolutionName.aspx
Do you review the documentation?

Old School:



Heavy, long documents
Sequence Diagrams
UML
http://rules.ssw.com.au/SoftwareDevelopment/RulestobetterArchitectureandCodeReview/Pages/DoYouReviewThe
Documentation.aspx
Suggestions for doco
eg. Enterprise Architect

Use Red for unimplemented stuff

Use the DateTime shape
To see the last time the diagram was modified and by
whom

Mark items as ‘TODO: Adam’.
For items still pending
Do you review the documentation

New School:

4 docs
•
•
•
•


Business.docx
_Instructions_Compile.docx
_Instructions_Deploy.docx
Technologies.docx
Unit Tests (low level)
Code and Work Items (low level  PBI)
Vote:
High Level Tools
Agenda
•
#1 The 1st things to look out for
•
•
•
•
#2 High Level tools
•
•
•
•
Processes
Does it work?
Documentation
Architecture
Code Analysis
Code Metrics
#3 Manual Checking
•
•
SOLID Design Principles
Code Review tools
Do you look at the architecture?

2 Choices:

VS Dependency Graph or … ?
• Ultimate Edition

nDepend
•
•
•
•
?
3rd Party Tool for a few hundred $
Weak dependency graph
No need for VS Ultimate
Bonus: See problems in the ‘Queries + Rules Explorer’
http://rules.ssw.com.au/SoftwareDevelopment/RulestobetterArchitectureandCodeReview/Pages/DoYouLookAtThe
Architecture.aspx
My Dream
My Dream – instead of this
My Dream – it automatically does this
Great Overview tool and learning tool
Not for day-to-day use as a code analysis tool.
Note: Don’t be distracted by a colour problem 
Do you use the best Code Analysis tools?
http://rules.ssw.com.au/SoftwareDevelopment/RulestobetterArchitectureandCodeReview/Pages/DoYouDoCodeAn
alysis.aspx
Do you use the best Code Analysis
tools?

Level 1 – ReSharper – All Rules

Level 2 - VS Code Analysis (FXCop) – Default Settings

Level 3 - VS Code Analysis (FXCop) – All Rules

Level 4 – StyleCop – All Rules

Level 5 – SSW Code Auditor – All Rules
?
http://rules.ssw.com.au/SoftwareDevelopment/RulestobetterArchitectureandCodeReview/Pages/DoYouDoCodeAn
alysis.aspx
Do you use the best Code Analysis
tools?
http://rules.ssw.com.au/SoftwareDevelopment/RulestobetterArchitectureandCodeReview/Pages/DoYouDoCodeAn
alysis.aspx
Do you use the best Code Analysis
tools?

Level 1 – ReSharper – campsite scout rule

Level 2 - VS Code Analysis (FXCop) – Default Settings

Level 3 - VS Code Analysis (FXCop) – Custom

Level 4 – StyleCop

Level 4 – StyleCop - Custom

Level 5 – SSW Code Auditor

Level 5 – SSW Code Auditor - Custom

TIP: Have a document with rules that you turn off and the reason
http://rules.ssw.com.au/SoftwareDevelopment/RulestobetterArchitectureandCodeReview/Pages/DoYouDoCodeAn
alysis.aspx
ReSharper
Resharper Custom
Code Analysis
Code Analysis Custom
Code Analysis – Suppress #1
Add in ‘Suppression File’ or in code?

?
Code Analysis – Suppress #2
Add in ‘Suppression File’ or in code?

Rule No Good – removed from Rule Set

Rule N/A in this case – in ‘Suppression File’

Rule is Valid – in this case I am overriding it
Code Analysis – Create work item #1
Add as Bug, PBI or Task...
?
Code Analysis – Create work item #2
Neno’s solution = Select 30 in a PBI
Leave as warning
Aussie solution = 1 becomes a PBI
StyleCop
Code Auditor
Rules turned off
Do you check the code coverage?

See if there are unit tests

See if they are any good (~ 80% coverage)
http://rules.ssw.com.au/SoftwareDevelopment/RulestobetterArchitectureandCodeReview/Pages/DoYouLookForCodeCoverage.aspx
Report time?

If you are doing a report, gather some more reports…
Eg. Code Metrics, Code Coverage, Code Clones

But lets assume we are continuing to ‘Probe’… So from:




SRP
…
…
Naming Conventions
Our solution is clean now. What next?
Agenda
•
#1 The 1st things to look out for
•
•
•
•
#2 High Level tools
•
•
•
•
Processes
Does it work?
Documentation
Architecture
Code Analysis
Code Metrics
#3 Manual Checking
•
•
SOLID Design
Principles
Code Review tools
Do you run Code Metrics to find
dodgy code?

Use the “Hot Spots” feature to quickly identify smelly
code

It measures:





Maintainability Index
Cyclomatic Complexity
Depth of Inheritance
Class Coupling
Lines of Code
Code Metrics Results
Manual Checking:
Getting our hands dirty
Manual Review

After using the automated high level tools it’s time to
actually jump into the code

Look for code that doesn’t follow SOLID principles… and
then design patterns

Speak to the devs
SOLID Principles

Single Responsibility Principle

Open Close Principle

Liskov Substitution Principle

Interface Segregation Principle

Dependency Inversion Principle
http://rules.ssw.com.au/SoftwareDevelopment/RulestobetterArchitectureandCodeReview/Pages/DoYouKnowCom
monDesignPrinciples.aspx
Single Responsibility Principle

A class should have one and only one responsibility
public class Address {
// Standard Address Properties
public Image GetGoogleMaps() {}
public decimal GetDistance(Address destination) {}
public bool ValidateAddress() {}
}
Single Responsibility Principle

The Address class has too many responsibilities



Showing an image of the address (tied to UI)
Calculations based on the address
Validation of the address

Another UI may use BingMaps instead of Google Maps

Some, if not all of these functions should be moved to
other classes like MapHelper and AddressHelper
Single Responsibility Principle
public class AddressHelper {
public decimal GetDistance(Address start, Address destination) {}
public bool ValidateAddress(Address address) {}
}
public class GoogleMapProvider : IMapProvider {
public Image GetMap(Address start) {}
}
public class BingMapProvider : IMapProvider {
public Image GetMap(Address start) {}
}
Open Closed Principle

Open for extension, but closed for modification

Use Abstract base classes that specify some base
functionality

E.g. In .NET WebRequest allows you to connect servers
via web protocols



HttpWebRequest
FtpWebRequest
FileWebRequest
Liskov’s Substitution Principle

Subtypes must be substitutable for their base types
public abstract class Duck {
public abstract void Quack();
}
Liskov’s Substitution Principle
public class PekinDuck : Duck {
public void Quack() {}
}
public class BatteryPoweredDuck : Duck{
public BatteryPoweredDuck(Battery energizer) {}
public void Quack() {}
}
Can’t swap PekinDuck with BatteryPoweredDuck because
you need pass it some batteries first
Another example
public class Rectangle {
protected int _width;
protected int _height;
public int Width { get { return _width; }
public int Height { get { return _height; }
public void SetWidth(int width) {
_width = width;
}
public void SetHeight(int height) {
_height = height;
}
}
public class Square : Rectangle {
public override void SetWidth(int width) {
_width = width;
_height = width;
}
public override void SetHeight(int height)
{
_width = height;
_height = height;
}
}
Another Example (cont…)

If we try to use these classes
var shape = new Rectangle();
shape.SetWidth(2);
shape.SetHeight(5);
Console.WriteLine(String.Format(“Area = {0}”, shape.Height
* shape.Width))
// Prints 10 as expected (2 * 5 = 10)
Another Example (cont…)

If we try the Square
var shape = new Square();
shape.SetWidth(2);
shape.SetHeight(5);
Console.WriteLine(String.Format(“Area = {0}”, shape.Height
* shape.Width))
// Prints 25 not as expected since we set the Width = 2 and
Height = 5
Another Example (cont..)
The Square violates the Liskov substitution principle as
we don’t get expected behaviour as setting the width will
also modify the height and vice versa
Interface Segregation Principle

Don’t create interfaces with lots of methods that don’t
necessarily get used in their implementations
public interface IBird {
public void Chirp();
public void Flap();
public void Fly();
public void Eat();
}
public class MockingBird : IBird {
public void Chirp() {}
public void Flap() {}
public void Fly() {}
public void Eat() {}
}
Interface Segregation Principle
public class Emu : IBird {
public void Chirp() {}
public void Flap() {}
public void Fly() {}
public void Eat() {
throw new NotImplementedException();
}
}
Interface Segregation Principle
public interface IBird {
public void Chirp();
public void Flap();
public void Eat();
}
public interface IFlyingBird : IBird {
public void Fly();
}
public class Emu : IBird {
public void Chirp() {}
public void Flap() {}
public void Fly() {}
}
Dependency Inversion Principle
Depend on abstractions, not on implementations. Anything required
to create a valid instance of an object, should have those
dependencies passed in as arguments to the constructor

Higher level classes to depend on abstractions of lower level classes
(swappable)
public class Employee {
public Employee() {
FavCoffee = new Cappuccino();
Skills = new List<Skills> {
new MSAccessSkill(),
new SharePointSkill(),
new DotNetNukeSkill()
}
}
}
Dependency Inversion Principle
public class Employee {
public Employee(IDrinkable iamthirsty, IEnumerable<Skills> skills) {}
}
Dependency Inversion Principle
var EricPhan = new Employee(
var MarkLiu = new Employee(
CoffeeFactory.Get(“Latte”),
CoffeeFactory.Get(“Skim Cappuccino”),
new List<Skills> {
new List<Skills> {
new BusinessIntelligenceSkill(),
new MSAccessSkill(),
new MVC4Skill(),
new SharePointSkill(),
new DotNetSkill()
new DotNetNukeSkill()
});
});
Do you know the common Design
Patterns?

?
“Communicate massive amount of data in a few words”

Adam Cogan
“Accepted solutions to well known problems”

Ben Day
http://rules.ssw.com.au/SoftwareDevelopment/RulestobetterArchitectureandCodeReview/Pages/DoYouKnowCom
monDesignPatterns.aspx
Do you know the common Design
Patterns?

Inversion of Control

Abstract factory

Iterator

Dependency Injection

Adapter

Visitor

Factory

Bridge

State

Singleton

Mediator

Composite

Repository

Proxy

Facade

Unit of Work

Flyweight

Observer

MVC

Chain of responsibility

Decorator

MVP

Command

Null object

MVVM

Memento

Interpreter
http://rules.ssw.com.au/SoftwareDevelopment/RulestobetterArchitectureandCodeReview/Pages/DoYouKnowCom
monDesignPatterns.aspx
AntiPatterns
Abstractness vs Instability Diagram

Shows the assemblies that are painful to maintain
i.e concrete and stable, and
which assemblies are potentially useless
i.e abstract and instable

Abstract: The assembly contains many abstract types (i.e
interfaces and abstract classes) and few concrete types

Stable: The assembly is considered stable if its types are
used by a lot of types of tier assemblies. In this
condition stable means painful to modify.
Do you start reading code?
Comments

Q:\ Good or Bad?

Comments are a smell

Includes comments that explain the intent (the why
rather than the what)
Do you start reading code?

Is clear and easy to read

Has consistent and meaningful names for everything

Has no duplicate or redundant code

Has consistent styles and formatting

Explains "why" when you read down, and "how" when
you read left to right
http://rules.ssw.com.au/SoftwareDevelopment/RulestobetterArchitectureandCodeReview/Pages/DoYouStartReadi
ngCode.aspx
Do you know Code Reviews after
check in are bad?

If you are aiming to get the to nirvana of Continuous
Deployment, then you cant *rely* on code reviews after
the fact.

Code Reviews have different status:
* Important
* Nice to have
The Scenario

Mark is migrating from DotNetNuke to MVC

He’s unsure of his code because he doesn’t know Razor

Add some code with a comment ‘this could be done
better’

Checks in anyway
Use the Code Review tools in TFS
2012

TFS 2012 has built in Code Review tools

Hooks into the check-in/shelving process

This allows code to be manually reviewed before
checking in
Suspend current changes
Request a Review
Assign
Reviewer
Reviewer Gets
the Request
Check out the
changeset
Related work
items
Modified files
Shows the
changed code
Side by Side
(Standard Diff)
#2
#1
Finalizing the Code Review
Check the
Code Review
Check any
comments
View code marked for review
Mark any flagged
code as completed
Mark the Code Review as Complete
Code Review Summary

Today: Developer does some work and wants to get it
checked before committing to source control

Suggestion to MS: Add a new scenario




No touching of code
Architect ‘Adds comments’ during code review
Automatically adds the developers who originally worked
on that section (instead of manually using annotate)
Creates “Code Review” work items
http://www.ssw.com.au/ssw/Standards/BetterSoftwareSuggestions/TeamFoundationServer.aspx
Resources

Thanks: Eric Phan and Adam Stephensen

Thanks: Marcel De Vries and Terje Sandstrom

Google: ssw tv architecture code reviews

http://rules.ssw.com.au/SoftwareDevelopment/Rulestobette
rArchitectureandCodeReview/
Summary
•
#1 The 1st things to look out for
•
•
•
•
#2 High Level tools
•
•
•
•
Processes
Does it work?
Documentation
Architecture
Code Analysis
Code Metrics
#3 Manual Checking
•
•
SOLID Design Principles
Code Review tools
Evaluations + 2 things

Include best tip or tool you heard. Plus your tip to me.

@adamcogan
Or
[email protected]

Come and visit me (+ Sydney .NET User Group)
Thank You!
Sydney | Melbourne | Brisbane | Adelaide
[email protected]
www.ssw.com.au
Delivering Awesome Web Applications
Questions?
Take a business card.
Refactoring

Level 1: Minimum

Level 2: Custom

Note: All disabled ones to be documented
eg. http://www.benday.com/2010/03/15/article-staticmethods-are-a-code-smell/
http://europe.msteched.com
www.microsoft.com/learning
http://microsoft.com/technet
http://microsoft.com/msdn
http://europe.msteched.com/sess
ions