Search

Categories

 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 

Send mail to the author(s) E-mail

# Thursday, 30 July 2015

Make the code better?  Or write tests to understand problem, and rewrite the code.

  • Enterprise environment in mind
  • Simplicity is key
  • Code will last for many many years
  • High churn rate of overseas developers
  • Favour pragmatism
  • Favour delivery of code

https://github.com/NotMyself/GildedRose

Solution builds in VS2013, single test runs fine with xunit and R# test runner.  Got under git source control

The Problem

  • All items have a SellIn value which denotes the number of days we have to sell the item
  • All items have a Quality value which denotes how valuable the item is
  • At the end of each day our system lowers both values for every item

    Strategy

  • Built tests, and refactored as much simple stuff as possible with tools

    Core method is UpdateItemQuality (which I'd like to break apart using CQS ie a command or query, however this kata is about the logic)

    Problem with UpdateItemQuality

    Lots of nested if's... tricky to understand all the logic.

    Try breaking apart core method into

    • Dexterity (normal)
    • AgedBrie (gets better with time)
    • Sulfuras
    • BackStagePasses

    Will it work putting each bit of logic in a separate method?  Yes it did work quite well.

    public Item UpdateItemQuality(Item item) { if (item.Name == "+5 Dexterity Vest") item = UpdateItemAsDexterity(item); if (item.Name == "Aged Brie") item = UpdateItemAsAgedBrie(item); if (item.Name == "Sulfuras, Hand of Ragnaros") item = UpdateItemAsSulfuras(item); if (item.Name == "Backstage passes to a TAFKAL80ETC concert") item = UpdateItemAsBackStage(item); if (item.Name == "Conjured Item") item = UpdateItemAsConjured(item); return item; } private Item UpdateItemAsDexterity(Item item) { item.SellIn -= 1; if (item.SellIn > 0) item.Quality -= 1; else item.Quality -= 2; if (item.Quality < 0) item.Quality = 0; return item; }

    Enums for item names?  Curly brackets would usually use in if's.

    Google around for other ideas

    | | # 
    # Tuesday, 28 July 2015

    https://github.com/emilybache/GildedRose-Refactoring-Kata

    https://github.com/NotMyself/GildedRose

    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

    Constants

    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

    Behaviour into StoreItem

    // 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, or delegates to it.  Can work with StoreItems now instead.  Putting the UpdateQuality method on StoreItem
    private static StoreItem GetNormalItem(int sellIn = DEFAULT_STARTING_SELLIN, int quality = DEFAULT_STARTING_QUALITY) { return new StoreItem( new Item { Name = "+5 Dexterity Vest", SellIn = sellIn, Quality = quality } ); }

    then a test:

    Long Method Smell, Conditional Complexity Smell

    public void UpdateQuality() { if (this.Name != "Aged Brie" && this.Name != "Backstage passes to a TAFKAL80ETC concert") { if (this.Quality > 0) { if (this.Name != "Sulfuras, Hand of Ragnaros") { this.Quality = this.Quality - 1; } } } else { if (this.Quality < 50) { this.Quality = this.Quality + 1; if (this.Name == "Backstage passes to a TAFKAL80ETC concert") { if (this.SellIn < 11) { if (this.Quality < 50) { this.Quality = this.Quality + 1; } } if (this.SellIn < 6) { if (this.Quality < 50) { this.Quality = this.Quality + 1; } } } } } if (this.Name != "Sulfuras, Hand of Ragnaros") { this.SellIn = this.SellIn - 1; } if (this.SellIn < 0) { if (this.Name != "Aged Brie") { if (this.Name != "Backstage passes to a TAFKAL80ETC concert") { if (this.Quality > 0) { if (this.Name != "Sulfuras, Hand of Ragnaros") { this.Quality = this.Quality - 1; } } } else { this.Quality = this.Quality - this.Quality; } } else { if (this.Quality < 50) { this.Quality = this.Quality + 1; } } } }

    Plan is to head towards UpdateQuality be a delegating method to different Strategies.

    image

    Summary

    After exploring the code, and where it went:

    • Didn't like the complexity
      • Didn't like the number of UpdateQuality methods everywhere
      • Didn't like passing this (StoreItem) into a Strategy..
    • Liked the tests
    | | # 
    # Monday, 27 July 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

    image

    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; this.save.Enabled = 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, 24 July 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

    Tips

    • 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

    Obfuscators

    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

    | | # 
    # Monday, 15 August 2011

    Act of changing internal implementation of a class or method with aim of making it more readable and maintainable.

    YAGNI – You aren’t going to need it

    OOP Principles

    • Encapsulation
    • Inheritance.. Is a
    • Polymorphism – more than 1 form.. in C# we use Interfaces

    Is a.. Has a..  

    Favour Composition over inheritance

    SOLID Principles

    Single Responsibility Principle – method should do only 1 thing.

    Open / Closed Principle – open for extension, closed for modification.  Closely related to Inheritance.  eg Base class which is closed for modification, but can be extended by overriding.

    Liskov Substitution Principle – Design by contract.

    Interface Segregation Principle

    Dependency Inversion Principle

    Code Smells (Antipatterns)

    Duplicate Code / Similar Classes eg doing same calculation in different methods

    Big Classes / Methods and regions in classes bad too.. rather have many smaller classes

    Comments – dont need.  Well written code should be easy to follow.  Use source control to see who did what.

    Bad Names

    Feature Envy

    Too Much If/Switch

    Try/Catch Bloat

    Typical Refactoring

    Extracting Classes or Interfaces

    Code Snippet
        public class Invoice {
        }

        public class Customer {
        }

        public class InvoiceService {
            public string CreateInvoice(Invoice invoice) {
                return "a";
            }
            public string ProcessPayment(Invoice invoice, Double amount) {
                return "a";
            }
            public double GetAmountOwed(Invoice invoice) {
                return 1;
            }
            public double GetTotalAmountInvoicedLastFY(Customer customer) {
                return 1;
            }
            public double GetTotalAmountPaidLastFY(Customer customer) {
                return 1;
            }
        }

    Extracted out interfaces, so implementation only gets to see methods it should do:

    Code Snippet
    public class Invoice {
        }

        public class Customer {
        }

        public interface IInvoiceCreatingService {
            string CreateInvoice(Invoice invoice);
        }

        public interface IInvoicePaymentService {
            string ProcessPayment(Invoice invoice, Double amount);
            double GetAmountOwed(Invoice invoice);
        }

        public interface IInvoiceReportingService {
            double GetTotalAmountInvoicedLastFY(Customer customer);
            double GetTotalAmountPaidLastFY(Customer customer);
        }

        public class InvoiceService : IInvoiceCreatingService, IInvoicePaymentService, IInvoiceReportingService {
            public string CreateInvoice(Invoice invoice) {
                return "a";
            }
            public string ProcessPayment(Invoice invoice, Double amount) {
                return "a";
            }
            public double GetAmountOwed(Invoice invoice) {
                return 1;
            }
            public double GetTotalAmountInvoicedLastFY(Customer customer) {
                return 1;
            }
            public double GetTotalAmountPaidLastFY(Customer customer) {
                return 1;
            }
        }

        public class TestInvoicePaymentClass {
            public string DoinvoicePayments() {
                IInvoicePaymentService invoiceService = new InvoiceService();
                invoiceService.ProcessPayment(new Invoice(), 1);
                return "a";
            }
        }

    Extract Methods

    Some code to refactor:

    Code Snippet
    public class CustomerAddress {
            public string State;
        }
        public class Invoice {
            public string InvoiceNumber;
            public double TotalPrice;
            public int Quantity;
            public string CustomerName;
            public CustomerAddress CustomerAddress;
            public string CustomerBillingInformation;

            public bool Approved { get; set; }
        }
        public class Customer {
            public string CustomerName;
            public CustomerAddress CustomerAddress;
            public string CustomerBillingInformation;
        }

        public class CustomerService {
            public Customer GetCustomer(string customerNumber) {
                return new Customer();
            }
        }

        public class PaymentProcessingService {
            public string ProcessPayment(double totalPrice, string customerBillingInformation) {
                var paymentAuthorizationCode = "a";
                return paymentAuthorizationCode;
            }
        }

        public class InvoiceService {
            public void Post(Invoice invoice) {
            }


            public class WidgetService {
                private const double PricePerWidget = 1.5;
                CustomerService _customerService = new CustomerService();
                PaymentProcessingService _paymentProcessingService = new PaymentProcessingService();
                InvoiceService _invoiceService = new InvoiceService();

                public string PlaceOrderForWidgets(int quantity, string customerNumber) {
                    var invoice = new Invoice {
                        InvoiceNumber = Guid.NewGuid().ToString(),
                        TotalPrice = PricePerWidget * quantity,
                        Quantity = quantity
                    };

                    var customer = _customerService.GetCustomer(customerNumber);
                    invoice.CustomerName = customer.CustomerName;
                    invoice.CustomerAddress = customer.CustomerAddress;
                    invoice.CustomerBillingInformation = customer.CustomerBillingInformation;

                    double tax;
                    switch (invoice.CustomerAddress.State.ToUpper()) {
                        case "OH":
                            tax = invoice.TotalPrice * .15;
                            break;
                        case "MI":
                            tax = invoice.TotalPrice * .22;
                            break;
                        case "NV":
                            tax = invoice.TotalPrice * .05;
                            break;
                        default:
                            tax = 0.0;
                            break;
                    }

                    var shippingPrice = invoice.TotalPrice * .1;
                    invoice.TotalPrice += shippingPrice;
                    invoice.TotalPrice += tax;

                    var paymentAuthorizationCode = _paymentProcessingService.ProcessPayment(invoice.TotalPrice,
                                                                                            customer.CustomerBillingInformation);
                    invoice.Approved = !string.IsNullOrEmpty(paymentAuthorizationCode);
                    _invoiceService.Post(invoice);
                    return "a";
                }
            }


        }

    This does 5 different tasks:

    • creates an invoice
    • associates the invoice with a customer
    • determines tax based on the customers state
    • determines shipping costs
    • authorizes payment

    So 5 different reasons this method may have to change (breaks SRP)

    Rename Variables, Fields, Methods and Classes

    Properties and Fields are information an object contains.  Can have get and set procedures on them

    Code Snippet
    public class SampleClass {
            public int SampleProperty { get; private set; } // property, field or public member variable.. auto implemented property
            int _privateVariable; // class level private and protected variable

            public int sampleMethod(string sampleParam) {
                string procedureLevelVariable = "";
                return 1;
            }
        }
    | | #