Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

initial commit #2739

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

initial commit #2739

wants to merge 3 commits into from

Conversation

Barosz30
Copy link

@Barosz30 Barosz30 commented Nov 8, 2023

No description provided.

Copy link

@zaura333 zaura333 left a comment

Choose a reason for hiding this comment

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

Overall good job! :) Code works and the purpose of each step is clear, but there's room to make it more compact and limit unnecessary repeating

return fixedKey;
});

// console.log(array);
Copy link

Choose a reason for hiding this comment

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

We can get rid of unnecessary comments

Comment on lines 19 to 40
let array = sourceString.split(';');

if (array.length === 0) {
return {};
}

// let's remove unnecesary signs here (clearing array)
array = array.map((key) => {
return key.replace(/\n/g, '').trim();
}).filter(Boolean);

// creating key:value pairs in array
array = array.map((key) => {
return key.split(':');
});

// cleaning inside, this time triming each element
array = array.map((key) => {
const fixedKey = key.map((el) => el.trim());

return fixedKey;
});
Copy link

Choose a reason for hiding this comment

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

Instead of overwriting array, you could try to implement chaining of array methods

Comment on lines 31 to 40
array = array.map((key) => {
return key.split(':');
});

// cleaning inside, this time triming each element
array = array.map((key) => {
const fixedKey = key.map((el) => el.trim());

return fixedKey;
});
Copy link

Choose a reason for hiding this comment

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

E.g. here you could use two methods on key ( .split(...).map(...) ) while mapping the whole array only one time

Copy link

@mvjl000 mvjl000 left a comment

Choose a reason for hiding this comment

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

I totally agree with the previous reviewer. You are iterating through the array way more times than necessary.

It works, good for you ;) But I would love to see a 'cleaner' solution

@Barosz30 Barosz30 requested a review from mvjl000 November 9, 2023 10:26
Copy link

@mvjl000 mvjl000 left a comment

Choose a reason for hiding this comment

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

Looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants