Get rid of unnecessary String.isEmpty() checks

[HEdingfield]

Finish resolving conversation in #126, which will require additional validation checks:

HEdingfield commented 2 days ago

Thanks, @tarheel, I'll go through the 7 unnecessary instances of using isEmpty() and revert them in a future commit. Is it mainly the slight performance hit you're worried about here? I was moved to do it because I found 1 or 2 genuine bugs caused by not doing the isEmpty() check, since it's possible to have "" in a config file. I like the idea of changing the getters to return null if the string is empty... I think that's similar to what you were suggesting?

tarheel commented 22 hours ago

Zero concern about perf! It's more readability. I find it confusing when the code checks for a case that we already know should be impossible. If the consequence of our assumption being wrong is really bad, perhaps we add an assert, but otherwise I think it just muddles things for someone working on that code in the future.

I would prefer to catch all these issues at validation time. If the empty string isn't valid, disallow it then -- and whenever we reference the value after that point, assume it's not empty.