1

I am peer reviewing code written in vanilla javascript, and one of the authors occasionally performs a lazy-typecast if statement because he is evaluating values from a REST service that has very sloppy data. Occasionally a value representing money in USD comes back as a string, and sometimes as a number, probably depending on what validation was in place at the time the user entered the data over the years.

So sometimes we get a value of 150 and sometimes we get "150" (without the quotes, if you know what I mean).

The data is just displayed, and there are no math operations done against it: it simply needs to be compared to another number so some display styling can be done.

Instead of writing some function that checks the type of data and forces it to be a uniform type, the developer does something like this:

var baseVal = ReturnsAnIntegerValue(); // returns a number
var retrievedVal = CallToAwfulDataService(); // could return number or string
if (retrievedVal && retrievedVal == baseVal) {
  // do stuff
}
else { ...

At first the == made me see a red flag, but when I asked him about it, he said that the initial check for if (retrievedVal && ... would prevent unexpected outcomes from situations where baseVal and retrievedVal are each some combination of null, 0, empty string, or undefined, like this:

if (null && null == undefined) // false       |     if (null == undefined) // true
if (0 && 0 == "") // false                    |     if (0 == "") // true
if (0 && 0 == "0") // false                   |     if (0 == "0") // true
if (0 && 0 == "") // false                    |     if (0 == "") // true

You get the idea. I feel like this should be bad code because I was taught that the == is bad form, but the code is readable and concise, without the need for an extra function that type-converts just for show. I can't think of a scenario where we are likely to be hurt by it.

Is this bad code? Specifically: what is wrong with it? Is there a better practice for handling this?

edit

I forgot to mention, I have looked at a bunch of SO questions that mostly seem to back up the == is code smell school of thought, but I can't see what is technically risky about this scenario.

Community
  • 1
  • 1
tengen
  • 2,125
  • 3
  • 31
  • 57

1 Answers1

0

You should probably have some "normalize" function, and test cases with data samples from the backend.

Good practices generally assume that "all is fine" with the environment, which isn't your case.

Which ever way you go, you're doomed as far as "good practices" are concerned.

sabof
  • 8,062
  • 4
  • 28
  • 52
  • I agree... as I said, I felt this approach was not standard. When I asked the developer about it, his response was "Tell me how this can break. Why replace 1 extra comparison operator with an entire function?" I am at a loss to respond. That's what I am looking for here. If this is wrong, tell me how it can break. – tengen Feb 25 '14 at 16:43
  • The operation is complex (no matter if it's implemented with a single operator) and there might be things about the data you didn't anticipate (commas in the string? dots? letters?). I think any code with such characteristics deserves a separate function. – sabof Feb 25 '14 at 17:05
  • If there are no commas, letters, etc. the code might still be "too clever". – sabof Feb 25 '14 at 17:07
  • By "too clever" do you mean the code is making assumptions about the data it processes? – tengen Feb 26 '14 at 17:08
  • No, I just mean that it could be written in a way that would be better understood by most developers. – sabof Feb 26 '14 at 17:37
  • @TMcKeown I'll accept this answer as I imagine it's right from a best practices perspective (it does remove uncertainty to just process all the data, necessary or not), though I like your comment about readability and simplicity. I dislike approaches that "overcode", and I really like the simplicity of the code in question. Processing data or excessively testing for nulls/etc, shows a lack of faith in the contracts your code is making internally, IMHO. Sabof, thanks for taking the time to help, I appreciate it. I have benefited from your answers around SO in the as as well. – tengen Feb 27 '14 at 15:20