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 #2678

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

Conversation

maksym-mishchanchuk
Copy link

No description provided.

Copy link

@dffUqp dffUqp left a comment

Choose a reason for hiding this comment

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

Almost done:smiley:

// write code here
[].__proto__.join2 = function(separator = ',') {
let stringAfterJoin = '';
const copyArray = this.slice();
Copy link

Choose a reason for hiding this comment

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

  1. [CODE STYLE] - avoid creating needless copies of arrays, you can iterate existing array

Comment on lines 16 to 18
if (finallySeparator === null) {
finallySeparator = 'null';
}
Copy link

@dffUqp dffUqp Aug 7, 2023

Choose a reason for hiding this comment

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

little advice for you

console.log(String(null)) // 'null'
console.log(typeof String(null)) // string

const test = null 

console.log(`${test}`) // 'null'
console.log("result: " + test) // result: null

}

for (let i = 0; i < copyArray.length; i++) {
if (copyArray[i] === undefined || copyArray[i] === null) {
Copy link

Choose a reason for hiding this comment

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

try to invert this condition i think if we go in this direction it might help us to write shorter code

Suggested change
if (copyArray[i] === undefined || copyArray[i] === null) {
if (copyArray[i] != null) {

Copy link

@Moroz-Dmytro Moroz-Dmytro left a comment

Choose a reason for hiding this comment

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

Let's make your code cleaner

// write code here
[].__proto__.join2 = function(separator = ',') {
let stringAfterJoin = '';
const finallySeparator = separator;

Choose a reason for hiding this comment

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

Why do we need this?

Comment on lines 11 to 13
if (this.length === 0) {
return '';
}

Choose a reason for hiding this comment

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

Do we really need this?
If length === 0 we will skip the loop and return stringAfterJoin which is equal to ''

}

for (let i = 0; i < this.length; i++) {
if (this[i] !== null && this[i] !== undefined) {

Choose a reason for hiding this comment

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

You are using this[i] 3 times. Create variable for this value

stringAfterJoin += this[i];
}

if (i < this.length - 1 && finallySeparator !== undefined) {

Choose a reason for hiding this comment

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

You set a default value for the separator, so it will never be equal to undefined

stringAfterJoin += element;
}

if (i < this.length - 1) {
Copy link

Choose a reason for hiding this comment

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

I suggest putting this.length in a variable, since we already use it twice, but I guess it's optional

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.

5 participants