2
\$\begingroup\$

I have the method below, which generates backtesting results. However, it's highly unreadable. What could you possibly suggest to me in order to make it more readable?

public DataTable GenerateResultTable(List<BacktestResult> data)
{
    var table = new DataTable();

    table.Columns.Add("Pair");
    table.Columns.Add("Buys");
    table.Columns.Add("Avg Profit %");
    table.Columns.Add("Cum Profit %");
    table.Columns.Add($"Tot Profit {_backtestOptions.StakeCurrency}");
    table.Columns.Add("Tot Profit %");
    table.Columns.Add("Avg Duration");
    table.Columns.Add("Wins");
    table.Columns.Add("Draws");
    table.Columns.Add("Losses");

    // the number of the pairs in the backtest results
    var maxOpenOrders = data.GroupBy(e => e.Pair).Count();
    
    // Split results for each pair
    var query = data.GroupBy(e => e.Pair).Select(e => new { Pair = e.Key, Count = e.Count(), Value = e });

    // in case there are no records to show
    // if there are no rows, it throws an exception..
    if (!query.Any())
    {
        table.Rows.Add("", "", "", "", "", "", "", "", "", "");
    }

    foreach (var result in query)
    {
        var _profitSum = result.Value.Sum(e => e.ProfitPercentage);
        var _profitTotal = _profitSum / maxOpenOrders;

        var key = result.Pair;
        var trades = result.Count;
        var profitMean = trades > 0 ? result.Value.Average(e => e.ProfitPercentage) : 0;
        var profitMeanPercentage = trades > 0 ? result.Value.Average(e => e.ProfitPercentage) * 100 : 0;
        var profitSum = _profitSum;
        var profitSumPercentage = _profitSum * 100;
        var profitTotalAbs = result.Value.Sum(e => e.ProfitAbs);
        var profitTotal = _profitTotal;
        var profitTotalPercentage = _profitTotal * 100;
        var avgDuration = trades > 0 ? Math.Round(result.Value.Average(e => e.TradeDuration)) : 0;
        var wins = result.Value.Count(e => e.ProfitAbs > 0);
        var draws = result.Value.Count(e => e.ProfitAbs == 0);
        var losses = result.Value.Count(e => e.ProfitAbs < 0);

        table.Rows.Add(key, trades, $"{profitMeanPercentage:f2}", $"{profitSumPercentage:f2}", $"{profitTotalAbs:f8}", $"{profitTotalPercentage:f2}",
            $"{TimeSpan.FromMinutes((double)avgDuration):h\\:mm\\:ss}", wins, draws, losses);
    }

    return table;
}

public class BacktestOptions
{
    public string StakeCurrency { get; set; }
    public List<string> Pairs { get; set; }
    public string StrategyName { get; set; }
    public decimal StakeAmount { get; set; }
    public decimal OpenFee { get; set; }
    public decimal CloseFee { get; set; }
}

public class BacktestResult
{
    public string Pair { get; set; }
    public decimal ProfitPercentage { get; set; }
    public decimal ProfitAbs { get; set; }
    public decimal OpenRate { get; set; }
    public decimal CloseRate { get; set; }
    public DateTime OpenDate { get; set; }
    public DateTime CloseDate { get; set; }
    public decimal OpenFee { get; set; }
    public decimal CloseFee { get; set; }
    public decimal Amount { get; set; }
    public decimal TradeDuration { get; set; }
    public SellType SellReason { get; set; }
}

appsettings.json

{
  "BacktestConfiguration": {
    "StakeCurrency": "USDT",
    "Pairs": [ "TRXUSDT" ],
    "StrategyName": "RsiStrategy",
    "StakeAmount": 1000,
    "OpenFee": 0.001,
    "CloseFee": 0.001
  }
}
\$\endgroup\$
4
  • \$\begingroup\$ Please share with use the code of the BacktestResult class and the declaration of _backtestOptions \$\endgroup\$ Feb 5 '21 at 10:22
  • \$\begingroup\$ @PeterCsala, added them both to the question. _backtestOptions is basically BacktestOptions retrieved from appsettings.json. \$\endgroup\$
    – nop
    Feb 5 '21 at 10:33
  • \$\begingroup\$ Why do you generate a DataTable? \$\endgroup\$
    – BCdotWEB
    Feb 5 '21 at 11:33
  • \$\begingroup\$ @BCdotWEB, github.com/minhhungit/ConsoleTableExt \$\endgroup\$
    – nop
    Feb 5 '21 at 11:43
