locked
Refactoring - Code Review suggestions for practice. RRS feed

  • Question

  • User-1625163388 posted

    Hi Can some one suggest me the Refactoring for the below code as I am looking for how to approach to fix this bad code as an exercise.  I have highlighted the ones i think we should refactor.

    public class OrderManager
        {
            private readonly IOrderStore orderStore;
    
            public OrderManager(IOrderStore orderStore)
            {
                this.orderStore = orderStore;
            }
    
            public void WriteOutSmallOrders()
            {
                var orders = orderStore.GetOrders();
                SmallOrderFilter filter = new SmallOrderFilter(new OrderWriter(), orders);
                filter.WriteOutFiltrdAndPriceSortedOrders(new OrderWriter());
            }
    
            public void WriteOutLargeOrders()
            {
                var orders = orderStore.GetOrders();
                LargeOrderFilter filter = new LargeOrderFilter(new OrderWriter(), orders);
                filter.WriteOutFiltrdAndPriceSortedOrders(new OrderWriter());
            }
        }
    
    
        public class LargeOrderFilter
        {
            private IOrderWriter orderWriter;
            private List<Order> orders;
    
            public LargeOrderFilter(IOrderWriter orderWriter, List<Order> orders)
            {
                filterSize = "100";
                this.orderWriter = orderWriter;
                this.orders = orders;
            }
    
            protected string filterSize;
    
            public void WriteOutFiltrdAndPriceSortedOrders(IOrderWriter writer)
            {
                List<Order> filteredOrders = this.FilterOrdersSmallerThan(orders, filterSize);
                Enumerable.OrderBy(filteredOrders, o => o.Price);
    
                ObservableCollection<Order> observableCollection =
                    new ObservableCollection<Order>();
    
                foreach (Order o in filteredOrders)
                {
                    observableCollection.Add(o);
                }
    
                writer.WriteOrders(observableCollection);
            }
    
            protected List<Order> FilterOrdersSmallerThan(List<Order> allOrders, string size)
            {
                List<Order> filtered = new List<Order>();
                for (int i = 0; i <= allOrders.Count; i++)
                {
                    int number = orders[i].toNumber(size);
    
                    if (allOrders[i].Size <= number)
                    {
                        continue;
                    }
                    else
                    {
                        filtered.Add(orders[i]);
                    }
                }
    
                return filtered;
            }
        }
    
        public class SmallOrderFilter : LargeOrderFilter
        {
            public SmallOrderFilter(IOrderWriter orderWriter, List<Order> orders)
                : base(orderWriter, orders)
            {
                filterSize = "10";
            }
        }
    
    
        public class Order
        {
            public double Price
            {
                get { return this.dPrice; }
                set { this.dPrice = value; }
            }
    
            public int Size
            {
                get { return this.iSize; }
                set { this.iSize = value; }
            }
    
            public string Symbol
            {
                get { return this.sSymbol; }
                set { this.sSymbol = value; }
            }
    
            private double dPrice;
            private int iSize;
            private string sSymbol;
    
            public int toNumber(String Input)
            {
                bool canBeConverted = false;
                int n = 0;
                try
                {
                    n = Convert.ToInt32(Input);
                    if (n != 0) canBeConverted = true;
                }
                catch (Exception ex)
                {
                }
    
                if (canBeConverted == true)
                {
                    return n;
                }
                else
                {
                    return 0;
                }
            }
        }
    
    
        // These are stub interfaces that already exist in the system
        // They're out of scope of the code review
        public interface IOrderWriter
        {
            void WriteOrders(IEnumerable<Order> orders);
        }
    
        public class OrderWriter : IOrderWriter
        {
            public void WriteOrders(IEnumerable<Order> orders)
            {
            }
        }
    
        public interface IOrderStore
        {
            List<Order> GetOrders();
        }
    
    
        public class OrderStore : IOrderStore
        {
            public List<Order> GetOrders()
            {
                return new List<Order> { new Order {
                    Price = 10,
                    Size =1,
                    Symbol = "TShirt"
                }, new Order {
                    Price = 15,
                    Size =2,
                    Symbol = "Sport Goods"
                } };
            }
        }
    

    Many Thanks

    Monday, January 22, 2018 8:50 PM

