# Commission calculation

Posted on

Problem

I am trying to improve the performance of the current solution for calculating commission in a betting platform.

The following is the code currently used to solve the problem. The primary problem here is the `CalculateCommission` method, which is ugly and seems to do the same thing multiple times. I’ve tried but not been able to come up with a way to simplify the query (or solve the problem in some other way not thought of), so am looking for community help to fix the problem.

``````//All agents commissions for the current user
userCommissions = List<Commission>

public class Commission
{
public long Id { get; set; }
public long AgentId { get; set; }
public int Level { get; set; }
public long? UserId { get; set; }
public long? SportId { get; set; }
public long? LeagueId { get; set; }
public EMarketType MarketType { get; set; }
public int Commission { get; set; }
}

public class AgentCommission
{
public long AgentId { get; set; }
public int Commission{ get; set; }

public AgentCommission()
{
}

public AgentCommission(long agentId, int commission)
{
Commission= commission;
AgentId = agentId;
}
}

public class BetCommission
{
public int TotalCommission { get; private set; }
public List<AgentCommission> AgentCommissions { get; }

public BetCommission()
{
AgentCommissions = new List<AgentCommission>();
}

{
TotalCommission += agentCommission.Commission;
}
}

protected ConcurrentDictionary<string, BetCommission> betCommissionsByUser;

public void CalculateCommission() {
var levelCommissions = userCommissions.GroupBy(x => new { x.MarketType, x.Level }).ToList();
var keysGrouping = userCommissions.GroupBy(x => new { x.SportId, x.LeagueId, x.MarketType }).ToList();

foreach (var bpKey in keysGrouping)
{
var betCommission = new BetCommission();
var sportId = bpKey.Key.SportId ?? 0;
var leagueId = bpKey.Key.LeagueId ?? 0;

var key = \$"{userId}-{sportId}-{leagueId}-{bpKey.Key.MarketType}";

var commissionsForCurrentKey = levelCommissions.Where(x => x.Key.MarketType == bpKey.Key.MarketType)
.SelectMany(x => x)
.Where(x => (x.SportId == null || x.SportId == sportId) && (x.LeagueId == null || x.LeagueId == leagueId))
.OrderBy(x => x.Level).ThenByDescending(x => x.LeagueId).ThenByDescending(x => x.SportId);

var currentLevel = 0;
foreach (var pos in commissionsForCurrentKey)
{
if (currentLevel == pos.Level)
continue;

currentLevel = pos.Level;
}

betCommissionsByUser.AddOrUpdate(key, betCommission, (k, v) => betCommission);
}
}
``````

Some background information on the problem:

The platform has a hierarchy of agents that can get commission from users lower in the hierarchy. Commissions are configured to be either global or tied to a specific sport or league. In addition to the optional Sport and League, a commission also has a required value `MarketType`.

When calculating the commission for an agent, the most specific commission configured for the agent is what should be used. For instance, if an agent has configured both a global commission and a sport-specific commission, then the sport-tied one takes precedence.

The goal is to have a dictionary where I can obtain the positions (for all agents in the hierarchy) with a single lookup.

An example of the commission structure:

Commission table on User 1:

``````            General       Soccer      Champions League
Agent 1       1%                      10%
Agent 2       1%          5%          10%
Agent 3       0%          0%          10%
Agent 4       0%          5%
``````

If User 1 places a bet on a Soccer match in the France League 1, the commission should be as follows:

``````Agent 1 = 1%
Agent 2 = 5%
Agent 3 = 0%
Agent 4 = 5%
``````

But if User 1 places a bet on a Soccer match in Champions League, the commission should be as follows:

``````Agent 1 = 10%
Agent 2 = 10%
Agent 3 = 10%
Agent 4 = 5%
``````

Solution

The first thing to change would be the query for `commissionForCurrentKey` to include the condition `x.Level != 0` if `Level` can be negative too, or `x.Level > 0` if it can’t be negative (which I assume now).
In this way any item having a `Level == 0` would be eleminated, hence at least the first iteration in the `foreach (var pos in commissionsForCurrentKey)` loop wouldn’t be senseless.

I would like to encourage you to always use braces `{}` for single statement `if`‘s to make the code less error prone.

By extracting the creation of the `AgentCommission`‘s to a separate method your code will benefit regarding readability and maintainability.

This could look like so

``````private IEnumerable<AgentCommission> CreateAgentCommissions(IOrderedEnumerable<Commission> commissionsForCurrentKey)
{
int currentLevel = 0;
foreach (var pos in commissionsForCurrentKey)
{
if (currentLevel == pos.Level) { continue; }

currentLevel = pos.Level;
yield return new AgentCommission(pos.AgentId, pos.Commission);
}
}
``````

Variables should be placed as near as their usage, so `key` could be just placed above the call to `AddOrUpdate()`.

By changing this second `.Where()` to just compare like `(x.SportId == bpKey.Key.SportId) && (x.LeagueId == bpKey.Key.LeagueId)` it is more readable too.

``````.OrderBy().ThenByDescending().ThenByDescending()
``````

it would be more readable to write

``````.OrderBy()
.ThenByDescending()
.ThenByDescending()
``````

I don’t like the public parameterless constructor of `AgentCommission`. What use does it have ? IMHO none, so you could just delete it. Also having `public` setters can do some harm and should be avoided if the same values can be set by the constructor.

Adding a constructor which takes an `IEnumerable<AgentCommission>` to the `BetCommission` class would be a good idea.

Applying the mentioned points will lead to

``````public class BetCommission
{
public int TotalCommission { get; private set; }
public List<AgentCommission> AgentCommissions { get; }

public BetCommission(IEnumerable<AgentCommission> agentCommissions)
{
AgentCommissions = new List<AgentCommission>(agentCommissions);
}

public BetCommission() : this(Enumerable.Empty<AgentCommission>())
{}

{
TotalCommission += agentCommission.Commission;
}
}

public void CalculateCommission()
{
var levelCommissions = userCommissions.GroupBy(x => new { x.MarketType, x.Level }).ToList();
var keysGrouping = userCommissions.GroupBy(x => new { x.SportId, x.LeagueId, x.MarketType }).ToList();

foreach (var bpKey in keysGrouping)
{
var commissionsForCurrentKey = levelCommissions.Where(x => x.Key.MarketType == bpKey.Key.MarketType)
.SelectMany(x => x)
.Where(x => (x.SportId == bpKey.Key.SportId) && (x.LeagueId == bpKey.Key.LeagueId) && (x.Level > 0))
.OrderBy(x => x.Level)
.ThenByDescending(x => x.LeagueId)
.ThenByDescending(x => x.SportId);

var agentCommissions = CreateAgentCommissions(commissionsForCurrentKey)
var betCommission = new BetCommission(agentCommissions);

var sportId = bpKey.Key.SportId ?? 0;
var leagueId = bpKey.Key.LeagueId ?? 0;
var key = \$"{userId}-{sportId}-{leagueId}-{bpKey.Key.MarketType}";

betCommissionsByUser.AddOrUpdate(key, betCommission, (k, v) => betCommission);
}
}
``````

I am missing some querying concepts here. The bet seems to be about a sport and a league:

``````var bet = new UserBet(sport, league, amount); // and probably more...
``````

Then there are agents handling that bet:

``````var allCommissions = Query.GetAgentCommissionsForBet(bet);
``````

To keep this query simple, I would return all commissions for all agents matching sport or leage, assuming league is unique for the sport. Agent 1 would have two entries in this list, Agent 2 would have three.

``````var commissions = Calculator.CalculateSpecificCommissions(allCommissions, bet);