0

I'm new to software engineering and right now I'm focused on learning the best practices to consistently write robust code. Recently I've been maintaining an application built by other people and/or my colleagues before my arrival at the company. While writing my code, I make sure of what I'm dealing with and read the "foreign" code I'm bound to use. Sometimes I find that a method is written not to return a null value. Such occasions eventually sprouted a question in my mind: should I be checking for null anyway? What if the method I'm using gets changed by a colleague and, from that point onwards, is made capable of returning null? I'm getting quite paranoid, to the point that I do such checks everywhere I use someone else's methods to get an object I need and even do so with the methods I wrote myself. What if someone else touches them? We're a team working on the same things, after all. A trivial example:

// GetData() never returns null, as far as I know, as of now,
// and an empty list is a valid value for itemsList
List<BusinessItems> itemsList = ColleagueClass.GetData() ?? new List<BusinessItems>();
BusinessItems[] itemsArray = itemsList.OrderBy(item => item.CustomerName).ToArray();

Without the null-coalescing operator to assign a default valid value, if somehow GetData() starts to return null, this code breaks. I'm sprinkling ?? and ?. almost everywhere this situation could happen in the future, if something gets changed at some point, unbeknownst to me. I'm concerned this might be code cluttering due to paranoia, instead of robust writing.
May I please ask for your opinions?

EDIT: Since this question has been flagged as a duplicate, I want to make it more specific, because I don't find the answer I'm looking for in those indicated as already answered.
What I'm doing with my code is checking for something that simply doesn't happen... Right now. The methods against which I'm defensive just never return null and I'm not accessing a database. Therefore, as of now, all the checks I'm doing are pointless. What I'm doing (I think, I'm not sure, hence the question) is prevention: "what if the method I rely on gets changed at some point and can return null?". The answer to this hypothetical question is almost always that null can be interpreted as the absence of a value (Java implements Optional<T> exactly for this) and the workflow could proceed accordingly without throwing, e.g., an ArgumentNullException.
Another example, hoping to clarify:

// GetBusinessObject() never returns null, but what if it gets changed? 
// null can still be a valid value for orderId
BusinessObject businessObject = ColleagueClass.GetBusinessObject();
string orderId = businessObject?.OrderID;

So, is this "prevention" that I'm doing just paranoia-driven code cluttering, as I'm afraid it is, or am I doing a good job by preventively designing the workflow to handle potential changes in the code?

I hope I explained my doubts well. I'm sorry if I couldn't, I'm a newbie scared of doing a bad job.

10
  • If a method that is supposed to return a List<> ever returns null, I'd consider the implementation broken. Jun 17, 2019 at 12:20
  • Indeed, maybe in that case throwing an exception might be the best option. Why would you return null instead of an empty list? That can be the sign of a sever error.
    – StackLloyd
    Jun 17, 2019 at 12:53
  • There are some cases where NULL is more meaningful than an empty list. For example, if the list is just one of several properties on the returned object, NULL might indicate that the parameters provided did not include enough info to fetch the optional List property. You can think of it like NULL = 'I don't know' and EMPTY = 'none'. @StackLloyd, your instinct to NULL-check everything is totally valid, but as you've seen, it can clutter up your code. I tend to check for NULL at application boundaries, but once I'm in my app, I don't NULL check every time I pass a param around.
    – GHP
    Jun 17, 2019 at 12:58
  • @Graham Sure, but in that case, throwing a InsufficientDataProvidedException may be more meaningful than just returning null. Unless the method indicates that "I don't know" is an option it should not return a value that means that it does not know. Jun 17, 2019 at 13:09
  • 1
    I think that silent fails are to be avoided and if a value shouldn't be what it is, then an exception must be raised and/or the workflow must be halted to make it clear what happened. On the other hand, if the value is legit, though it needs to be handled differently, then the workflow should proceed accordingly. However, I might be overdoing it. Maybe I should just let it be and, if something gets changed in the future and this causes a breakdown, so be it, we will fix it...
    – StackLloyd
    Jun 17, 2019 at 13:25

0

Browse other questions tagged or ask your own question.