-
Notifications
You must be signed in to change notification settings - Fork 56
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
[1.1.0] Modify the return reserved keyword & Modify login variables names #8
Conversation
src/SudoSu.php
Outdated
|
||
$this->auth->loginUsingId($userId); | ||
$this->auth->loginUsingId($sudosuUserId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but can I ask what your reasoning is behind changing $userId
to $sudosuUserId
in SudoSu::loginAsUser
?
Not disputing it, just curious!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrterryh see this link https://www.python.org/dev/peps/pep-0020/#id3 (Explicit is better than implicit)
also read this article (https://blog.goyello.com/2013/05/17/express-names-in-code-bad-vs-clean/)
as $userId and $currentUserId wasn't clear enough what's the role of each id and made me confused when i was reading the code .
one last thing , i recommend this book especially first six chapters
https://www.amazon.com/Clean-Code-Handbook-Software-Craftsmanship/dp/0132350882
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that explicit is always better than implicit (and I've already read Clean Code!), but I think in this case it's pretty explicit (in my opinion):
function loginAsUser($userId, $originalUserId)
Does that cause confusion for you? Again, I'm not disputing, just wondering because it reads pretty expressively for me.
If anything (for me at least), naming it $sudosuUserId
would cause me more confusion because I'd assume that a sudosuUserId
is different to a regular user ID. Maybe that's just me though?
I'd be interested to get another person's thoughts on this too. /cc @krve
Either way, thanks for the PR @Te7a-Houdini, I love when people contribute like this :) Appreciate it! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the need to name it $sudosuUserId
considering that $userId
makes perfect sense in the given context.
Currently I don't think the code has any way in which it could be misinterpreted.
$this->session->put('sudosu.has_sudoed', true);
$this->session->put($this->sessionKey, $originalUserId);
$this->auth->loginUsingId($userId);
I think $userId
seems explicit enough.
EDIT: If you want it to be more clear about the parameter I think a descriptive comment would be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for the insight @krve. I'm going to rename the parameters back to what they were, but if you still think they cause confusion, feel free to open a separate PR and we can have a more in-depth discussion!
Reference issue #5 for commit 3f8e94e
Modify login variable names to be more explicit (commit a2a77e6 )