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.

StackLloyd
  • 109
  • 1
  • If a method that is supposed to return a `List<>` ever returns `null`, I'd consider the implementation broken. – Paul Kertscher Jun 17 '19 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 '19 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. – Graham Jun 17 '19 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. – Paul Kertscher Jun 17 '19 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 '19 at 13:25
  • If we write defensive code, it should be *tested* like any code we write. Thus, this defensiveness adds bloat both to the code itself and the engineering process around it. – Erik Eidt Jun 17 '19 at 15:48
  • @PaulKertscher in my example, I refer to the List property as optional, thus throwing an exception would not be correct. I do not personally prefer exception-driven flow, I prefer to return more complex Result objects from method calls, where the error messaging is included in the Result structure. But certainly a lot of folks (mostly coming from the Java world) like using exceptions in the manner you suggest. – Graham Jun 17 '19 at 17:39
  • Your team needs to decide when, if ever, nulls are allowed. If a null is a valid return value, you need to handle it. My preference is to treat every null as a bugg, and use Maybe/Optional for optional values. This means I never check for null, if there is one, the program is faulty and should crash. But the important thing is that it is perfectly clear for everyone using the code what values are allowed. – JonasH Jun 18 '19 at 07:00
  • If null is a valid state of a value (read `value = null` will compile), then your code must handle it. That being said you only need to do this at the boundary, assuming the code in your system is trust worthy... – Kain0_0 Jun 18 '19 at 07:36
  • A colleague told me I'm just writing redundant code. The caller should sanitize the parameters before passing them to the method, so the method should work assuming they are valid. The method does not and should not ever return null, so the caller should work with the return value assuming it is valid. If something breaks, that is a programming error. I only partially agree, as I find this approach fault-prone. Maybe I only want to notify the user that an error occurred, instead of letting the application crash because of an unexpexted NPE. – StackLloyd Jun 19 '19 at 12:55

0 Answers0