Send mail to the author(s) E-mail

# Tuesday, July 28, 2015

Going through a refactoring to see how Steve does it.

namespace GildedRose.Console { class Program { // not allowed to mess with this private field... camel case? IList<Item> Items; static void Main(string[] args) {

R# highlights: namespace issues, field camelCase, and args not being used..

Extract to a ServiceClass

var service = new ItemQualityService(); service.UpdateQuality(app.Items);

He pulled out to a new file very quickly, and checked in.

namespace GildedRose.Console { public class ItemQualityService { public void UpdateQuality(IList<Item> Items)

foreach loop extracting out

public class ItemQualityService { public void UpdateQuality(IList<Item> Items){ foreach (Item item in Items){ UpdateQuality(item); } } private void UpdateQuality(Item item){

there was loads of stuff inside.  Now extracted out.

Adding tests

install-package fluentassertions

public class ItemQualityService_UpodateItemQualityShould { [Fact] public void ReduceNormalItemQualityByOne() { var qualityService = new ItemQualityService(); var normalItem = new Item{Name = "+5 Dexterity Vest", SellIn = 10, Quality = 20}; qualityService.UpdateItemQuality(normalItem); normalItem.Quality.Should().Be(19); } }

Great to get the code under source control!

Extract static Method

var normalItem = GetNormalItem();

Ctrl Shift R - R#

private static Item GetNormalItem(int sellIn = DEFAULT_STARTING_SELLIN, int quality = DEFAULT_STARTING_QUALITY) { return new Item { Name = "+5 Dexterity Vest", SellIn = sellIn, Quality = quality }; }

Extract Field

public class ItemQualityService_UpdateItemQualityShould { // newing up in a field private ItemQualityService itemQualityService = new ItemQualityService();

Instead of a ctor.

Magic Numbers

[Fact] public void ReduceNormalItemSeelInByOne() { var qualityService = itemQualityService; var normalItem = GetNormalItem(); int startingSellIn = normalItem.SellIn; qualityService.UpdateItemQuality(normalItem); normalItem.SellIn.Should().Be(startingSellIn-1); }

Simple way of not having .Be(19) in the test


private const int SYSTEM_MAX_QUALITY = 50; private const int DEFAULT_STARTING_SELLIN = 10; private const int DEFAULT_STARTING_QUALITY = 20;

SystemMaxQuality is recommended naming convention


// not allowed to touch this public class Item { public string Name { get; set; } public int SellIn { get; set; } public int Quality { get; set; } }

As part of the spec is not to touch this class, we're going to create a wrapper for it.

public class StoreItem { private readonly Item _item; public StoreItem() { } public StoreItem(Item item) { this._item = item; } public string Name { get { return _item.Name; } set { _item.Name = value; } } public int SellIn { get { return _item.SellIn; } set { _item.SellIn = value; } } public int Quality { get { return _item.Quality; } set { _item.Quality = value; } } }

StoreItem wraps Item.  Can work with StoreItems now instead. 2:18 of 7:08
| | # 
# Monday, July 27, 2015

example - a big long aspx page which does lots in the save_Click code behind

