Program for a trading application

I am writing a program for my trading application. Over the months the program has been growing steadily, and what is used to be a small program now I would call a medium size program (about 1000-2000 lines in total), and it will keep growing as I add more features to it. It has grown to a point where it start to get difficult to read my code.

Below is a small part of the code from my over program. It is particularly nasty because I have a lot of calculations involved. What I think the following code is especially hard to read is because there are many similarities in blocks but differ from a plus (+) or minus (-) sign, so I cannot easily chunk it to new methods (refactoring). Should I just group the code into like 20-30 lines or so together and declare a new method? Then I will have many small methods which it is only be called in one place, which I think it defeat the purpose of a new method.

I have been reading extension method or so but I don't know if it is easy to apply to my program.

```
public class CVB
{
public Bar bar = new Bar();

public class Bar
{
public DateTime startTime;
public DateTime endTime;
public double open;
public double high;
public double low;
public double close;
public double bestBid;
public double bestAsk;
public int volume;
public int delta;
public int accVolume;

public DateTime lastPrintTime;
public DateTime lastMarcoTime;

public string writeLine = null;
public List outputString = new List();
public Boolean validBarWrite = false;
}

public Info info = new Info();

public class Info
{
public string symbol = null;
public string exchange = null;
public string finalSymbol = null;
public int CVBSize = 200;
public double tickSize;
public int largeSizeLimit = 50;
public Boolean realtimeChart = false;
public int realtime

You obviously don't do enough encapsulation. I see at least three classes with no behaviour (Bar, Level2 and Info), your "main" class (CVB) contains only static method... it does not seem like good OOP for me (and when we are talking C# we should mostly talk OOP).

Let's try to fix it, one thing at a time.

public static void processLine(ref Output output, ref CVB[] cvb, int c)

Why are output and cvb ref, if it's not assigned anywhere in code? It should not be. Only things that implies assignment is call to writeToString(ref output, ref cvb, c);, which should not modify anything, because it's only writing. You have not included your code, but let's assume that it's not doing any assignments inside too. So we get rid of those refs.

Next thing in here: you're passing in an array of CVBs and some c, most likely an index. But everywhere in method (with exception to calls to writeString) you're using cvb[c]. So we can safely extract it into temporary variable. Let's call it cvbEl, for the lack of best option.

var cvbEl = cvb[c];

Another "safe" extraction is to put your 0.00000001 into a (class-level) constant, which we will name Tolerance, because as far as I can see, it is a floating-point comparison tolerance.

const double Tolerance = 0.00000001;

Now I'll do some strange thing: I'll extract your call to writeString to variable. I do this only because it is the only thing that operates with cvb and c and I can not make any decision about it without seeing it first. So I'll just save entire call for later.

Action writeToStringCall = () => writeToString(ref output, ref cvb, c);

And now, thanks to this, all our code operates on cvbEl. So we can safely move it to CVB instance method, because all this processing is coupled to single instance of CVB. Let's call this method Process (of course, all the code inside it does not use cvbEl, but uses implicit this. Now, all that's left of yours processLine is:

public static void processLine(Output output, CVB[] cvb, int c)
{
    var cvbEl = cvb[c];
    Action writeToStringCall = () => writeToString(ref output, ref cvb, c);
    cvbEl.Process(output, writeToStringCall);
}

In fact, it could be an one-liner if inlined all the variables, but I leave them as is for the sake of readability.

Next thing: you should extract every case in your switch as a separate method. Look at it now:

switch (output.type)
{
    case "Trade":
        Trade(output, writeToStringCall);
        break;
    case "Ask":
        Ask(output);
        break;
    case "BestAsk":
        BestAsk(output);
        break;
    case "Bid":
        Bid(output);
        break;
    case "BestBid":
        BestBid(output);
        break;
    case "Volume":
        Volume(output);
        break;
}

In fact, repeating pattern (for every type of Output call a method passing output inside) clearly calls for some inheritance on Output side: you should have several different Output classes, subtyped after your Output.Type, with method ProcessSmth(CVB), which should have processing related to that type of output. Having that you would replace this whole switch with output.ProcessSmth(this). But I won't do that now, and instead concentrate of contents of there methods (although they are already pretty small and readable, with exception of Trade).

private void Volume(Output output)
{
    bar.accVolume = output.volume;
}

Already nothing to do here.

ask.RemoveAll(x => x.price <= outputPrice + 0.000000001)

This line is repeated three times (although I suppose that third one is a mistake), so we extract it into method, also giving it meaningful name (RemoveAllAsksLessThan). We repeat same operation for remaining RemoveAlls. Let's look what's left of these:

private void BestBid(Output output)
{
    bar.bestBid = output.price;
    RemoveAllBidsGreaterThan(output.price);
    RemoveAllAsksLessThan(output.price);

    if (output.volume > 0)
        bid.Add(new Level2(output.price, output.volume));
}

private void Bid(Output output)
{
    RemoveAllBidsEqualTo(output.price);
    RemoveAllAsksLessThan(output.price);

    if (output.volume > 0)
        bid.Add(new Level2(output.price, output.volume));
}

private void BestAsk(Output output)
{
    bar.bestAsk = output.price;
    RemoveAllBidsGreaterThan(output.price);
    RemoveAllAsksLessThan(output.price);

    if (output.volume > 0)
        ask.Add(new Level2(output.price, output.volume));
}

private void Ask(Output output)
{
    RemoveAllAsksEqualTo(output.price);
    RemoveAllBidsGreaterThan(output.price);

    if (output.volume > 0)
        ask.Add(new Level2(output.price, output.volume));
}

All that's left is Trade. Most of it happens only when bar.bestAsk != 0 || bar.bestBid != 0. Let's invert that if so we don't have to think what happens if both bests are 0 (because we simply return at that, don't leave us hanging!).

StackExchange Code Review Q#14054, answer score: 7