Oof, the lesson that should have been learned here is that this is an inherently unsafe way to design the API of this class. While Java's type system isn't the most expressive in the world, certainly you could design something that makes it impossible to construct in a way that you'd ever see this failure.
Hell, you could even use a builder pattern (and make the constructor private), which is pretty common in Java-land:
If you have no warnings (for example), you don't call `.withWarnings()`, and then you have zero warnings and no message to go with it. If you have warnings, you call the `.withWarnings()` method and you must provide both bits of information.
Yes, it's more verbose. But that's Java for you, and the key takeaway is that it's impossible to construct an `ErrorMessage` with invalid internal state (assume that the `.with*()` methods have proper preconditions, etc.).
In addition, `.getSuccessMsg()` etc. should return `Optional<String>`. Alternatively, you could just have `.getSuccesses()`, which returns something like `Optional<ResponseData>`, which is just a poor-man's 2-tuple of (numSuccesses, successMsg). (And if you're using a recent Java version [which I suspect Salesforce is not], you can even use a record to save you the boilerplate.)
Absolutely, there are definitely many ways this could've been prevented (such as the builder pattern you mentioned).
I guess the point of the article is to show that many bugs I've encountered aren't due to a bad initial design but because of additional requirements that get added later, resulting in new code that doesn't play well with the old code.
Additionally, developers are generally trying to Get Things Done (and quick), so a lot of times you have to make a situational call to either 1) just modify the existing code to add the new needed functionality and accept the tech debt, or 2) spend extra time rewriting the whole class. There aren't really any good metrics (that I know of) that can definitively tell you when it's time for #2, I guess it's just intuition based on bad past experiences (like this one)?
I remember having access to the new (ish) Time API and Streams and stuff, so it was definitely at least Java 8, but alas no Records.
> many bugs I've encountered aren't due to a bad initial design but because of additional requirements that get added later
I only sorta agree with that. The initial design here wasn't great, either. While the multiple constructors likely more or less handled the various cases properly, it was still a recipe for future mistakes. And that's the kind of thing I try to drill into the heads of junior developers during code reviews. Instead of solely thinking about the problem at hand, they need to think about how the code they write might need to be changed in the future, and guard against possible mistakes and misuse. No one will ever be perfectly prescient, but we can at least try. Sometimes there's no substitute for experience here, though.
I do agree that additional requirements added later can put us in a bad spot. But I think if we examine why that is, though, we have to demand more rigor when these kind of updates happen. I get that there's often some urgency to adding these sorts of things, but that update would never have passed review by me. I would have insisted that the code be refactored so constructing that class would have been safe by design. I get that it's not really the "fault" of the person making the change ("oh, the code was like that already; I'm just adding something to it"), but that's not really the point. The changes made an already not-particularly-safe interface and made it even more unsafe to the point that the change actually broke things.
The whole point of good object oriented design is that your class interfaces make it impossible to produce an invalid state of an instance. If you can produce a sequence of mutating or initialization method calls that leave a class with broken invariants then your interface needs work.
Incidentally, this is why the common "a square is a rectangle" example is usually broken. Calling setX() on a rectangle does not break invariants but calling setX() on a square does.
> 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.
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.
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.
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).
They're a really bad idea, so much so that's it's the first item in Effective Java:
> Consider static factory methods instead of constructors
The reasons outlined in Effective Java include:
- Unlike constructors, static factory methods can have names.
- Unlike constructors, static factory methods are not required to create a new object each time they're invoked (immutable objects, flyweight pattern).
- Unlike constructors, they can return an object of any subtype of their return type.
- Unlike constructors, the class of the returned object can vary from call to call as a function of the input parameters.
Effective Java even has advice for overloading: Item 52, "use overloading judiciously"
> A safe, conservative policy is never to export two overloadings with the same number of parameters.
I prefer stronger guidance for overloading that I'll paraphrase poorly from a forgotten source (Guava maybe): each overload should return pretty much the same thing and it should be obvious from the call site. The OP's article violates this principle since warnings should be distinct from errors.
Oh, absolutely! The problem was that different parameters were added at different times (by different developers), and they weren't careful enough to realize they were doing it incorrectly.
Agreed, it would be nice if our linter supported a rule for this sort of thing (maybe something like this exists already)? Right now it's just a judgement call and people make mistakes :)
I am more surprised to see that Java didn't raise a compile time error for trying to read the value of an argument that doesn't exist in the method signature (successMsg).
Yeah, I was a little surprised too - I think because there were two constructors the compiler couldn't be certain whether an instance of the class did or didn't have the field initialized?
Hell, you could even use a builder pattern (and make the constructor private), which is pretty common in Java-land:
If you have no warnings (for example), you don't call `.withWarnings()`, and then you have zero warnings and no message to go with it. If you have warnings, you call the `.withWarnings()` method and you must provide both bits of information.Yes, it's more verbose. But that's Java for you, and the key takeaway is that it's impossible to construct an `ErrorMessage` with invalid internal state (assume that the `.with*()` methods have proper preconditions, etc.).
In addition, `.getSuccessMsg()` etc. should return `Optional<String>`. Alternatively, you could just have `.getSuccesses()`, which returns something like `Optional<ResponseData>`, which is just a poor-man's 2-tuple of (numSuccesses, successMsg). (And if you're using a recent Java version [which I suspect Salesforce is not], you can even use a record to save you the boilerplate.)