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

else:no need to use else #17057

Closed
wants to merge 1 commit into from
Closed

else:no need to use else #17057

wants to merge 1 commit into from

Conversation

Johnsavadkuhi
Copy link
Contributor

@Johnsavadkuhi Johnsavadkuhi commented Nov 15, 2017

good :

 if (engine._info)
    return { buffer, engine };
else
   return buffer;

better :

 if (engine._info)
    return { buffer, engine };
 return buffer;

good  : 
 if (engine._info)
    return { buffer, engine };
else  
   return buffer;

better  : 
 if (engine._info)
    return { buffer, engine };
 return buffer;
@nodejs-github-bot nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label Nov 15, 2017
@gibfahn
Copy link
Member

gibfahn commented Nov 15, 2017

-0 on this, I find the explicit else more readable, especially as the if doesn't have braces.

@Johnsavadkuhi why do you think it's better?

@Trott
Copy link
Member

Trott commented Nov 16, 2017

If we decide the style proposed here is better, we can use an ESLint's no-else-return rule to enforce it. No opinion either way from me on this.

@Johnsavadkuhi
Copy link
Contributor Author

Johnsavadkuhi commented Nov 16, 2017

first , thank you a lot because of creating Nodejs for us.
more readability is good i'm java programmer and am not professional as you in JavaScript i would like to say i came here to improve my JS Skill but when i saw a lot of nested if and else and also this way to return something and i thought it's like as java or c or c++ and then i changed it.

in my opinion creating professional code helps to other programmers to improve their programming skills also i think every programmer clearly know that return statement destroy function life and without else , readability still is good

every programmer know the result is not "hello"

function x ( ){ 
if (true ) 
    return  ; 
console.log("hello") ;
} 

and then :

function y ( ) {
if (true ) 
    return ; 
else  
      console.log("hello") ; 
}

which is better ? it is simple code and both are good and the second is more readable for beginner programmer not for professional and we know a beginner have to learn more and practice more and more

let me to show you a code in zlib.js

function createConvenienceMethod(ctor, sync) {
  if (sync) {
    return function(buffer, opts) {
      return zlibBufferSync(new ctor(opts), buffer);
    };
  } else {
    return function(buffer, opts, callback) {
      if (typeof opts === 'function') {
        callback = opts;
        opts = {};
      }
      return zlibBuffer(new ctor(opts), buffer, callback);
    };
  }
}

without else :

function createConvenienceMethod(ctor, sync) {
  if (sync) {
    return function(buffer, opts) {
      return zlibBufferSync(new ctor(opts), buffer);
    };
  }
  return function(buffer, opts, callback) {
    if (typeof opts === 'function') {
      callback = opts;
      opts = {};
    }
    return zlibBuffer(new ctor(opts), buffer, callback);
  };
}

which is better and shorter ? i think the second is better because is shorter and less Braces

@Johnsavadkuhi
Copy link
Contributor Author

...

@Trott
Copy link
Member

Trott commented Nov 17, 2017

No one has voiced a strong objection. (@gibfahn was a -0 which I take to mean they don't like it but wouldn't block it. Others have approved it.) So this can get a CI run and land if someone is sufficiently supportive to do that. Until that happens, it's still possible for another Collaborator to block it with an objection.

I'm neutral on this myself. I don't care one way or the other, but I do prefer consistency across the code base. So whatever is decided, I hope we can enforce with a lint rule. I suppose that would slightly favor this change, since the lint rule enforcing this style exists and could be deployed.

@apapirovski
Copy link
Member

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

I would like to see this land because it's an eslint rule I want to enable but currently there are way too many instances that need to be fixed. If we can make a dent via this and potentially code-and-learn type of tasks, then that would make it a bit more bearable.

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 21, 2017
jasnell pushed a commit that referenced this pull request Nov 22, 2017
PR-URL: #17057
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Nov 22, 2017

Landed in b98027e

@jasnell jasnell closed this Nov 22, 2017
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 28, 2017
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #17057
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #17057
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.