Refactoring exercise- Improvements to the below code snippet

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 orders;

public LargeOrderFilter(IOrderWriter orderWriter, List orders)

{

filterSize = "100";

this.orderWriter = orderWriter;

this.orders = orders;

}

protected string filterSize;

public void WriteOutFiltrdAndPriceSortedOrders(IOrderWriter writer)

{

List filteredOrders = this.FilterOrdersSmallerThan(orders, filterSize);

Enumerable.OrderBy(filteredOrders, o => o.Price);

ObservableCollection observableCollection =

new ObservableCollection();

foreach (Order o in filteredOrders)

{

observableCollection.Add(o);

}

writer.WriteOrders(observableCollection);

}

protected List FilterOrdersSmallerThan(List allOrders, string size)

{

List filtered = new List();

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 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 orders);

}

public class OrderWriter : IOrderWriter

{

public void WriteOrders(IEnumerable orders)

{

}

public interface IOrderStore

{

List GetOrders();

}

public class OrderStore : IOrderStore

{

public List GetOrders()

{

return new List { new Order {

Price = 10,

Size =1,

Symbol = "TShirt"

}, new Order {

Price = 15,

Size =2,

Symbol = "Sport Goods"

} };

}

Many Thanks

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

public abstract class OrderFilter
{
private IOrderWriter orderWriter;
private List orders;
public OrderFilter(IOrderWriter orderWriter, List orders)
{
this.orderWriter = orderWriter;
this.orders = orders;
}
protected abstract string FilterSize { get; }
public void WriteOutFiltrdAndPriceSortedOrders(IOrderWriter writer)
{
List filteredOrders = this.FilterOrdersSmallerThan(orders, FilterSize);
Enumerable.OrderBy(filteredOrders, o => o.Price);
ObservableCollection observableCollection =
new ObservableCollection();
foreach (Order o in filteredOrders)
{
observableCollection.Add(o);
}
writer.WriteOrders(observableCollection);
}
protected List FilterOrdersSmallerThan(List allOrders, string size)
{
List filtered = new List();
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 LargeOrderFilter : OrderFilter
{
public LargeOrderFilter(IOrderWriter orderWriter, List orders) : base(orderWriter, orders)
{
}
protected override string FilterSize => "100";
}
public class SmallOrderFilter : OrderFilter
{
public SmallOrderFilter(IOrderWriter orderWriter, List orders) : base(orderWriter, orders)
{
}
protected override string FilterSize => "10";
}

Then you would notice that

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

public abstract class OrderFilter
{
private List orders;
public OrderFilter(List orders)
{
this.orders = orders;
}
protected abstract int FilterSize { get; }
public void WriteOutFiltrdAndPriceSortedOrders(IOrderWriter writer)
{
List filteredOrders = this.FilterOrdersSmallerThan(orders, FilterSize);
Enumerable.OrderBy(filteredOrders, o => o.Price);
ObservableCollection observableCollection =
new ObservableCollection();
foreach (Order o in filteredOrders)
{
observableCollection.Add(o);
}
writer.WriteOrders(observableCollection);
}
protected List FilterOrdersSmallerThan(List allOrders, int size)
{
List filtered = new List();
for (int i = 0; i <= allOrders.Count; i++)
{
if (allOrders[i].Size <= size)
{
continue;
}
else
{
filtered.Add(orders[i]);
}
}
return filtered;
}
}
public class LargeOrderFilter : OrderFilter
{
public LargeOrderFilter(List orders) : base(orders)
{
}
protected override int FilterSize => 100;
}
public class SmallOrderFilter : OrderFilter
{
public SmallOrderFilter(List orders) : base(orders)
{
}
protected override int FilterSize => 10;
}

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

protected List FilterOrdersSmallerThan(List allOrders, int size)
{
List filtered = new List();
for (int i = 0; i <= allOrders.Count; i++)
{
if (allOrders[i].Size > size)
{
filtered.Add(orders[i]);
}
}
return filtered;
}

Now you could use auto-properties inside Order class

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

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

filteredOrders = Enumerable.OrderBy(filteredOrders, o => o.Price).ToList();

And then you could use it as Linq extension method

filteredOrders = filteredOrders.OrderBy(o => o.Price).ToList();

Yeah and there is a typo in WriteOutFilt[e]r[e]dAndPriceSortedOrders

1 Reply

Know the answer? Post it — somebody with the same question will find it here.

Bohdan Stupak8y ago
Accepted answer
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
public abstract class OrderFilter
    {
        private IOrderWriter orderWriter;
        private List orders;
        public OrderFilter(IOrderWriter orderWriter, List orders)
        {
            this.orderWriter = orderWriter;
            this.orders = orders;
        }

        protected abstract string FilterSize { get; }

        public void WriteOutFiltrdAndPriceSortedOrders(IOrderWriter writer)
        {
            List filteredOrders = this.FilterOrdersSmallerThan(orders, FilterSize);
            Enumerable.OrderBy(filteredOrders, o => o.Price);
            ObservableCollection observableCollection =
                new ObservableCollection();
            foreach (Order o in filteredOrders)
            {
                observableCollection.Add(o);
            }
            writer.WriteOrders(observableCollection);
        }
        protected List FilterOrdersSmallerThan(List allOrders, string size)
        {
            List filtered = new List();
            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 LargeOrderFilter : OrderFilter
    {
        public LargeOrderFilter(IOrderWriter orderWriter, List orders) : base(orderWriter, orders)
        {
        }

        protected override string FilterSize => "100";
    }

    public class SmallOrderFilter : OrderFilter
    {
        public SmallOrderFilter(IOrderWriter orderWriter, List orders) : base(orderWriter, orders)
        {
        }

        protected override string FilterSize => "10";
    }
Then you would notice that
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
public abstract class OrderFilter
    {
        private List orders;
        public OrderFilter(List orders)
        {
            this.orders = orders;
        }

        protected abstract int FilterSize { get; }

        public void WriteOutFiltrdAndPriceSortedOrders(IOrderWriter writer)
        {
            List filteredOrders = this.FilterOrdersSmallerThan(orders, FilterSize);
            Enumerable.OrderBy(filteredOrders, o => o.Price);
            ObservableCollection observableCollection =
                new ObservableCollection();
            foreach (Order o in filteredOrders)
            {
                observableCollection.Add(o);
            }
            writer.WriteOrders(observableCollection);
        }
        protected List FilterOrdersSmallerThan(List allOrders, int size)
        {
            List filtered = new List();
            for (int i = 0; i <= allOrders.Count; i++)
            {
                if (allOrders[i].Size <= size)
                {
                    continue;
                }
                else
                {
                    filtered.Add(orders[i]);
                }
            }
            return filtered;
        }
    }
    public class LargeOrderFilter : OrderFilter
    {
        public LargeOrderFilter(List orders) : base(orders)
        {
        }

        protected override int FilterSize => 100;
    }

    public class SmallOrderFilter : OrderFilter
    {
        public SmallOrderFilter(List orders) : base(orders)
        {
        }

        protected override int FilterSize => 10;
    }
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
protected List FilterOrdersSmallerThan(List allOrders, int size)
        {
            List filtered = new List();
            for (int i = 0; i <= allOrders.Count; i++)
            {
                if (allOrders[i].Size > size)
                {
                    filtered.Add(orders[i]);
                }
            }
            return filtered;
        }
Now you could use auto-properties inside Order class
public class Order
    {
        public double Price { get; set; }

        public int Size { get; set; }

        public string Symbol { get; set; }
    }
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
filteredOrders = Enumerable.OrderBy(filteredOrders, o => o.Price).ToList();
And then you could use it as Linq extension method
filteredOrders = filteredOrders.OrderBy(o => o.Price).ToList();
Yeah and there is a typo in WriteOutFilt[e]r[e]dAndPriceSortedOrders