  •      Do guarding UI stuff in code behind
  •      Call Service
  •      Update UI based on strongly typed return ie IsSuccessful, and ResultMessage


public partial class ApproveProposal2 : BasePage { // Initialisaing the field at declaration private ArticleProposalApprovalService _approvalService = new ArticleProposalApprovalService(); private void save_Click(object sender, System.EventArgs e) { if (dueDate.Text.Length < 1) { this.errorMessage.Text = "Please enter a due date."; this.errorMessage.Visible = true; return; } var result = _approvalService.Save(articleProposalId, dueDate.Text, sendEmail.Checked); if (result.IsSuccessful) { this.articleProposalInfo.Refresh(); this.errorMessage.Text = "Proposal due date processed."; this.dueDate.Enabled = false; = false; } else { this.errorMessage.Text = result.ResultMessage; } this.errorMessage.Visible = true; } } public class ArticleProposalApprovalService { private AuthorNotificationService _authorNotificationService = new AuthorNotificationService(); public ArticleProposalSaveResult Save(int articleProposalId, string dueDate, bool shouldSendEmail) { var result = new ArticleProposalSaveResult(); try { ArticleProposal proposal = new ArticleProposal(articleProposalId); if (proposal.ProposalText.Length < 1) { result.ResultMessage = "Proposal not found."; return result; } User user = new User(proposal.AuthorId); if (user == null) { result.ResultMessage = "Author " + proposal.AuthorId.ToString() + " not found"; return result; } proposal.DueDate = Convert.ToDateTime(dueDate); if (shouldSendEmail) { _authorNotificationService.NotifyAuthorProposalAccepted(user, proposal); } proposal.Save(); result.IsSuccessful = true; return result; } catch (Exception ex) { WriteError("approveProposal.asax.cs::next_Click : " + ex.ToString()); result.ResultMessage = "An exception occured while saving your information. Please try again later."; return result; } }

Lots of code.  It is better than the original though!

| | # 
# Friday, July 24, 2015

"Like cleaning your room.. good to do often"

Like editing.. first draft, to final version

Over time software rots.. incurs Technical Debt

  • Duplication
  • Excess coupling
  • Quick fixes
  • Hacks

Why Refactor

