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

simplify main.js #2913

Merged
merged 12 commits into from
Mar 12, 2020
Merged

simplify main.js #2913

merged 12 commits into from
Mar 12, 2020

Conversation

SEWeiTung
Copy link
Contributor

Change the "if" statment to two simple statements.

Maledong and others added 2 commits January 25, 2020 15:00
Change the "if" statment to two simple statements.
@SEWeiTung SEWeiTung requested a review from XhmikosR January 25, 2020 07:03
@XhmikosR
Copy link
Contributor

Not sure if this is an improvement.

@SEWeiTung SEWeiTung requested a review from a team January 28, 2020 06:24
@XhmikosR XhmikosR changed the title simplify the 'main.js' simplify main.js Feb 2, 2020
@XhmikosR
Copy link
Contributor

XhmikosR commented Feb 2, 2020

@Trott thoughts? I personally don't think this is an improvement.

@Trott
Copy link
Member

Trott commented Feb 2, 2020

@Trott thoughts? I personally don't think this is an improvement.

I appreciate the desire to improve the code but I think the existing code is more maintainable. It's easier to understand, at least to me.

@MaledongGit Is there something in particular about the existing code that makes it problematic that you were hoping to improve?

@nickserv
Copy link
Contributor

nickserv commented Feb 3, 2020

This change reduces repetition. I find the current code difficult to read as the duplication makes the intent of the code harder to understand. The variable name also makes it easier to understand.

static/js/main.js Outdated Show resolved Hide resolved
@XhmikosR
Copy link
Contributor

XhmikosR commented Mar 7, 2020

This whole thing is like premature optimization. I honestly don't see any real benefit from this patch. The code works fine, and we don't even have to get into if toString() is needed :P

@SEWeiTung SEWeiTung merged commit 74b9d51 into nodejs:master Mar 12, 2020
@SEWeiTung SEWeiTung deleted the simplify branch July 11, 2021 02:22
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.

8 participants