3 ms·

> Just because one field tells you something should be there doesn’t mean an extra null-check would hurt. IMO, it would. NPE exists for a reason: to instantly

> Just because one field tells you something should be there doesn’t mean an extra null-check would hurt. IMO, it would. NPE exists for a reason: to instantly alert you to a bug and/or contract violation. Maybe I misunderstood the author's intent here, but sweeping the contract violation under the rug with a superfluous null check doesn't seem to help anything here. If successMsg is null, what are you going to do, display "" instead? Often, immediate termination with NPE seems to be the best approach in cases like this, IMHO.

lostcolony 5y ago
That assumes the read of the field will happen immediately after this class is instantiated. Which is, of course, not likely to be the case in real code. Which is why NPEs are inherently problematic. If something is null, and it's being read, it means an implicit contract violation happened -before- the read (i.e., that field should have been set somewhere), and you -have no idea where-. Better to fail fast with a cleaner message when this ErrorResponse object is created, than when the null message is attempted to be used somewhere further down the call stack and you have no idea why it's null. Or more succinctly, "to instantly alert you to a bug and/or contract violation" is not what NPEs do. They alert you when you attempt to read a value that has not been set; they do not alert you when you failed to set a value you should have set. If you have a constructor and something should not be null in it, at least assert that it is not null so it fails fast; don't wait for it to be read who knows how much later and fail only then.
- trav4225 5y ago
  Oh, agreed 100%. Thanks for the clarification. This is indeed what I do. I heavily enforce contracts (something some of my coworkers do not appreciate ;-). Perhaps we don't agree on everything here, though, because I still do not see any problems with the existence of NPEs. However, I do appreciate language features that allow you to enforce non-nullability at compile time if desired.
bradleybuda 5y ago
> If successMsg is null, what are you going to do, display "" instead? Congratulations on passing the Enterprise SaaS coding question, we'd like to make you an offer! When can you start?
karagenit 5y ago
I hear what you're saying. In this specific case, we had a default success message string that would normally be displayed if a custom message wasn't provided, so it would've been fine. In general, however, we like to avoid any uncaught/unchecked exceptions whenever possible. This is for two reasons: first, when we get an NPE, the user gets a generic "Oops, something went wrong" type error message, which is useless for them when they call support and don't have an actual error code; second we have a pretty extensive in-house error logging framework that we prefer to report errors on, since it makes debugging the issue easier and gives us aggregate data on what parts of the code have been reporting more issues lately (and probably need a rewrite, like in this case). Thanks for reading!
- trav4225 5y ago
  Makes perfect sense. Thanks!

lostcolony 5y ago

That assumes the read of the field will happen immediately after this class is instantiated. Which is, of course, not likely to be the case in real code. Which is why NPEs are inherently problematic. If something is null, and it's being read, it means an implicit contract violation happened -before- the read (i.e., that field should have been set somewhere), and you -have no idea where-. Better to fail fast with a cleaner message when this ErrorResponse object is created, than when the null message is attempted to be used somewhere further down the call stack and you have no idea why it's null. Or more succinctly, "to instantly alert you to a bug and/or contract violation" is not what NPEs do. They alert you when you attempt to read a value that has not been set; they do not alert you when you failed to set a value you should have set. If you have a constructor and something should not be null in it, at least assert that it is not null so it fails fast; don't wait for it to be read who knows how much later and fail only then.

trav4225 5y ago
Oh, agreed 100%. Thanks for the clarification. This is indeed what I do. I heavily enforce contracts (something some of my coworkers do not appreciate ;-). Perhaps we don't agree on everything here, though, because I still do not see any problems with the existence of NPEs. However, I do appreciate language features that allow you to enforce non-nullability at compile time if desired.

bradleybuda 5y ago

> If successMsg is null, what are you going to do, display "" instead? Congratulations on passing the Enterprise SaaS coding question, we'd like to make you an offer! When can you start?

karagenit 5y ago

I hear what you're saying. In this specific case, we had a default success message string that would normally be displayed if a custom message wasn't provided, so it would've been fine. In general, however, we like to avoid any uncaught/unchecked exceptions whenever possible. This is for two reasons: first, when we get an NPE, the user gets a generic "Oops, something went wrong" type error message, which is useless for them when they call support and don't have an actual error code; second we have a pretty extensive in-house error logging framework that we prefer to report errors on, since it makes debugging the issue easier and gives us aggregate data on what parts of the code have been reporting more issues lately (and probably need a rewrite, like in this case). Thanks for reading!

trav4225 5y ago
Makes perfect sense. Thanks!