Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 50 additions & 1 deletion src/scripts/main.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,52 @@
'use strict';

// write code here
const li = document.querySelectorAll('li');
const father = li[0].parentNode;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deriving father from li[0] at module scope is unsafe when the NodeList might be empty and also couples code to a global variable. Compute the parent element inside sortList from the passed list (e.g. const parent = list[0]?.parentElement) and return early if missing. This addresses checklist items #11 and #12.


function sortList(list) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sortList should not rely on the top-level father variable. Use a parent derived from the list argument (use parentElement) and append sorted items to that parent. Making sortList self-contained ensures it works with any list passed in (checklist items #12 and #14).

const resultList = Array.from(list).sort((firstPerson, secondPerson) => {
const firstPersonSalary = parseSalary(firstPerson.dataset.salary, 'salary');
const secondPersonSalary = parseSalary(
secondPerson.dataset.salary,
'salary',
);

return secondPersonSalary - firstPersonSalary;
Comment on lines +7 to +14

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comparator calls parseSalary(...) which must return numeric values. Ensure parseSalary reliably returns a Number (see the comment on line 46). Also make the comparator operate on Numbers directly (subtract numeric results) — that is already attempted but depends on correct parseSalary behavior (checklist items #15 and #16).

});

for (const result of resultList) {
if (father) {
father.append(result);
Comment on lines +17 to +19

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appending using the top-level father inside the loop keeps global coupling. Use a parent local to sortList and append there. Also ensure sortList is not called multiple times (see lines 41-42) to avoid unnecessary DOM operations and ordering issues (checklist items #14 and #18).

}
}

return resultList;
}

function getEmployees(list) {
const arraysPersons = [];

for (const oneLi of list) {
arraysPersons.push({
name: oneLi.textContent,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using oneLi.textContent for name can include other text from the li. Select the specific element that contains the name (e.g. oneLi.querySelector('.name')?.textContent.trim()) and use textContent.trim() so name matches expected test values.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using oneLi.textContent for name will include other text (position/age) and may not match tests. Extract the name from the specific child (for example oneLi.querySelector('.name')?.textContent.trim()) and trim it. Tests expect name from the dedicated element and clean string (checklist item #5).

position: oneLi.dataset.position,
salary: parseSalary(oneLi.dataset.salary, 'salary'),
age: parseSalary(oneLi.dataset.age, 'age'),
Comment on lines +33 to +34

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

salary and age should be Numbers in the returned objects. You already call parseSalary, but ensure parseSalary returns Number for salary and age so getEmployees returns numeric salary and age (checklist items #5 and #16).

});
}

return arraysPersons;
}

sortList(li);
getEmployees(sortList(li));
Comment on lines +41 to +42

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling sortList(li); and then getEmployees(sortList(li)); runs sortList twice. Call sortList once, save its result (const sorted = sortList(li);) and then call getEmployees(sorted) (or call getEmployees on the parent’s children after sorting). This prevents duplicate DOM moves and guarantees the employee array order matches the visible list (checklist item #18 and previous-review suggestion #25).


function parseSalary(str, what) {
if (what === 'salary') {
return Number(str.slice(1).split(',').join('.'));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current salary normalization (str.slice(1).split(',').join('.')) converts thousand separators into a decimal point (e.g. "1,200" -> "1.200" -> 1.2), which is incorrect. Instead remove currency symbols and thousands separators, and normalize a decimal comma if present, then return Number(...). For example: const s = str.replace(/[^0-9.,-]/g, ''); const normalized = s.replace(/,/g, ''); return Number(normalized); — or implement safer logic to preserve decimal part. Ensure parseSalary always returns a Number (checklist items #15 and #16).

}

if (what === 'age') {
return Number(str);
}
}