Danger: Overloaded Constructors Ahead
calebwrites.substack.com
calebwrites.substack.com
Hell, you could even use a builder pattern (and make the constructor private), which is pretty common in Java-land:
final ErrorResponse errorResponse = ErrorResponse.newBuilder()
.withSuccesses(1, "Great!")
.withWarnings(3, "Hmm...")
.withErrors(1, "Uh oh")
.build();
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.)
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.
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.
P.S. thanks for reading!
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.
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.
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.
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.
Congratulations on passing the Enterprise SaaS coding question, we'd like to make you an offer! When can you start?
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!
Surely
> public ErrorResponse(int successCount, int errorCount, String errorMsg, String warningMsg);
> public ErrorResponse(int successCount, int errorCount, String errorMsg, String warningMsg, String successMsg);
would cause fewer problems than
> public ErrorResponse(int successCount, int errorCount, String warningMsg, String errorMsg);
> public ErrorResponse(int successCount, int errorCount, String successMsg, String warningMsg, String errorMsg);
> 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.
Thanks for reading!