-
Notifications
You must be signed in to change notification settings - Fork 132
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
Fix Issue #144 - Entity isSwimming function #171
base: barony-next
Are you sure you want to change the base?
Fix Issue #144 - Entity isSwimming function #171
Conversation
Update to Latest Code
+ 5 blank spellbooks.
Barony next update to latest
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 like this better, apart from all the redundant == true
expressions. x == true
is the same thing as x
if it's a boolean! Likewise, !x
is nicer than x == false
.
I disagree on two main reasons.
1. It helps prevent ambiguous code. I don't need to guess what type of variable it is. It helps distinguish it from all of the "ints used as bools" used throughout.
2. Despite it making the statement slightly longer, I strive for readability over all else. I can easily tell which value is expected, and what the normal case might be from a glance.
I elected to do this after sifting through the code and constantly having to decipher these if statements. I can't remember off the top of my head where, but the turning point was a variable being used as `!(var)` but the usual, average case was the opposite from expected.
All in all, I found the code very unreadable. I opted for this alternative. Of course, this is not my call to make. If addictgamer would prefer it the other way, I'll update the PR.
null
|
This only really makes sense when the choice of variable names is poor — |
IOW, |
I suppose another reason was to make everything explicit. Most of the warnings Barony has right now is due to implicit conversions and such, and in general, making everything explicit is how I prefer to program. However, after further reading (I had previously done reading on this subject prior to implementing any conditional checks like this, and sided with explicit), I do believe that if the variables are well named, it should be fine. I'll update now. |
Added comments and renamed function to "IsSwimming()"
Sadly, this is an issue. Well-named variables should be preferred over the alternatives. |
I believe I already updated everything.
null
|
Yes, the issues were already handled. If you have any additional concerns, please let me know. |
Merge pull request #774 from WALLOFJUSTICE/dev-23-aug
This is a fix for #144.
The issue was there wasn't a function to test whether or not a given Entity is swimming. There is 'isLevitating()' and 'isInvisible()' but until now, all swimming code had to be repeated everywhere.
This also goes along with pull request #142. It should be used as a replacement for #142, if it is accepted. It implements the swimming checks added/modified in #142 with the new method added in this PR.
Additionally, this also has the fix for #143.