3
\$\begingroup\$

I made the code underneath to sort my array and group people with the same score to show who all got 1st, 2nd and 3rd.

the "points" variable looks like this

points: {
  '204255221017214977': 3,
  '208993609255485441': 5,
  '382197942762602496': 4,
  '664606841409634324': 5,
  '779857709155090432': 4,
  '381084508276916225': 5,
  '794200859612545075': 4,
  '155149108183695360': 4,
  '438978127973318656': 3,
  '304028235082104843': 2,
  '174548757151481856': 1
}

I was wondering if there is a much easier way to sort and put together the string like I did underneath with much easier code. Cuz I'll be honest this code looks and feels aweful.

example endresult:

:first_place: @Excanator had 5 points
:first_place: @zerow had 5 points
:first_place: @AnimeHotOwO had 5 points

:second_place: @christmas beans had 4 points
:second_place: @ExcavatorGames had 4 points
:second_place: @Dyno had 4 points
:second_place: @LogiBot Premium had 4 points

:third_place: @Restarter v3 had 3 points
:third_place: @YAGPDB.xyz had 3 points

@aNt had 2 points
@Kaori had 1 point
const sorted = Object.keys(points).sort((a, b) => {
    return points[b] - points[a]
})

var results = `\n`

var firstPlace = points[sorted[0]]
var secondPlace = null
var thirdPlace = null
var leftover = null

