search

calband / calchart-viewer

An online show viewer and continuity generator for Calchart.
calband.github.io/calchart-viewer/
2 stars 2 forks source link

Remove redundant ternary operator #164

Closed jchou closed 9 years ago

jchou commented 9 years ago

Ternary ifs may be redundant when the expected return value is also a boolean. Not sure about benefits of strict string comparison here.

brandonchinn178 commented 9 years ago

:+1: I like this, although I think strict is better... something @noahsark769 was just strict about

jchou commented 9 years ago

I almost always tend to prefer strict comparison as well. Strings are an exception, though: the object "west" is not equal to the string primitive "west" used here. Not sure why anyone would pass in a string object rather than primitive in instantiation, but you never know ¯(ツ)/¯ just thought it might cause some confusion. Thoughts?

brandonchinn178 commented 9 years ago

What do you mean the object "west"? The only time this constructor is used is passing in a String in PDFGenerator.js:63

noahsark769 commented 9 years ago

Personally I like strict comparison also, but I can see the point that new String("hello") !== "hello" would be confusing. By the object, you mean a new String("hello") right? In that case, I can actually get behind this one.

jchou commented 9 years ago

Yeah, the value of the corresponding "West up" / "East up" radio is passed into the constructor so it works fine as it is used now, but from a modular perspective, I'd say that, in the current state of using certain strings to determine the behavior of the constructor, new String("west") and "west" should be considered the same. Yep, that's what I was referring to, Noah. Supes minor thing, I was just randomly perusing calchart-viewer ;P

brandonchinn178 commented 9 years ago

Yea sure, I'm fine with this. Welcome to the code, Jack!

kmdurand commented 9 years ago

It's cool to get a new contributors on GitHub. Shipping!