0
\$\begingroup\$

The code below takes a list of users and displays two lists

  1. Refused users
  2. Admitted users

Sample output:

Refuse: Phil, Lola.

Admit: Chris, Anne, Colin, Terri, Sam, Kay, Bruce.

Any feedback is highly appreciated.

const people = ['Chris', 'Anne', 'Colin', 'Terri', 'Phil', 'Lola', 'Sam', 'Kay', 'Bruce'];

let refusedPeople = ['Lola', 'Phil']
let admittedPeople = people.filter(name => !refusedPeople.includes(name))
const admitted = document.querySelector('.admitted');
const refused = document.querySelector('.refused');
admitted.textContent = 'Admit: ';
refused.textContent = 'Refuse: '

const refusedIndices = refusedPeople.map(x => people.indexOf(x)).sort()

const admittedIndices = admittedPeople.map(x => people.indexOf(x)).sort()

for (let [index, person] of people.entries()) {
    refusedPeople.includes(person) 
    ?  
        refusedIndices.slice(-1)[0] === index  
        ?    
        refused.textContent += `${person}.` 
        :
        refused.textContent += `${person}, ` 
    :
    admittedIndices.slice(-1)[0] === index
    ?
        admitted.textContent += `${person}.`
    :
        admitted.textContent += `${person}, `;
}
\$\endgroup\$
1
  • 4
    \$\begingroup\$ Please state the specification for the code. What does it do? The title should state the purpose of the code/application. \$\endgroup\$
    – ggorlen
    Nov 22 at 18:32
5
\$\begingroup\$

Avoid performing side-effects in the middle of a ternary expression. If you're not using the resulting value of the ternary operation, then you're using it wrong and should be using an ordinary if-then instead.

Also, be consistent with your usage of semicolons, and let/const.

const people = ['Chris', 'Anne', 'Colin', 'Terri', 'Phil', 'Lola', 'Sam', 'Kay', 'Bruce'];

const refusedPeople = ['Lola', 'Phil'];
const admittedPeople = people.filter(name => !refusedPeople.includes(name));
const admitted = document.querySelector('.admitted');
const refused = document.querySelector('.refused');
admitted.textContent = 'Admit: ';
refused.textContent = 'Refuse: ';

const refusedIndices = refusedPeople.map(x => people.indexOf(x)).sort();

const admittedIndices = admittedPeople.map(x => people.indexOf(x)).sort();

for (const [index, person] of people.entries()) {
    if (refusedPeople.includes(person)) {
        if (refusedIndices.slice(-1)[0] === index) {
            refused.textContent += `${person}.`;
        } else {
            refused.textContent += `${person}, `;
        }
    } else if (admittedIndices.slice(-1)[0] === index) {
        admitted.textContent += `${person}.`;
    } else {
        admitted.textContent += `${person}, `;
    }
}
<p class="admitted"></p>
<p class="refused"></p>

Next, I'm going to split the for loop into two. You seem to have two independent things going on in that loop anyways. If you're dealing with a refused person, then you update the refused text, and if you're dealing with an admitted person, then you update admitted text. Why not just independently loop over your refusedPeople and admittedPeople arrays instead?

