Hacker News Reader

Comment by saboot | Hacker News Reader

Perhaps an alternative would be

  valid_user = loggedIn() && hasRole(ROLE_ADMIN)
  if (valid_user) {
    valid_data = data != null && validate(data)
    if (valid_data) {
      ...
    }
  }

View on HN

For whatever reason, I'd prefer comments to this version. Note: I actually agree with the OP about pulling the logic into named conditionals whenever possible, but in the case you do want the short-circuiting behavior I would not bother with the variables at that point.

  if (loggedIn() && hasRole(ROLE_ADMIN)) {
    // User has permission to do this
    if (data != null && validate(data)) {
      // Submitted data is valid
      ...
    }
  }

I think part of the reason for doing it the way they do in the article is to reduce nesting. I saw the problem with missing the short circuit as well though which is why I like the suggestion of using

  if (isValidUser() && isValidData(data))

And checking for data being null in the isValidData method. There is a little overhead in the call and return if data is null, but the clarity provided seems like a win in these kinds of cases.

I prefer code over comments

  userHasPermission = (loggedIn() && hasRole(ROLE_ADMIN))
  if (userHasPermission) {
    dataIsValid = (data != null && validate(data))
    if (dataIsValid) {
      ...
    }
  }

That's shorter, introduces terms in reading order (readers do not have to wonder what if (loggedIn() && hasRole(ROLE_ADMIN)) means before encountering userHasPermission. Yes, you can write the comment before the if statement, but then, it tends to become longer: "check whether the user has permission to do this") and less likely to become inconsistent when code evolves.

I see such comments as symptoms of time pressure or a somewhat sloppy, but caring, programmer.

That's an interesting trick, but it does create a whole bunch of throwaway booleans. Still, I can see that if the situation gets really complex breaking it down like this can be a benefit (but not so much in the current example where it actually increases cognitive load because you will have to read more lines to figure out what is really going on rather than what the boolean name indicates, for instance, userHasPermission does not fully cover the load and if validate would return false on being sent 'null' for that data element then you could get rid of the flag altogether).

Reply on news.ycombinator.com