Code Review Asked by Stan Loona on December 2, 2021
I’ve been working on a personal project. There’s this game called TFT which is an autochess game. Each match is played by 8 players and there’s a shared pool of champions. Each champion has their cost and how many of each there are, below is the table:
12 Champs that cost 1 gold, 39 of each champ.
12 Champs that cost 2 gold, 26 of each champ.
12 Champs that cost 3 gold, 18 of each champ.
9 Champs that cost 4 gold, 13 of each champ.
6 Champs that cost 5 gold, 10 of each champ.
So, I’ve been doing some functions to get the probability of getting x champ in a 5 champion deck, something I forgot to say is that in the game, you have your player level which somewhat influences in the pool, below is the multiplicator array, each row is a player level starting from Level 2 (which is the zeroth row) to Level 9, and each column is the champions cost (cost 1 to 5).
// pool.class.js
class Pool {
constructor(champPool) {
this.champPool = champPool; //this is an array of Champions(name,pool,cost)
this.chancePerTier = [
[1, 0, 0, 0, 0], //Level 2
[0.7, 0.25, 0.05, 0, 0], //Level 3
[0.5, 0.35, 0.15, 0, 0], //Level 4
[0.35, 0.35, 0.25, 0.05, 0], //Level 5
[0.25, 0.35, 0.3, 0.1, 0], //Level 6
[0.2, 0.3, 0.33, 0.15, 0.02], //Level 7
[0.15, 0.2, 0.35, 0.24, 0.06], //Level 8
[0.1, 0.15, 0.3, 0.3, 0.15] //Level 9
];
}
getChampProbability(champName, playerLevel) {
let poolSize = this.getPoolBasedOnLevel(playerLevel);
let poolIndex = this.champPool.findIndex(function(champ) {
return champ.getName() === champName;
});
if (poolIndex === -1) return 0;
let champPool = this.champPool[poolIndex].getPool();
let champCost = this.champPool[poolIndex].getCost();
return (
(champPool / poolSize) *
5 *
100 *
this.chancePerTier[playerLevel - 2][champCost - 1]
).toFixed(2);
}
getPoolBasedOnLevel(summonerLevel) {
let pool;
if (summonerLevel == 2) {
pool = this.champPool.reduce(function(prevChamp, currentChamp) {
if (currentChamp.getCost() == 1) {
return prevChamp + currentChamp.getPool();
} else {
return prevChamp;
}
}, 0);
} else if (summonerLevel == 3 || summonerLevel == 4) {
pool = this.champPool.reduce(function(prevChamp, currentChamp) {
if (currentChamp.getCost() >= 1 && currentChamp.getCost() <= 3) {
return prevChamp + currentChamp.getPool();
} else {
return prevChamp;
}
}, 0);
} else if (summonerLevel == 5 || summonerLevel == 6) {
pool = this.champPool.reduce(function(prevChamp, currentChamp) {
if (currentChamp.getCost() >= 1 && currentChamp.getCost() <= 4) {
return prevChamp + currentChamp.getPool();
} else {
return prevChamp;
}
}, 0);
} else {
pool = this.getWholePool();
}
return pool;
}
getWholePool() {
let allPool = this.champPool.reduce(function(prevChamp, currentChamp) {
return prevChamp + currentChamp.getPool();
}, 0);
return allPool;
}
}
module.exports = Pool;
// champion.class.js
class Champion {
constructor(name, pool, cost, origin) {
this.name = name; // String
this.pool = pool; // Integer
this.cost = cost; // Integer
this.origin = origin; // Origin
}
getName() {
return this.name;
}
setPool(newPool) {
this.pool = newPool;
}
getPool() {
return this.pool;
}
getCost() {
return this.cost;
}
getOrigin() {
return this.origin;
}
}
module.exports = Champion;
The code is working, but I think there’s a good-looking approach to this ugly mess.
getPoolBasedOnLevel()
is the oddity here. There are three arrow functions doing similar jobs. Using one function with a lookup should clean things up. Effectively, each of them is doing this:
const sum = list=>list.reduce((a,b)=>a+b,0)
function poolTotal(champ_pool,min_cost,max_cost) {
const champs_for_level = champ_pool.filter(champ=>
champ.getCost()>=min_cost &&
champ.getCost()<=max_cost
);
const pools = champs_for_level.map(
champ=>champ.getPool()
)
return sum(pools);
}
That function also implicitly determines the min and maxchampion cost for each level. It would benefit readability to state this is explcitly.
function costsForLevel(level) {
if (level===2) return {min:1,max:1}
if (level===3 || level===4) return {min:1,max:3}
if (level===5 || level===6) return {min:1,max:4}
return {min:0,max:Number.MAX_VALUE}
}
With both of these functions defined, then getPoolBasedOnLevel()
can be written as so
getPoolBasedOnLevel(summonerLevel) {
const {min,max} = costForLevel(summonerLevel);
return poolTotal(this.champPool,min,max);
}
Answered by Ted Brownlow on December 2, 2021
As was already mentioned in the comments, a switch
statement could be used to clean up the sets of if
statements. Another option would be to abstract the similar callback functions passed to .reduce()
- perhaps using partially applied functions to accept parameters - e.g. the minimum values in conditions like currentChamp.getCost() <= 3
.
It seems like let
is used for most variables. It is wise to default to using const
to avoid accidental re-assignment. When you decide you do need to reassign a value then use let
.
The method Pool::getChampProbability()
could use this.champPool.find()
instead of this.champPool.findIndex()
, since the index
is only used to dereference an array element. This would mean 0
should be returned if find()
returned undefined
(instead of if (poolIndex === -1) return 0;
).
The method Pool::getWholePool()
has a single use variable- i.e. allPool
. That variable can be eliminated. Use of a linter would help find things like this.
The else
keywords can be eliminated in many spots following a return
in a conditional e.g. many of the else’s in Pool::getPoolBasedOnLevel()
Answered by Sᴀᴍ Onᴇᴌᴀ on December 2, 2021
Get help from others!
Recent Questions
Recent Answers
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP