1
Answer

Refactoring exercise- Improvements to the below code snippet

Arnold Lans

Arnold Lans
7y
141
1
Hi I am looking for suggestions to how we can improve this code. I am looking for the best possible improvements .
 
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) //fix naming violation
{
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 
Answers (1)
1
Bohdan Stupak

Bohdan Stupak

NA 201 4.4k 7y
First what catches the eye is wrong inheritance. Rule of thumb is that inheritance is used when there is "is-a" relation. small order filter is definitely not large order filter :) Instead you could use the abstract class for a common functionality 
 
  1. public abstract class OrderFilter  
  2.     {  
  3.         private IOrderWriter orderWriter;  
  4.         private List<Order> orders;  
  5.         public OrderFilter(IOrderWriter orderWriter, List<Order> orders)  
  6.         {  
  7.             this.orderWriter = orderWriter;  
  8.             this.orders = orders;  
  9.         }  
  10.   
  11.         protected abstract string FilterSize { get; }  
  12.   
  13.         public void WriteOutFiltrdAndPriceSortedOrders(IOrderWriter writer)  
  14.         {  
  15.             List<Order> filteredOrders = this.FilterOrdersSmallerThan(orders, FilterSize);  
  16.             Enumerable.OrderBy(filteredOrders, o => o.Price);  
  17.             ObservableCollection<Order> observableCollection =  
  18.                 new ObservableCollection<Order>();  
  19.             foreach (Order o in filteredOrders)  
  20.             {  
  21.                 observableCollection.Add(o);  
  22.             }  
  23.             writer.WriteOrders(observableCollection);  
  24.         }  
  25.         protected List<Order> FilterOrdersSmallerThan(List<Order> allOrders, string size)  
  26.         {  
  27.             List<Order> filtered = new List<Order>();  
  28.             for (int i = 0; i <= allOrders.Count; i++)  
  29.             {  
  30.                 int number = orders[i].toNumber(size);  
  31.                 if (allOrders[i].Size <= number)  
  32.                 {  
  33.                     continue;  
  34.                 }  
  35.                 else  
  36.                 {  
  37.                     filtered.Add(orders[i]);  
  38.                 }  
  39.             }  
  40.             return filtered;  
  41.         }  
  42.     }  
  43.     public class LargeOrderFilter : OrderFilter  
  44.     {  
  45.         public LargeOrderFilter(IOrderWriter orderWriter, List<Order> orders) : base(orderWriter, orders)  
  46.         {  
  47.         }  
  48.   
  49.         protected override string FilterSize => "100";  
  50.     }  
  51.   
  52.     public class SmallOrderFilter : OrderFilter  
  53.     {  
  54.         public SmallOrderFilter(IOrderWriter orderWriter, List<Order> orders) : base(orderWriter, orders)  
  55.         {  
  56.         }  
  57.   
  58.         protected override string FilterSize => "10";  
  59.     }  
 Then you would notice that 
 
  1. private IOrderWriter orderWriter;  
 is never used. You can safely remove it. And remove from constructor too.
Then what is noticeable is that FilterSize is clearly of the wrong type. Use int instead 
  
  1. public abstract class OrderFilter  
  2.     {  
  3.         private List<Order> orders;  
  4.         public OrderFilter(List<Order> orders)  
  5.         {  
  6.             this.orders = orders;  
  7.         }  
  8.   
  9.         protected abstract int FilterSize { get; }  
  10.   
  11.         public void WriteOutFiltrdAndPriceSortedOrders(IOrderWriter writer)  
  12.         {  
  13.             List<Order> filteredOrders = this.FilterOrdersSmallerThan(orders, FilterSize);  
  14.             Enumerable.OrderBy(filteredOrders, o => o.Price);  
  15.             ObservableCollection<Order> observableCollection =  
  16.                 new ObservableCollection<Order>();  
  17.             foreach (Order o in filteredOrders)  
  18.             {  
  19.                 observableCollection.Add(o);  
  20.             }  
  21.             writer.WriteOrders(observableCollection);  
  22.         }  
  23.         protected List<Order> FilterOrdersSmallerThan(List<Order> allOrders, int size)  
  24.         {  
  25.             List<Order> filtered = new List<Order>();  
  26.             for (int i = 0; i <= allOrders.Count; i++)  
  27.             {  
  28.                 if (allOrders[i].Size <= size)  
  29.                 {  
  30.                     continue;  
  31.                 }  
  32.                 else  
  33.                 {  
  34.                     filtered.Add(orders[i]);  
  35.                 }  
  36.             }  
  37.             return filtered;  
  38.         }  
  39.     }  
  40.     public class LargeOrderFilter : OrderFilter  
  41.     {  
  42.         public LargeOrderFilter(List<Order> orders) : base(orders)  
  43.         {  
  44.         }  
  45.   
  46.         protected override int FilterSize => 100;  
  47.     }  
  48.   
  49.     public class SmallOrderFilter : OrderFilter  
  50.     {  
  51.         public SmallOrderFilter(List<Order> orders) : base(orders)  
  52.         {  
  53.         }  
  54.   
  55.         protected override int FilterSize => 10;  
  56.     }  
 This allows you to remove Order.toNumber method. In future keep in mind Int32.TryParse method https://msdn.microsoft.com/ru-ru/library/f02979c7(v=vs.110).aspx
 Now you can simplify if statement with inversion
 
  1. protected List<Order> FilterOrdersSmallerThan(List<Order> allOrders, int size)  
  2.         {  
  3.             List<Order> filtered = new List<Order>();  
  4.             for (int i = 0; i <= allOrders.Count; i++)  
  5.             {  
  6.                 if (allOrders[i].Size > size)  
  7.                 {  
  8.                     filtered.Add(orders[i]);  
  9.                 }  
  10.             }  
  11.             return filtered;  
  12.         }  
 Now you could use auto-properties inside Order class
 
  1. public class Order  
  2.     {  
  3.         public double Price { get; set; }  
  4.   
  5.         public int Size { get; set; }  
  6.   
  7.         public string Symbol { get; set; }  
  8.     }  
This one is a pure function. It doesn't work by side-effecting your argument value. Instead, you should assign return value somewhere like this  
  1. filteredOrders = Enumerable.OrderBy(filteredOrders, o => o.Price).ToList();  
 
 And then you could use it as Linq extension method
 
  1. filteredOrders = filteredOrders.OrderBy(o => o.Price).ToList();  
 Yeah and there is a typo in WriteOutFilt[e]r[e]dAndPriceSortedOrders
Accepted
Next Recommended Forum