2
\$\begingroup\$

Here are my observations:

  • List<BacktestResult> data: Please try to use more expressive names than data. It is way too general and it does not help the reader / maintainer of the code. (same apply to table and query)

  • Populating columns: Your implementation has a lot of repetition in it: tables.Columns.Add.

    • One way to reduce that by using the AddRange:
var table = new DataTable();
table.Columns.AddRange(new []
{
    new DataColumn("Pair"), 
    new DataColumn("Buys"), 
    new DataColumn("Avg Profit %"), 
    new DataColumn("Cum Profit %"), 
    new DataColumn($"Tot Profit {_backtestOptions.StakeCurrency}"), 
    new DataColumn("Tot Profit %"), 
    new DataColumn("Avg Duration"), 
    new DataColumn("Wins"), 
    new DataColumn("Draws"), 
    new DataColumn("Losses"), 
});

We are almost there, here we have repeated the new DataColumn. With the following technique you can separate data and logic:

var columns = new[]
{
    "Pair", "Buys", "Avg Profit %", "Cum Profit %", $"Tot Profit {_backtestOptions.StakeCurrency}",
    "Tot Profit %", "Avg Duration", "Wins", "Draws", "Losses"
};

table.Columns.AddRange(columns.Select(name => new DataColumn(name)).ToArray());
  • data.GroupBy(e => e.Pair): This has been called twice, but unnecessarily.
    • Materialize it by calling the ToList or ToArray on it:
    • With this approach we could get rid of the maxOpenOrders variable.
var resultsGroupByPair = data.GroupBy(e => e.Pair).ToArray();
var query = resultsGroupByPair.Select(br => new { Pair = br.Key, Count = br.Count(), Value = br });
//...
var _profitTotal = _profitSum / resultsGroupByPair.Length; 
  • query: Same applies here, Possible multiple enumeration
    • Materialize it by calling the ToList or ToArray on it
  • if there are no rows, it throws an exception..: This comment is not align with the code. Try to comment the Whys and Why nots, not the what and how. The latter two should be clear by the code itself, but the chosen strategy (and the excluded alternatives) might not be obvious from the code.
  • profitMean, profitSum, profitTotal: They are unused. If they are not needed then delete them.
  • trades > 0 ?: This condition has been repeated several times.
    • Introduce a local variable and refer to that one instead: bool isTradesPositive
  • result.Value.Average, result.Value.Sum: Same as in the previous case. If you use the same expression multiple times then save its result into a variable.
  • wins, draws, losses: Instead do 3 iterations you can do it with a simple one.
    • For example with Aggregate:
var (wins, losses, draws) = result.Value.Aggregate((0, 0, 0), (outcome, e) =>
{
    if (e.ProfitAbs > 0) outcome.Item1++;
    else if (e.ProfitAbs < 0) outcome.Item2++;
    else outcome.Item3++;
    return outcome;
});

or

var (wins, losses, draws) = result.Value.Aggregate((0, 0, 0), 
    ((int winCount, int loseCount, int drawCount) outcome, BacktestResult e) =>
    {
        if (e.ProfitAbs > 0) outcome.winCount++;
        else if (e.ProfitAbs < 0) outcome.loseCount++;
        else outcome.drawCount++;
        return outcome;
    });
\$\endgroup\$
8
  • \$\begingroup\$ Thanks for your comprehensive answer! The second example with win, losses, draws doesn't work, because of ((int winCount, int loseCount, int drawCount) outcome, BacktestResult e). \$\endgroup\$
    – nop
    Feb 5 '21 at 12:01
  • \$\begingroup\$ @nop Could you please elaborate what do you mean by doesn't work? Does not compile? Does throw exception? Returns with three 0? or else? \$\endgroup\$ Feb 5 '21 at 12:28
  • \$\begingroup\$ These are the exceptions pastebin.com/EXnVhAtL. i.imgur.com/XUno1Jy.png \$\endgroup\$
    – nop
    Feb 5 '21 at 12:48
  • \$\begingroup\$ @nop This is compilation error. Let me check. \$\endgroup\$ Feb 5 '21 at 12:52
  • 1
    \$\begingroup\$ It's because of the specified types of the arguments. No idea why that is. I guess I will stick to the .Item1, 2, 3 way. Thanks again \$\endgroup\$
    – nop
    Feb 5 '21 at 14:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.