(This code sample assumes two things. 1. Your refusedPeople array won't contain anything that's not also in people. 2. The order of output is not important. If either of these matters to you, I'll let you figure out how to best incorporate those details).

const people = ['Chris', 'Anne', 'Colin', 'Terri', 'Phil', 'Lola', 'Sam', 'Kay', 'Bruce'];

const refusedPeople = ['Lola', 'Phil'];
const admittedPeople = people.filter(name => !refusedPeople.includes(name));

const admitted = document.querySelector('.admitted');
const refused = document.querySelector('.refused');

admitted.textContent = 'Admit: ';
refused.textContent = 'Refuse: ';

for (const [index, person] of refusedPeople.entries()) {
    if (index === refusedPeople.length - 1) {
        refused.textContent += `${person}.`;
    } else {
        refused.textContent += `${person}, `;
    }
}

for (const [index, person] of admittedPeople.entries()) {
    if (index === admittedPeople.length - 1) {
        admitted.textContent += `${person}.`;
    } else {
        admitted.textContent += `${person}, `;
    }
}
<p class="admitted"></p>
<p class="refused"></p>

Perhaps you see those two for loops and quickly think "oh, those are very similar, I wonder if I can extract the duplicate parts into some helper function". Turns out, the language already provides just the helper function you need - array.join(). The use of array.join will let us toss both of these for loops.

const people = ['Chris', 'Anne', 'Colin', 'Terri', 'Phil', 'Lola', 'Sam', 'Kay', 'Bruce'];
const refusedPeople = ['Lola', 'Phil'];
const admittedPeople = people.filter(name => !refusedPeople.includes(name));

const admittedElement = document.querySelector('.admitted');
admittedElement.textContent = `Admit: ${admittedPeople.join(', ')}.`;

const refusedElement = document.querySelector('.refused');
refusedElement.textContent = `Refuse: ${refusedPeople.join(', ')}.`;
<p class="admitted"></p>
<p class="refused"></p>

Now that's looking pretty good :).

\$\endgroup\$
5
  • 1
    \$\begingroup\$ Really like you rewrite! \$\endgroup\$
    – konijn
    Nov 23 at 8:12
  • \$\begingroup\$ Thanks! Isn't it more appropriate to use "let" in loop variables? \$\endgroup\$
    – Elbasel
    Nov 23 at 15:51
  • 2
    \$\begingroup\$ "let" is required in a c-style loop for (let i = 0; i < whatever; ++i), because the same binding persists across all iterations, and you reassign to it between each iteration. In a for-of loop, you're effectively creating a new loop variable with each iteration, thus no reassignment is happening, so "const" is appropriate. The use of "const" in for-of will also self-document the fact that you're not reassigning to the loop variable in the middle of your for loop's body. \$\endgroup\$ Nov 23 at 16:10
  • \$\begingroup\$ Your output differs from OP's. Names can not be refused if they are not in the list of people IE You can't be refused of you don't ask. \$\endgroup\$
    – Blindman67
    Nov 25 at 0:24
  • \$\begingroup\$ @Blindman67 Yes - that was one of the assumptions I stated my refactoring made - "Your refusedPeople array won't contain anything that's not also in people". If this assumption is false, then you're right, and the O.P. would need to modify the answer to fit their needs, or to use your answer. \$\endgroup\$ Nov 25 at 2:38
2
\$\begingroup\$

Using a Set

You are best to use a Set to reduce the complexity that comes with Array.includes.

Ignoring the sort. Your code has a complexity of \$O(n^2)\$ however using a set will reduce that to \$O(n)\$

Example of \$O(n^2)\$

dataA.filter(v => dataB.includes(v));

Example of same task with \$O(n)\$ complexity

set = new Set(dataB);
dataA.filter(v => set.has(v));

Functions

Even if writing examples always create the code of interest as a named function. See rewrite ListAdmittance

Also to reduce the verbosity (repeated noise) of your code use functions to perform repeated tasks. In rewrite the functions query and strList reduce the need to repeat the long document.querySelector( and simplifies the listing of names.

Sort

You need only Array.sort the list of people not the list of refused people. The refused people must be in the people list and thus you use the people list to order the refused list.

In the example the sort is performed outside the function listAdmittance should not be part of the functions role.

Rewrite

The rewrite aims to reduce complexity and code noise, and be reusable.

const query = (qStr, root = document) => root.querySelector(qStr);
const strList = (items, s = "[", del = ", ", e = ".") => s + items.join(del) + e;

listAdmittance(
    ['Chris', 'Anne', 'Colin', 'Terri', 'Phil', 'Lola', 'Sam', 'Kay', 'Bruce'].sort(),
    ['Phil', 'Lola', `Bill`]
);

function listAdmittance(all, refused) {
    const rMap = new Set(refused);
    query("#admitted").textContent = strList(all.filter(n => !rMap.has(n)), "Admit: ");
    query("#refused").textContent  = strList(all.filter(n => rMap.has(n)), "Refuse: ");
}
 
<div id="admitted"></div>
<div id="refused"></div>

You could also avoid the need to query the document as follows

listAdmittance(
    ['Chris', 'Anne', 'Colin', 'Terri', 'Phil', 'Lola', 'Sam', 'Kay', 'Bruce'].sort(),
    ['Phil', 'Lola', `Bill`]
);
const strList = (items, s = "[", del = ", ", e = ".") => s + items.join(del) + e;
function listAdmittance(all, refused) {
    const rMap = new Set(refused);
    admittedEl.textContent = strList(all.filter(n => !rMap.has(n)), "Admit: ");
    refusedEl.textContent  = strList(all.filter(n => rMap.has(n)), "Refuse: ");
}
 
<div id="admittedEl"></div>
<div id="refusedEl"></div>

\$\endgroup\$

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.