Answers

  • User303363814 posted

    How do you intend to use this code?  That is, what do the callers look like?  Having an OrderManager is a bit of a code smell.

    The Order class.  The toNumber method has nothing to do with an Order and should not be in the Order class.  .Net already provides the Int.Parse() methods anyway, don't reinvent the wheel.  The properties in the Order class are more complicated than they should be.  All that "property gets and set private member" stuff went out in the dark ages.  Simple == easy to maintain.  Put the Order class on a diet

    public class Order {
        public double Price {get; set;}
        public int Size {get; set;}
        public string Symbol {get; set;}
    }

    Easy to write, easy to read, easy to maintain

    Interfaces

    Interfaces are great if there is more than one class implementing the interface or you want to have code which is 'agnostic' about the particular implementation of an interface.  (They also make testing easier, you can provide 'test' versions of classes to exercise your code)  This is not the case with IOrderWriter.  Each time you want to use the IOrderWriter interface you explicitly create a new OrderWriter.  Get rid of the interface and you lose nothing except some code noise.

    OrderManager

    Make it simpler.  Why do you have WriteOutSmallOrders and WriteOutLargeOrders as methods?  (Also, the name is bad.  It doesn't just write out orders it also fetches them) The calling code must choose which method it calls - so all you need is a single method with a parameter that specifies which order size to fetch and write out.  (Note that I have bigger design concerns in that you have 'hidden' in the method called WriteOut**Orders a call to 'Get' orders.  Are you sure it is OK to 'Get' order and then ignore them if the caller has invoked the 'Small' version?  Smells bad)

    publc class OrderManager
    {
       const int LARGE_ORDER_SIZE = 100;
       const int SMALL_ORDER_SIZE = 10;
    readonly IOrderStore orderStore;
    public OrderManager(IOrderStore orderStore) \\ This is good for testing { orderStore = orderStore; } public void GetAndWriteOrders(bool largeOrders) { int orderSize = largeOrders ? LARGE_ORDER_SIZE : SMALL_ORDER_SIZE; var orders = orderStore.GetOrders(); var writer = new OrderWriter(); writer.WriteOrders(orders.Where(o => o.Size <= orderSize) .OrderBy(o => o.Size)); } }

    You don't need the filter classes at all.  You can change the parameter to GetAndWriteOrders if you like.  It could be the actual maximum size that the caller wants to show or it could be an enum which lets them choose between more than two options.  For the moment you only have two possibilities - Large and Small - so a bool is a simple solution.  (By the way - what if the caller wants to write ALL the orders?  At the moment they can only do size less than 100)

    I realise that this does not exactly match the concept of re-factoring.  There is a little bit of redesign.  The reason for this is that the code, overall, is not really object oriented.  It uses the words of object oriented programming but it is deeply rooted in a procedural mindset.

    Overall, I would replace the entirety of the code you presented with the new Order class I have already shown (3 lines of code), the new, simplified OrderManager with a single method.  Keep OrderWriter (but get rid of the interface).  Keep IOrderStore and the implementation OrderStore.

    • Marked as answer by Anonymous Thursday, October 7, 2021 12:00 AM
    Monday, January 22, 2018 10:17 PM

All replies

  • User303363814 posted

    How do you intend to use this code?  That is, what do the callers look like?  Having an OrderManager is a bit of a code smell.

    The Order class.  The toNumber method has nothing to do with an Order and should not be in the Order class.  .Net already provides the Int.Parse() methods anyway, don't reinvent the wheel.  The properties in the Order class are more complicated than they should be.  All that "property gets and set private member" stuff went out in the dark ages.  Simple == easy to maintain.  Put the Order class on a diet

    public class Order {
        public double Price {get; set;}
        public int Size {get; set;}
        public string Symbol {get; set;}
    }

    Easy to write, easy to read, easy to maintain

    Interfaces

    Interfaces are great if there is more than one class implementing the interface or you want to have code which is 'agnostic' about the particular implementation of an interface.  (They also make testing easier, you can provide 'test' versions of classes to exercise your code)  This is not the case with IOrderWriter.  Each time you want to use the IOrderWriter interface you explicitly create a new OrderWriter.  Get rid of the interface and you lose nothing except some code noise.

    OrderManager

    Make it simpler.  Why do you have WriteOutSmallOrders and WriteOutLargeOrders as methods?  (Also, the name is bad.  It doesn't just write out orders it also fetches them) The calling code must choose which method it calls - so all you need is a single method with a parameter that specifies which order size to fetch and write out.  (Note that I have bigger design concerns in that you have 'hidden' in the method called WriteOut**Orders a call to 'Get' orders.  Are you sure it is OK to 'Get' order and then ignore them if the caller has invoked the 'Small' version?  Smells bad)

    publc class OrderManager
    {
       const int LARGE_ORDER_SIZE = 100;
       const int SMALL_ORDER_SIZE = 10;
    readonly IOrderStore orderStore;
    public OrderManager(IOrderStore orderStore) \\ This is good for testing { orderStore = orderStore; } public void GetAndWriteOrders(bool largeOrders) { int orderSize = largeOrders ? LARGE_ORDER_SIZE : SMALL_ORDER_SIZE; var orders = orderStore.GetOrders(); var writer = new OrderWriter(); writer.WriteOrders(orders.Where(o => o.Size <= orderSize) .OrderBy(o => o.Size)); } }

    You don't need the filter classes at all.  You can change the parameter to GetAndWriteOrders if you like.  It could be the actual maximum size that the caller wants to show or it could be an enum which lets them choose between more than two options.  For the moment you only have two possibilities - Large and Small - so a bool is a simple solution.  (By the way - what if the caller wants to write ALL the orders?  At the moment they can only do size less than 100)

    I realise that this does not exactly match the concept of re-factoring.  There is a little bit of redesign.  The reason for this is that the code, overall, is not really object oriented.  It uses the words of object oriented programming but it is deeply rooted in a procedural mindset.

    Overall, I would replace the entirety of the code you presented with the new Order class I have already shown (3 lines of code), the new, simplified OrderManager with a single method.  Keep OrderWriter (but get rid of the interface).  Keep IOrderStore and the implementation OrderStore.

    • Marked as answer by Anonymous Thursday, October 7, 2021 12:00 AM
    Monday, January 22, 2018 10:17 PM
  • User-1625163388 posted

    Hi Paul

    Thanks for the valuable input. This code is intended to discuss various ways in refactoring the bad code to improve the refactoring techniques as a case study.

    we basically trying to see what code we can abstract and how we can apply SOLID principles as they are one of the basic concepts we trying to discuss and when do we need to use Interfaces vs Abstract with the given piece of code.

    at end of session we aim to create a clean code.

    You mentioned what if the caller wants to return ALL the orders? How are we going to achieve with the current implementations. Also the Constants we used are they any good as if we had to make a change in future this requires a change to the method.

    Thanks

    Monday, January 22, 2018 10:39 PM
  • User303363814 posted

    Do you think the code I provided is easier to understand?  That is the bottom line.  Can you fellow workers understand and maintain the code?  If your team do not understand the code then it is BAD code - whether it follows SOLID principles or not.

    I don't know what the restrictions are on the calling code but I would probably just let the caller specify the maximum size as a parameter.  Maybe with a 'special' value of zero which means 'no limit'.

    public void GetAndWriteOrders(int maxSize = 0)
    {
          var orders = orderStore.GetOrders();
          var writer = new OrderWriter();
    
          writer.WriteOrders(orders.Where(o => o.Size <= maxSize || maxSize <=0)
                                   .OrderBy(o => o.Size));
    }
    

    If you don't pass any value to this method then you get all orders, otherwise the maxSize specified is obeyed.

    It is up to you.  What sort of interface do you want to provide to your callers?  There is no correct, best, or even SOLID answer.  Code is only correct if it correctly implements the specification set down for it.

    Tuesday, January 23, 2018 7:34 AM