  • Improves design
  • Improves readability
  • Removes defects
  • ... so helps you program faster (as we'll keep on making changes to the system)

When Refactor

PDD - Pain Driven Development!

Part of code reviews

The Refactoring Process

  • Check in code into source control
  • Verify existing behaviour
  • In no unit tests, write charaterisation tests..
    • ie write a test you know will fail
  • Refactor


  • Small steps
  • Check in often

Simple Refactorings

0.5) Code cleanup eg Ctrl K D to format, Removed Unused usings, remove redundant qualifiers (greyed out in R#), remove dead code

Refactor towards testable code

1.Parameters - make clearer

2.Magic numbers - eg a constant, or a field, Enums??

Organising Code Smells

  • The Bloaters
  • The OO Abusers
  • The Change Preventers
  • The Dispensers
  • The Couplers
  • The Obfuscators
  • Environment Smells
  • Test Smells

The Bloaters

  • The Long Method.. prefer short methods...1 screen (actually Steve McConnell that up to 100 lines is easier to develop) .  > LOC is just an indicator that maybe the function is doing too much
  • Long / Nested loops
    • make easier to understand by extracting to another method

3.Extract methods / into a Service class - make it easier to test.  Not necessarily smaller methods

4.Stuff in right abstraction level.. eg data access, business logic.. caching..

5. Primitive obsession eg use ints, strings.  Use objects to help. eg DrawLine(1,2,3,4,5), DrawLine(startPoint, endPoint, colour).  Email type, FileType (rather than strings)

6. Long parameter lists into a method... why not pass in an object


7.Regions - don't use?

8.Comments..untrustworthy..should be used to say Why.. not What or How.  Hanselman wants context.. links to instructions from code.. ie wiki.. why.. or links to github issues.  Why.. if this is bad, why??

9.Poor names eg GeneratePrimeFactorsOf.. not encodings ie bnThing.. suffix it.  UserNameTextBox, UserNameLabel

| | # 
# Tuesday, July 14, 2015

Pretty good fit for the Observer pattern

IObserver<T> is part of the framework, but no implementations.  They are in ReativeExtensions

SRP as only has 1 reason to change - ie should just react to WarmerPlateStatus values.. ie it just sets the WarmerPlate to a different state.

Dependency Inversion Principle - injecting in.. doesn't depend on details

Very easy to test

Use Rx to poll every second:

  • GetBrewButtonStatus
  • GetBoilderStatus
  • GetWarmerPlaceStatus
| | # 
# Friday, July 10, 2015

Why write better code?

  • Long term productivity
  • Maintainability

Make clearer code

Make it obvious what a method does ie name, return type, Command/Query

CQS - Command Query Separation

If follow CQS can trust the codebase / easier to reason about the code without fully understanding the implementation details.

Commands - has side effects eg Write file to disk.  Returns void.
Query - returns data (safe to invoke... does not affect can have confidence to use)

Fail fast and give good exception messages

Nullable References are Evil

Value types eg ints, decimals, bools are non-nullable by default

Ref types eg Classes are nullable.

  • Never return null.. if need the concept of a value not being present then
    • Read returns a Maybe of string.. invalid states are impossible
// Return value may be there, and may not be // and don't want to return null as it is tainted // want a collection of 0 or 1 strings // IEnumerable<string> is too wide as it could have 10 elements // Maybe<T> can return 0 of 1 <T> public Maybe<string> Read(int id) { var path = this.GetFileName(id); if (!File.Exists(path)) return new Maybe<string>(); var message = File.ReadAllText(path); return new Maybe<string>(message); } public void Tester() { // this method may, or may not return a string Maybe<string> thing = Read(49); var message = thing.DefaultIfEmpty("").Single(); }


Productive, Maintainable, through decomposition

  • Single Responsibility (SRP)
  • Open Closed (OCP)
  • Liskov Substitution (LSP)
  • Interface Segregation (ISP)
  • Dependency Inversion (DIP)

Single Responsibility Principle

  • A class should have only 1 reason to change  (do 1 thing and do it well)
  • Separation of concerns eg caching/logging

Interface Segregation Principle

Favour Role Interfaces - defines very few members.. even 1

over Header Interfaces - extracted..essentially all the methods.. big interfaces

Dependency Inversion

Favour composition over inheritence

Decorator (Russian Doll)

| | # 
# Thursday, July 09, 2015

Cross cutting concerns - Logging, Caching... can be factored away with decorators

Business logic left in the class

Solid is a reaction to design smells


solid is supple, allowing us to change in direction and requirements as we go along

Code Review

Faced with a codebase I still don't understand, my strategy is:

  • Get it compiling!
  • Run unit tests
  • Make new unit tests to step through code
  • Make a diagram of how it all works
  • Start a new project, and just rip out the simplest section (Saving) to understand pattern
  • Explore different parts of code eg SpySink for testing how logging works
| | # 
# Tuesday, July 07, 2015

"High-level modules should not depend on low-level modules.
Both should depend on abstractions"

"Abstractions should not depend upon details
Details should depend upon abstractions"

Favour composition over inheritance

Bertrand Meir - talked about inheritance all the time.  He invented Eiffel, which allows for multiple inheritance, so didn't need interfaces in that language.


A design pattern - a special implementation of an interface.

A handy way of getting rid of implementation detail of what a Save is..

// A Command (returns void) public void Save(int id, string message) { // 4 Commands that take id as an argument //this.log.Saving(id, message); //, message); //this.cache.Save(id, message); //this.log.Saved(id, message); // Calls the compositeWriters save this.writer.Save(id, message); }

it is composed in the ctor of the MessageStore

public MessageStore(DirectoryInfo workingDirectory) { // Fail fast.. so can't have an invalid state of no working directory if (workingDirectory == null) throw new ArgumentNullException("There must be a working directory passed to save to"); if (!Directory.Exists(workingDirectory.FullName)) throw new ArgumentException("The workingDirectory must exist", "workingDirectory"); this.WorkingDirectory = workingDirectory; this.log = new StoreLogger(); var c = new StoreCache(); this.cache = c; var fileStore = new FileStore(workingDirectory); = fileStore; this.fileLocator = new FileLocator(); // Deterministic order this.writer = new CompositeStoreWriter( new LogSavingStoreWriter(), fileStore, c, new LogSavedStoreWriter()); }

And here:

public class CompositeStoreWriter : IStoreWriter { private IStoreWriter[] writers; public CompositeStoreWriter(params IStoreWriter[] writers) { this.writers = writers; } public void Save(int id, string message) { foreach (var w in this.writers) w.Save(id, message); } }

Works well with composing commands - save doesn't return anything

2015-07-07 06.53.48

Decorator (Russian Doll model)

If want to compose queries, decorator is better.

// IStore Writer is the role interface that defines the Save method public class StoreCache : IStoreCache, IStoreWriter { private ConcurrentDictionary<int, Maybe<string>> cache; public StoreCache() { this.cache = new ConcurrentDictionary<int, Maybe<string>>(); } public void Save(int id, string message) { var m = new Maybe<string>(message); // add key, value or update with int, string message this.cache.AddOrUpdate(id, m, (i, s) => m); } // A Function that takes an int and returns a string // the signature of the ConcurrentDictioary GetOrAdd method public Maybe<string> GetOrAdd(int id, Func<int, Maybe<string>> messageFactory) { return this.cache.GetOrAdd(id, messageFactory); } }

Before decorator

// IStore Writer is the role interface that defines the Save method public class StoreCache : IStoreCache, IStoreWriter { private readonly IStoreWriter writer; private ConcurrentDictionary<int, Maybe<string>> cache; public StoreCache(IStoreWriter writer){ this.cache = new ConcurrentDictionary<int, Maybe<string>>(); this.writer = writer; } public void Save(int id, string message) { // If you squint it could be base.Save(id, message) // favouring composition over inheritance this.writer.Save(id, message); var m = new Maybe<string>(message); // add key, value or update with int, string message this.cache.AddOrUpdate(id, m, (i, s) => m); }


// Applied decorator pattern to the StoreCache // so when Save is called on the StoreCache // it actually calls Save on the fileStore.. the 'base' // before, as they both implement IStoreWriter var c = new StoreCache(fileStore); this.cache = c; this.log = new StoreLogger(); = fileStore; this.fileLocator = new FileLocator(); this.writer = new CompositeStoreWriter( new LogSavingStoreWriter(), c, new LogSavedStoreWriter());


The 'Russian Doll' model

| | # 
# Monday, July 06, 2015


"Objects are truly a poor mans closures!"
"Closures are poor mans objects!"

SRP - end up with lots of small classes
ISP - extreme version of role interface with only has 1 method

So have lots of fine grained classes with a single method

Objects are data with behaviour

// An object is data with behaviour public class FileStore : IMessageQuery { // The data of the object private DirectoryInfo workingDirectory; public FileStore(DirectoryInfo workingDirectory) { this.workingDirectory = workingDirectory; } // The behaviour of the object public string Read(int id) { var path = Path.Combine( this.workingDirectory.FullName, id + ".txt"); return File.ReadAllText(path); } } public interface IMessageQuery { string Read(int id); }

Functions are pure behaviour

// Functions are pure behaviour... no data captured // same thing as the object does // takes workingDirectory and id (DirectoryInfo and int) as input // returns string as output Func<DirectoryInfo, int, string> read = (workingDirectory, id) =>{ var path = Path.Combine( workingDirectory.FullName, id + ".txt"); return File.ReadAllText(path); };


// An outer variable var workingDirectory = new DirectoryInfo(Environment.CurrentDirectory); // This function 'closes over' or captures the workingDirectory Func<int, string> read = id => { var path = Path.Combine( workingDirectory.FullName, id + ".txt"); return File.ReadAllText(path); };

However this actually compiles (to IL, but then back to C# as)


So it has data and behaviour.. so closures are behaviour with data

What's more lightweight?

Don't need to come up with a name for the concrete class in a closure or interface.. just need a name for the function

| | # 
# Monday, June 29, 2015

Clients should not be forced to depend on methods they do not use

Favour Role Interfaces - defines very few members.. even 1

over Header Interfaces - extracted..essentially all the methods.. big interfaces

| | # 

Commonly programmers react badly when they see a 'SOLID' codebase - as it has lots of small classes

- same amount of code
- arranged differently

- prefer 100's of classes which are all very small

| | #