authority icon indicating copy to clipboard operation
authority copied to clipboard

Refactoring the class to remove useless conditions ( else )

Open ghost opened this issue 9 years ago • 1 comments

  • This is my first contribution. I cleared a small piece of code and all tests are passing. :+1:

ghost avatar Dec 06 '14 01:12 ghost

Hey @iBrunoSan,

Thanks for the effort you put in and congrats on it being your first contribution!

I actually did some of these things intentionally, so I'll give you a brief explanation as to why in a moment because it might not be immediately clear.

First I just wanted to address a few points of critique to help you in further contributions that I noticed in here.

  1. You changed the file modes from 0644 to 0755 on all of the files in this PR. This was likely just something that your editor did for you (sometimes editors think they know what we want). If you ever notice these changing in the diffs just be sure to set them back to what their original values were.
  2. Give full explanations of the changes in your PR descriptions or commits. You want the maintainer to get a good idea of what was changed before they dive in and notice some extras. This doesn't have to be super thorough, but just a good overview.
  3. Try to observe coding styles before making some changes. I try to follow PSR-2 and I saw a few style things in your code that wasn't PSR-compliant. It's a super easy fix and I don't have this documented anywhere, but just something to be aware of in general.

As for my reasoning with the else clauses:

In my libraries I try to avoid multiple return statements per method so that people coming along can easily parse through the function, see I'm returning a given value at the end only then jump back to see how it's being set prior. While I tend to use things like sentinel return expressions in my production code and JS or Ruby code, I have some bias toward single exit methods in libraries and in standard code blocks unless it's the first line. The highest return count I try to go is two.

Additionally I find long return blocks like this difficult to parse for people in PHP https://github.com/machuga/authority/pull/24/files#diff-d8d497cfa26245d5c1313eac5650273aR57. While in some Ruby or JS code or Lisp I'd find this better suited, but in PHP it (subjectively) harder to read.

Hopefully that helps explain some of my means and gives you some helpful things to take away onto other projects.

machuga avatar Dec 08 '14 00:12 machuga