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>();
}
public void AddAgentCommission(AgentCommission agentCommission)
{
AgentCommissions.Add(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;
betCommission.AddAgentCommission(new AgentCommission(pos.AgentId, pos.Commission));
}
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.
Instead of writing
.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>())
{}
public void AddAgentCommission(AgentCommission agentCommission)
{
AgentCommissions.Add(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.
Next step is to get the best ones from this list:
var commissions = Calculator.CalculateSpecificCommissions(allCommissions, bet);
Then the calculator could group all commissions on agent and pick the most specific based on the bet.
I see two possible performance problems here:
- The commission query. I have no idea if there are many agents, or how difficult it is to retrieve their commissions. This may, or may not, be a bottleneck. By moving the query out of the way, it would be easier to introduce more efficient database schemas or caching.
- The commission calculator. In my head, I see a list being filtered in memory. That by itself should be fast, but I know nothing about how many times that calculation needs to be done or when it’s done.
I suggest you profile your code for bottlenecks if you haven’t already done so. I had to figure out once, why a certain piece of code needed 40 minutes to generate two short chapters of text: There were millions of synchronous calls to the database over the network. That’s an order of magnitude more than there were words in said chapters.