for(const key of sorted) {
    var amount = points[key]
    if(leftover){
        results += `<@${key}> had ${amount} point${amount === 1 ? '' : 's'}\n`
    } else if(thirdPlace) {
        if(thirdPlace === amount) {
            results += `:third_place: <@${key}> had ${amount} point${amount === 1 ? '' : 's'}\n`
        } else {
            results += `\n`
            results += `<@${key}> had ${amount} point${amount === 1 ? '' : 's'}\n`
            leftover = amount
        }
    } else if(secondPlace) {
        if(secondPlace === amount) {
            results += `:second_place: <@${key}> had ${amount} point${amount === 1 ? '' : 's'}\n`
        } else {
            results += `\n`
            results += `:third_place: <@${key}> had ${amount} point${amount === 1 ? '' : 's'}\n`
            thirdPlace = amount
        }
    } else if(firstPlace) {
        if(firstPlace === amount) {
            results += `:first_place: <@${key}> had ${amount} point${amount === 1 ? '' : 's'}\n`
        } else {
            results += `\n`
            results += `:second_place: <@${key}> had ${amount} point${amount === 1 ? '' : 's'}\n`
            secondPlace = amount
        }
    }
}
```
\$\endgroup\$
2
\$\begingroup\$

Functions

Always write code as functions. Even when its just an example, writing code as a function gives it a name, and make you think in terms of that name.

Functions should be as simple as possible doing one thing only. (see rewrite)

Review

  • USE SEMICOLONS!!!
  • Don't add code that is redundant or superfluous. Eg var secondPlace = null can be var secondPlace.
  • Keep it DRY. (Don't Repeat Yourself). You have a lot of repeated code that does similar or the same. Using function will reduce a lot of repeated code.
  • Use constants to define magic values. (see rewrite)
  • Use an Array to hold lists of strings then use Array.join to join them using the delimiter (see rewrite)
  • Use appropriate names. Names define the abstractions you use, poor naming make code harder to understand especially when a single abstract concept gets many names, or the same name represent multiple abstractions.
    • points As records should be results
    • key could be id.
    • amount could be score

Rewrite

Rewrite is just an example as there are many ways to do this.

Functions

The rewrite breaks the task into 3 parts.

  • groupByScore groups by score. Using a Map indexed by score to hold arrays of ids with each score. Returns the groups as the Map
  • sortByScore Takes the grouped results and sorts by score returning an array
  • resultsByPlace Takes an array of grouped results and returning an array of strings each containing the place, id, and score.

Constants

There are two constants that remove magic values from the code and moves them to one place at the top of the code.

One of the constants is defined as a function score2Str and created the string eg " has 1 point"

Util functions

Common tasks can be written as utility functions. These function can be part of a library of functions you can use within other projects. There is one Util function pluralize that adds a "s" when a value is not != 1. It is defined as a tagged template and can be used as a tagged template

pluralize` point${score}`

Or called as a function

pluralize([` point`], score)

As modules

Thus we get the code as

File strings/utils.jsm

const pluralize = (strs, value) => strs[0] + (value != 1 ? "s" : "") + (strs[1] ? strs[1]: "");
export {pluralize};

File studentResults.jsm

import {pluralize} from "./strings/utils.jsm";

const PLACINGS = ":First Place: ,:Second Place: ,:Thrid Place: ".split(",");
const score2Str = score => " has " + score +  pluralize` point${score}`;

function groupByScore(students) {
    const scoreGroups = new Map();
    Object.entries(students).forEach(([id, score]) => {
        const group = scoreGroups.get(score);
        if (group) { group.push(id) }
        else { scoreGroups.set(score, [id]) }
    });
    return scoreGroups;
}    
function sortByScore(scoreGroups) {
    return [...scoreGroups.entries()].sort((gA, gB) => gB[0] - gA[0]);
}    
function resultsByPlace(sorted, cats = PLACINGS, scoreStr = score2Str) {
    var idx = 0;
    const results = [];
    for (const [score, group] of sorted) {
        const cat = idx < cats.length ? cats[idx ++] : (idx ++, "");
        results.push(...group.map(id => cat + id + scoreStr(score)));
    }
    return results;
}
export {groupByScore, sortByScore, resultsByPlace};

As running example.

As CodeReview snippets do not support modules below is a working example compatible with CodeReview snippets.

Code

const pluralize = (strs, value) => strs[0] + (value != 1 ? "s" : "") + (strs[1] ? strs[1]: "");
const tag = (tag, props = {}) => Object.assign(document.createElement(tag), props);
const append = (par, ...sibs) => sibs.reduce((p, sib) => (p.appendChild(sib), p), par);

// Rewrite
const studentScores = {'Adam': 3, 'Bill': 5,'Cindy': 4, 'Dafney': 5, 'Eric': 4, 'Fred': 5, 'Graham': 4, 'Hillary': 4, 'Ian': 3, 'Jill': 1, 'Kev': 0};
const PLACINGS = ":First Place: ,:Second Place: ,:Thrid Place: ".split(",");
const score2Str = score => " has " + score +  pluralize` point${score}`;

const results = resultsByPlace(sortByScore(groupByScore(studentScores)));
append(document.body, ...results.map(result => tag("div",{textContent: result})));
function groupByScore(students) {
    const scoreGroups = new Map();
    Object.entries(students).forEach(([id, score]) => {
        const group = scoreGroups.get(score);
        if (group) { group.push(id) }
        else { scoreGroups.set(score, [id]) }
    });
    return scoreGroups;
}
function sortByScore(scoreGroups) {
    return [...scoreGroups.entries()].sort((gA, gB) => gB[0] - gA[0]);
}
function resultsByPlace(sorted, cats = PLACINGS) {
    var idx = 0;
    const results = [];
    for (const [score, group] of sorted) {
        const cat = idx < cats.length ? cats[idx ++] : (idx ++, "");
        results.push(...group.map(id => cat + id + score2Str(score)));
    }
    return results;
}

\$\endgroup\$
3
  • \$\begingroup\$ Can I ask, why do you say I should ABSOLUTELY use semicolons? I've never really learnt the use of them and code seems to work without it too, is it performance related? \$\endgroup\$ – ExcanatorGames Jan 20 at 19:56
  • \$\begingroup\$ @ExcanatorGames JavaScript requires semicolons. Not using them and they will be inserted automatically ASI (Automatic Semicolon Insertion). There are many ambiguous situations where the parser will not put the semicolon where one would expect. This can result in very hard to spot bugs (may not throw errors) Unless you know of by heart how ASI inserts semicolons you risk code that does not work as expected. Using them ensures this will never happen. For details developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/… \$\endgroup\$ – Blindman67 Jan 20 at 20:15
  • \$\begingroup\$ I see! thank you very much for the explanation! \$\endgroup\$ – ExcanatorGames Jan 20 at 21:09
1
\$\begingroup\$

Your code would benefit from separating data and display. Results can also be constructed by using the .join() method on an array, reducing the need for checking boundaries. I'm not sure that these modifications resulted in "easier" code, but the singular point of truth makes it harder for bugs to hide.

const points = {
  '204255221017214977': 3,
  '208993609255485441': 5,
  '382197942762602496': 4,
  '664606841409634324': 5,
  '779857709155090432': 4,
  '381084508276916225': 5,
  '794200859612545075': 4,
  '155149108183695360': 4,
  '438978127973318656': 3,
  '304028235082104843': 2,
  '174548757151481856': 1
}

// arrange items into a dictionary of lists
// keyFunc determines bin for each item
function bin(items, keyFunc) {
    const bins = new Map();
    for (const item of items) {
        const key = keyFunc(item);
        if (!bins.get(key)) bins.set(key,[]);
        bins.get(key).push(item);
    }
    return bins;
}

// create a list of lists of player tuples
// each top level list represents a medal position
function createPodium(points) {
    // bin players by score
    const bins = bin(Object.entries(points), ([player,score])=>score);
    const podium = [...bins.entries()];
    // sort medal positions by descending score
    podium.sort((a,b)=>b[0]-a[0]);
    // return only the players
    return podium.map(([score,players])=>players);
}

const MEDAL_NAMES = ['first_place', 'second_place', 'third_place'];
function medalName(index) {
    const medal = MEDAL_NAMES[index];
    if (medal) return `:${medal}: `;
    else return '';
}

const playerString = (medal,[name,score]) =>
    medalName(medal)+
    `<${name}> had ${score} point${score>1?'s':''}`;

const podiumString = podium =>
    podium.flatMap((players,medal)=>
        players.flatMap(
            player=>playerString(medal,player)
        ).concat(['']) // add extra line between
    ).join('\n');

const podium = createPodium(points);
console.log(podiumString(podium));
\$\endgroup\$
1
  • \$\begingroup\$ You're right it's not necessarily easier code but it sure does look and feel a lot better than what I came up with! It's a bit too advanced for me to think of doing it like this but that's great because it means I'll learn from it! thank you! \$\endgroup\$ – ExcanatorGames Jan 20 at 8:24
0
\$\begingroup\$

You can also use string templating. Something like this:

const points = {
    '204255221017214977': 3,
    '208993609255485441': 5,
    '382197942762602496': 4,
    '664606841409634324': 5,
    '779857709155090432': 4,
    '381084508276916225': 5,
    '794200859612545075': 4,
    '155149108183695360': 4,
    '438978127973318656': 3,
    '304028235082104843': 2,
    '174548757151481856': 1
};

const placeTemplates = {
    5: (userId, points) => `:first_place: ${userId} had ${points} points`,
    4: (userId, points) => `:secondt_place: ${userId} had ${points} points`,
    3: (userId, points) => `:third_place_place: ${userId} had ${points} points`,
    2: (userId, points) => `${userId} had ${points} points`, // DRY :-/
    1: (userId, points) => `${userId} had ${points} points` // DRY :-/
}

const aggregated = Object.entries(points).reduce((akku, userId_point) => {
    const [userId, point] = userId_point;
    if(!Array.isArray(akku[point])) {
        akku[point] = []
    }
    akku[point].push(placeTemplates[point](userId, point));
    return akku;
}, {});

const result = Object.keys(aggregated).reverse().reduce((akku, point) => akku.concat(aggregated[point]), []).join("\n");
document.getElementById('wrapper').innerText = result;
<div id="wrapper"></div>

\$\endgroup\$
3
  • \$\begingroup\$ Wouldn't I have to make a placeTemplate for every score possible? I put the points for testing purpouses no higher than 5 but in theory they can go till inifity \$\endgroup\$ – ExcanatorGames Jan 20 at 8:21
  • \$\begingroup\$ You can create placeTemplates with loop.. \$\endgroup\$ – Arthur Rubens Jan 20 at 8:37
  • \$\begingroup\$ fair fair. my brain was working at 5am hrs/thought my bad. \$\endgroup\$ – ExcanatorGames Jan 20 at 19:01

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.