search

Maruno17 / pokemon-essentials

A heavily modified RPG Maker XP game project that makes the game play like a Pokémon game. Not a full project in itself; this repo is to be added into an existing RMXP game project.
Other
208 stars 398 forks source link

Fixing Nil and Empty String Checks #162

Closed Golisopod-User closed 2 years ago

Golisopod-User commented 2 years ago

This PR updates the checks for nil and empty strings from direct checks to method-based checks.

Marin-MK commented 2 years ago

Except for the .nil? changes, this seems rather pointless, and not a convention that developers will adopt either.

Golisopod-User commented 2 years ago

Except for the .nil? changes, this seems rather pointless, and not a convention that developers will adopt either.

Refactoring is never pointless.

Marin-MK commented 2 years ago

Refactoring is pointless when the changes you make don't improve code quality.

Golisopod-User commented 2 years ago

Refactoring is pointless when the changes you make don't improve code quality.

Using .empty? and .nil? instead of direct == checks for "" or nil does improve the code quality.

jonisavo commented 2 years ago

nil.empty? should be a bug. It degrades code quality. When I see x.empty?, I expect x to be a collection of some sorts, or other class like File depending on the context. A check for nil should happen before that and the special case handled. This just encourages more rampant usage of nil.

Plus, sweeping pull requests like this always make me a little antsy when we have no unit tests to check everything works.

Golisopod-User commented 2 years ago

nil.empty? should be a bug. It degrades code quality. When I see x.empty?, I expect x to be a collection of some sorts, or other class like File depending on the context. A check for nil should happen before that and the special case handled. This just encourages more rampant usage of nil.

Rails adds a method called blank? which does the same thing as empty? does here, but sure, I can just remove the new empty? method if you're worried about it. I've made sure to use empty? only in places where we know the value is not nil, but a String. If a nil value has to be checked then nil_or_empty? has been used, instead of just empty?, so it's a simple change. I'll remove it if Maruno doesn't approve of it.

Maruno17 commented 2 years ago

I don't want to be adding methods to core classes (def empty?) just for the sake of some questionable refactoring. It doesn't add or condense anything (.empty? is one character longer than == ""), so I don't see a reason to have it.

It seems you haven't thought about what you were changing either. For example, if !self_event.nil? should really just be if self_event because it'll only ever be nil or a Game_Event and will never be false. There are several such examples, another of which is (!target.nil? ? target : user) which should just be target || user. You've also ended up with some cases of if !x.nil?... else, which is its own bad practice.

I would also argue that if !x.nil? isn't necessarily better than if x != nil. The latter translates better to English ("if x is not nil") than the former ("if not x nil"), and x could be a long phrase which puts a big gap between "not" and "nil", making it harder to comprehend.

This whole PR seems like change for the sake of it, not change because it's useful. If you thought about the changes as I mentioned in my second paragraph, then it'd be useful because it would actually be tidying up the code. At the moment you're just rearranging the mess, not to mention occasionally over-condensing the code like in Event_Handlers line 234 (which now looks like a jumble).

Golisopod-User commented 2 years ago

empty? is the same character length as == nil, which is what I'm trying to replace here. And x.empty? is shorter than nil_or_empty?(x) too. It's also a faster check than == nil. This is also a utility that Marin encourages the use of, because it's a function included in his scripting utilities. But if you still want it removed, then I'll remove it.

The rampant use of if !x has stemmed from you being peeved about using unless mostly. I'd be very happy to change alot of instances of if ! to unless. But I don't know if you'll approve of that, considering you're also against the use of nil? because of translation to English or whatever and are completely ignoring the fact that .nil? is almost two times faster than == nil in most cases.

The aim of this PR was to eliminate direct nil and "" checks, because using method checks has been benchmarked to be faster. I haven't made these changes "for the sake of changing them", I changed it to further optimise the code. I know that you feel intimidated by large pull requests, so I tried to keep my changes as close to the original code as possible. But if you're looking for a more thorough refactor, then I'll be very glad to mark this PR for review and continue refactoring. It was high time someone took the initiative to do so.

Maruno17 commented 2 years ago

My comments stand.

And I do dislike unless, and I don't want it used. But it seems you missed the point of what I was saying, if that's how you chose to respond to it.

The speed of evaluating == nil versus .nil? is clearly inconsequential. They're both fast enough. This shouldn't be used as an argument for these changes. We're not just writing code that runs quickly; we're writing code that's user-friendly. Given that speed is not a factor here, I'll judge the code on whether it looks good, and I've somewhat voiced my opinion on that already.

I would have made these changes myself already if I thought they were important. It's nice that someone other than me is contributing to Essentials, but as I said, it seems it was done blindly and without consideration of what the code actually does.