|
Undefined is... undefined. Yes, but not in the way you put it.
For example (written it in more replies here) a function that returns orders for a given customer number:
- If the customer is unknown (invalid customer number), the customer's orders are unknown, i.e. undefined. The list of orders is then undefined (null).
- If the customer is known, but has no orders, the list of orders is an empty list.
Very simple, situation dependent.
Peter the small turnip
(1) It Has To Work. --RFC 1925[^]
|
|
|
|
|
Returning null when the customer doesn't exist is extremely poor programming.
For you to even get to a point in code where you are using non-existent customers is the problem.
You should instead be raising an exception because a method designed to return orders for a customer is NOT expecting the customer to not exist
|
|
|
|
|
Yeah, in production code used by many colleagues, sure I should have a function like GetOrdersForCustomers() that throws an exception and TryGetOrdersForCustomers() that returns null (the implementation would be in the latter, and the null check in the former - for performance reasons - exceptions are extremely expensive).
Returning null (or whatever return value signalling an error) is the old school C convention, whereas exceptions is preferrable today, I know that.
But... if for some reason you're writing stuff for use by a single customer in a relatively small project (as I mostly do), I go for the "return null" case in places where it makes sense, just to save me the time of writing an extra wrapper method that checks for null and throws. My colleagues (mostly web designers) don't really care if I throw the exception or if their code does.
So again, it's situation dependent. And audience dependent. And a matter of taste.
Point of annoyance: CultureInfo.GetCultureInfo(name) throws a CultureInfoNotFoundException if the name is invalid. Thanks a lot. I REALLY need to catch an exception there (with a humungous performance hit). If a "Try..." version exists (which it should, admittedly), by all means, throw an exception. If not, return null. Please!
EDIT: Situation dependent, yes. My colleagues are, as I said, web designers, so my code is mainly used in Razor, which is essentially a scripting language. They are more comfortable with all methods being "Try..."-methods, as it enables them to try to get stuff based on user input (query strings or whatnot) without pre-cleaning (the call itself does the cleaning, and returns null if something's wrong). They know I always protect my code against crazy stuff - it's easier for me than it is for them. And a null check and an appropriate response is easier for them to implement than a try/catch (they don't really know what try/catch is, I think - and after all, it doesn't belong in a Razor view, it belongs in the controller). And cheaper. So again, situation dependent.
But yes, I agree. A production-class library shared by many - throw on invalid input and return null only if the method starts with Try. Sure, I'm an old fart from the C times, but I can learn new stuff, too... and yes, I love exceptions...
EDIT SOME MORE: And of course, the not-so-hypothetical DataLayer.GetOrdersForCustomer method would in most cases only be called by me, from the Customer.Orders property, which, when lazy-loading, of course initializes itself with an empty list of the Get...-call returns null (which it will, if it is a newly created, unsaved, customer - and the null-returning saves me an extra check to see if that is the case - a ?? does the job). That is DEFINITELY a situation where I would NEVER return null.
Peter the small turnip
(1) It Has To Work. --RFC 1925[^]
modified 5-Nov-14 15:51pm.
|
|
|
|
|
PeterTheSwede wrote: But... if for some reason you're writing stuff for use by a single customer in a relatively small project (as I mostly do), I go for the "return null" case in places where it makes sense, just to save me the time of writing an extra wrapper method that checks for null and throws. My colleagues (mostly web designers) don't really care if I throw the exception or if their code does.
Then that is a choice to cut corners and ignore good practice... how then does that relate to the topic when you are knowingly going against conventional wisdom for the sake of doing a quick and dirty?
PeterTheSwede wrote: So again, it's situation dependent. And audience dependent. And a matter of taste.
Actually I couldn't disagree more. The only situational dependence is your choice to ignore what you know you shouldn't ignore for its own sake. You are making it sound as if the situation itself dictates it due to some efficiency when its not... You are making a choice to code poorly because its "a single customer in a relatively small project". Don't blame the best practice for a personal choice you make to not use it
PeterTheSwede wrote: Point of annoyance: CultureInfo.GetCultureInfo(name) throws a CultureInfoNotFoundException if the name is invalid. Thanks a lot. I REALLY need to catch an exception there (with a humungous performance hit). If a "Try..." version exists (which it should, admittedly), by all means, throw an exception. If not, return null. Please!
What are you doing trying to type your own names into culture info? And why would you be upset about a huge performance hit when something EXCEPTIONAL has happened? Thats the point of exceptions, they are exceptional and happen because the intended flow of the application has been interrupted by something it wasn't expecting... so of course it should do exactly this. Again that is just poor coding if you are going to let any old random string into that method. Now if you are saying there needs to be an additional method which looks up the internal collection of cultures instead of bailing out then by all means, that sounds like a good reason to have the function, but code should NEVER expect to run with exceptions as if they are expected.
PeterTheSwede wrote: DIT: Situation dependent, yes. My colleagues are, as I said, web designers, so my code is mainly used in Razor, which is essentially a scripting language. They are more comfortable with all methods being "Try..."-methods, as it enables them to try to get stuff based on user input (query strings or whatnot) without pre-cleaning (the call itself does the cleaning, and returns null if something's wrong). They know I always protect my code against crazy stuff - it's easier for me than it is for them. And a null check and an appropriate response is easier for them to implement than a try/catch (they don't really know what try/catch is, I think - and after all, it doesn't belong in a Razor view, it belongs in the controller). And cheaper. So again, situation dependent.
If that is the case your methods should return a boolean indicating success or a result object that encapsulates both success, a fail reason and whatever returned entity you are expecting.
Yet again... don't blame empty lists vs nulls because you are choosing to cut corners in how you write the method. There are a thousand other ways to do it.... the point that is being made here is that the syntax of a method states that it will return a LIST... therefore it should return a LIST under ALL circumstances. The fact that lists are objects and objects can be null is unrelated to how functions that return lists should work.
|
|
|
|
|
You said it yourself. A list is an object. If it's undefined/unknown, it's null (which means undefined/unknown). If it's empty, it's empty. Big difference. Simple as that.
And yes, again, I agree that a TryGetOrdersForCustomer(number) that returns null for invalid customer numbers and a GetOrdersForCustomer(number) that throws an exception would be preferrable. But I wouldn't call it lazy, just budget-realistic. Some projects don't warrant being pedantic... seriously... the customer just won't pay for it, and we all need a salary...
But let's dwell a bit on GetCultureInfo:
- Yes, the list of accept-languages from a browser cannot be trusted, so it is perfectly in order to have a culture name that is invalid. For example. Hundreds per second, even - if a browser brand released a broken version.
- If there was a TryGetCultureInfo method, I wouldn't complain. Returning null. This would serve as a one-stop-shop for validation and getting the information I need.
- If there was a CultureInfo.IsCultureDefined(name) method and no TryGetCultureInfo, I would still complain. Because "var x = CultureInfo.IsCultureDefined(name) ? CultureInfo.GetCultureInfo(name) : null;" causes TWO lookups (and of the performance of these, we know nothing) against some list/dictionary, whereas the TryGetCultureInfo of course would only cause ONE.
- Yes, I can enumerate all existing cultures and build my own list and my own logic, but nevertheless... I consider it broken as is - I shouldn't have to fix it (I do, of course, because I have to, but that's off topic).
OK, so a CultureInfo is not a list. But like I (and you) said, a list is an object, so the comparison is valid. And of COURSE, a GetInstalledCultures() (or whatever it's name is) should always return a list. Because it is never unknown/undefined, although it may be empty. See the difference? And the LinQ Where() method should of course always return an IEnumerable, because null would be semantically incomprehensible (in THAT context).
Returning a boolean and having an ugly out parameter (OK, getting better i vNext)? Nah. Too much code to write for the colleagues. And for me. Ugly.
My rationale for returning null for a nonexisting customer:
- If my colleague validates the customer number, he can safely iterate the list, because it always is a list.
- If he doesn't, he can do a null check (which would serve as a customer number validation), or get bitten.
If he doesn't validate the customer number and doesn't check for null, the only difference is where the exception gets thrown. NOTHING else.
Peter the small turnip
(1) It Has To Work. --RFC 1925[^]
modified 5-Nov-14 17:42pm.
|
|
|
|
|
You completely missed my point.
I specifically said that the fact a list is an object and objects can be null does NOT mean it is valid to return null when a signature indicated it returns a list.
Yet you continue saying that I said otherwise?
Can you not see THAT is the issue? People confuse the fact that something is possible with it automatically being valid... Its not.
|
|
|
|
|
Its a little hard responding when you keep editing
PeterTheSwede wrote: But I wouldn't call it lazy, just budget-realistic. Some projects don't warrant being pedantic... seriously... the customer just won't pay for it, and we all need a salary
But this is a decision not based on best practice but on economics. It is an admission of cutting corners and that isn't what this article is talking about. It is trying to validate an alternate "best practice" approach of doing this in ALL cases... not just the low budget ones.
So I think inclusion of a choice to cut corners (never said lazy), doesn't negate the logic of best practice and it certainly isn't the situation dictating that practice... its dictating whether you ignore it or not.
So essentially what you are saying is that you ignore the best practice of returning an empty list because a customer wont pay for that one extra line of code or those few additional nanoseconds it takes to write and handle an exception....
That kind of logic has no place in a dicussion about best practices and what are VALID approaches at a conceptual level (which again I keep saying this... is what the article is about)
PeterTheSwede wrote: My rationale for returning null for a nonexisting customer:
- If my colleague validates the customer number, he can safely iterate the list, because it always is a list.
- If he doesn't, he can do a null check (which would serve as a customer number validation), or get bitten.
If he doesn't validate the customer number and doesn't check for null, the only difference is where the exception gets thrown. NOTHING else.
Simply replace "If he doesn't then can do a null check" with "if he handles the CustomerDoesntExistException or get bitten" and again you have exactly the same thing.
But the primary point here is that returning a null has an unknown implication. YOU have decided that means the customer does't exist, but what if the database wasn't reachable? Do you still return null? Do you raise exceptions for that one? So now you have 2 different methods of indicating to the caller the state of what is happening instead of one.
Not only that, but your choice flies in the face of convention. If a file didn't exist, I dont get a null, I get a FileNotFoundException... why? Because null just isn't informative enough. It could have been permissions, hence I would get an AccessDeniedException.
But you are clearly missing the point of "encapsulation"... For you to write a function thinking about how that function may or may not be used OUTSIDE, or whether they will or will not make a check and trying to make your response "seem" to fit the one tiny exceptional case is just plain wrong.
If the function says that it will get you the orders for a customer, then that is exactly what it should do. If there is a problem doing it then it should raise an exception. You yourself said that it is a data layer method, which means it has been all the way through the layers... the LAST thing in the world you would do is break tier hierarchy by presuming that this function gets blind data straight from the UI... again that is a choice you are making to cut corners... and again it isn't anything to do with best practice.
And your talk about the "cost" to raise an exception is completely invalid. If you have a customer that does not exist then the application can no longer proceed because the very thing you were asking it to do is no longer valid. No more processing can occur and hence you don't have any restriction on processing time. That is why it doesn't matter how costly exceptions are... because they are exceptional and they only happen when you don't expect them (like someone asking for orders for a customer that doesn't exist).
Perhaps if I were to give you an example that may be more fitting... Lets take a Web Service that has an API call to retrieve the orders for a customer. So it is now known to be the top level interface, which means it will receive input directly from an unfiltered source. Would you EVER just return null from this API call? Absolutely not. You would return a result or a status indicating whether the call was successful or not and in that return status you would indicate any errors that occurred during the processing of that... and what would you put in that list of errors when the customer doesn't exist? You would put an error saying that the customer doesn't exist... and where would you find out this detail? When your database call to get the orders for the customer raises an exception.
You would never in your life return a null without providing any other information... if you did, your fired, I hear Kmart is hiring floor staff, you may be better suited there
|
|
|
|
|
Sorry for the editing...
Yes, I realize now that I veered away from the topic - best practices.
And on that subject, I agree completely with what you say. However, I insist that there should always be a non-throwing, null-returning option (by convention starting with "Try"), and it should bullet-proof with regard to input (i.e. it should not expect that any validation what so ever has been made before the call - it should do that itself).
Example: A CMS system that internally throws and catches an exception (and THEN returns null) when trying to retrieve a dictionary item (translatable text snippet) thet doesn't exist - something which is done intentionally quite frequently, as many texts are optional. We had one web site that did this too frequently, so I replaced the call with a wrapper I wrote, which creates missing entries (empty). The site got about 100% faster after we hit each page once. So yes, exceptions DO matter, in some environments and situations.
Web APIs: Agree. My web services for use by angularjs always return error conditions (and related messages, if any) in a consistent way, so that the js coder can use common code to handle them.
Peter the small turnip
(1) It Has To Work. --RFC 1925[^]
|
|
|
|
|
PeterTheSwede wrote: And on that subject, I agree completely with what you say. However, I insist that there should always be a non-throwing, null-returning option (by convention starting with "Try"), and it should bullet-proof with regard to input (i.e. it should not expect that any validation what so ever has been made before the call - it should do that itself).
Example: A CMS system that internally throws and catches an exception (and THEN returns null) when trying to retrieve a dictionary item (translatable text snippet) thet doesn't exist - something which is done intentionally quite frequently, as many texts are optional. We had one web site that did this too frequently, so I replaced the call with a wrapper I wrote, which creates missing entries (empty). The site got about 100% faster after we hit each page once. So yes, exceptions DO matter, in some environments and situations.
Sorry but I am going to have to step in again here and highlight the problem with returning nulls and blinding exceptions.
Dictionaries contain items, and anyone can put anything into a dictionary (excusing validation logic). So I could legitimately put a null into a dictionary entry. If you are overloading an exception and returning null instead, how do you differentiate between the fact the dictionary entry was there and its contents was a null, and the fact that the dictionary entry does not exist?
Again the same problem arises.. you are overloading the purpose of the function to do something it should not be designed to do... to tell you through the data itself a condition of the internal workings. If it is valid that a key may not exist, then why wouldn't you write the code to check for it first? After all, it is only doing an indexed key lookup, so the cost is small, but its the price you pay for having a dictionary of unknown keys. At least by doing this you are very specific in what you are doing and those reading the code afterwards know EXACTLY what you are meaning. When they see the ContainsKey they know implicitly there is a chance the key may not exist and if they still get a null back from the function after confirming that the key was there, they know the dictionary contained a null instead of this mysterious overload of "null means no key".
So again the problem has absolutely nothing to do with the best practice of always returning an empty list not being as good as returning a null.. again it is about code being written in a poor way.
I have yet to find one single answer that anyone has given that relates to the actual practice itself. They all relate back to people doing things in code they shouldn't, breaking encapsulation, creating illogical business rules or forgetting to do their validation correctly.
So if you are still "insisting" there should "always" be a non-throwing, null-returning option... I would be questioning the reasoning behind why you are trying to force that logic and apply it unilaterally across the board in all cases....
The worst thing a programmer can ever do is swallow an exception and translate that into something different (caveat: I know something it is necessary when there isn't a .Net function that can do it for you)
|
|
|
|
|
Enigmaticatious wrote: Sorry but I am going to have to step in again here and highlight the problem with returning nulls and blinding exceptions.
No need. Blinding exceptions is horrendous.
Enigmaticatious wrote: Dictionaries contain items, and anyone can put anything into a dictionary (excusing validation logic). So I could legitimately put a null into a dictionary entry. If you are overloading an exception and returning null instead, how do you differentiate between the fact the dictionary entry was there and its contents was a null, and the fact that the dictionary entry does not exist?
First, note that the dictionary in the CMS is not a dictionary in the programming sense. It's a dictionary in the sense of a word or phrase translation lookup system, where most texts get a fallback value, and for content that only exists in one language, the fallback ususally suffices, so the entries are never created (this is a manual operation managed by an editor or site manager).
By specification returns a null text if the dictionary item doesn't exist or if the text for the current language is null. The problem is that it checks this by catching an exception (only the one sayin that the key didn't exist in the dictionary) and then returning a null text as per specification. I never see the exception unless I look at the system's log files, which tracks all encountered exceptions (even when caught at a higher level), but they are NOTICEABLE slow, as they may be hit tens of times on each page - sometimes much more. My wrapper is actually a parallel implementation, which creates the dictionary item when it doesn't exist, and then returns the text (which is null, because nothing is written in the translations - also by design and spec. It also hadles fallback texts (in the call) and a default language fallback. As a bonus, it gets the customers (site manager) attention to the fact that it EXISTS and can be customized...
But it IS a very special case, in that a missing dictionary item is not an error (and never has been). It is perfectly normal, and only means that the text has not been customized. And there should be no exceptions when everything works as expected. But sure, give me a null or malformed dictionary key and you'll se an ArgumentNullException or ArgumentException in all cases. Or some Sql-exception if the dictionarys database is offline.
Best practice for my customer order thingy (which is usually what it looks like to my colleagues, as the GetCustomerOrders() method would normally be internal in a lower layer):
var customer = Store.TryFindCustomer(Model.CustomerNumber);
if (customer == null)
{
@:@RenderErrorMessage("Invalid customer number entered")
}
else if (customer.Orders.Any())
{
@:@RenderOrderList(customer.Orders)
}
else
{
<p>The customer has no order history.</p>
}
And on ANY other error other than an invalid customer number (or nonexisting), you get an exception.
And YES, the Orders property would always return a list. In an example somebody mentioned, like Patient.Allergies, MAYBE null could be returned if they were unknown. But perhaps not best practice. I would call it KnownAllergies and return an empty list, end then have other means to communicate the testing status.
Enigmaticatious wrote: So if you are still "insisting" there should "always" be a non-throwing, null-returning option... I would be questioning the reasoning behind why you are trying to force that logic and apply it unilaterally across the board in all cases....
I don't. Maybe I was unclear. I DO want it in cases where a) the use case for the Try-pattern is valid, obvious and common (such as when parsing user input - the type.TryParse(value, out result) functions are HIDEOUS, but they are from the time before nullable value types... they are typical examples of where I would return null instead of false, and get rid of the out parameter.
Otherwise... I only return null when it is obvious that it can happen and when the result actually MEANS "unknown, not given, not relevant". Doing it for lists... well, that is from case to case. I tend to return null internal functions if I expect it to happen often, and then I handle that on a higher level.
Enigmaticatious wrote: The worst thing a programmer can ever do is swallow an exception and translate that into something different (caveat: I know something it is necessary when there isn't a .Net function that can do it for you)
Agree on that too except in some marginal cases. If you write a large library using many other libraries you MAY want to cram SOME exceptions into a more "catcheable" exception class hiearchy (but of course always report the InnerException). This helps the library consumer to differentiete between, for example, transient and permanent errors. In a system that talks to a couple of stacker cranes in a warehouse (system name SM), I have:
- SMIcsCommunicationsException: Alert the operator that I can't talk to the electronics managing crane X and its conveyers. Have someone check the cabling.
- SMIcsStateAnomalyException: Alert the operator that the cranes opinion of what it is doing and with what is not consistent with mine. Tell the ICS system to revalidate status and check so that it really carries the same pallet I think it does.
Almost all other exceptions are allowed to bubble, and get caught, logged and presented at the highest level. The system is normally locked after that (I don't allow them to keep using it until I've investigated - we don't want a pallet missing in a warehouse with 50 000 of them). It's fun to run the cranes manually, but not THAT fun. And someone has to compare all barcodes with a list...
Peter the small turnip
(1) It Has To Work. --RFC 1925[^]
|
|
|
|
|
PeterTheSwede wrote: The problem is that it checks this by catching an exception (only the one sayin that the key didn't exist in the dictionary) and then returning a null text as per specification
And why would it do this instead of checking the index to see if the key exists? This sounds like even though you know the key may not exist, you are allowing a failure to occur and making the exception a working part of the system... which exceptions should never be used in that way.
PeterTheSwede wrote: but they are NOTICEABLE slow, as they may be hit tens of times on each page - sometimes much more
If course they are! They should be... they only occur in "exceptional" circumstances and shouldn't be used as part of normal code operation. Yet again, the flaw is in the design of the code, not the best practice of nulls vs empty lists.
PeterTheSwede wrote: But it IS a very special case, in that a missing dictionary item is not an error (and never has been). It is perfectly normal, and only means that the text has not been customized. And there should be no exceptions when everything works as expected
Now I am wondering why the method doesn't take a parameter for the default value to use when one doesn't exist and why you are not using a key lookup instead of just letting the lookup fail with an exception. I think you could achieve exactly the same result regardless of whether you used null or not, possibly in a "TryXXX" scenario.. which by the way is an established pattern
PeterTheSwede wrote: Otherwise... I only return null when it is obvious that it can happen and when the result actually MEANS "unknown, not given, not relevant". Doing it for lists... well, that is from case to case. I tend to return null internal functions if I expect it to happen often, and then I handle that on a higher level.
At last, we are back on the actual topic
So why for lists is it "case to case"? Since when is a container no longer a container just because the request for its contents was for something that didn't exist?
I think this is the crucial part that keeps getting missed (which is why I repeat myself). A method returns a list, that is its job, to give you a list. Its job is to not also inform you whether the target you are retrieving the list from is valid or not, that is what exceptions are for because it is expecting it to be valid because you wouldn't call it unless it was. So if it says it will get you a list, then it should give you a list ALWAYS. It isn't valid to ever return null because what you are saying in reality is not "The customer doesn't exist" but instead "A list of orders is not applicable to that customer". That is invalid logic. The list is valid, the fact it is empty is valid... what isn't valid is that you passed a customer that doesn't exist and thus should raise an exception.
To do anything else would be to overlap multiple intentions onto the same function which I honestly believe is bad programming and I honestly think that someone who would promote such a thing to the extent where they validly believe it can be equally compared to a best practice of more than 20 years should be something they are ashamed of.
I dont see any validity in this bloated chain of articles at all, certainly not enough to validly challenge the existing best practice (which may be why so many people keep using unrelated examples that are off topic or have to fudge their code to work around the glaring problems), and I think it does a huge disservice to fledgling programmers out there who may naively see this as being a genuine comparison.
I keep asking for a concrete example that shows specifically that it is the use of an empty list instead of a null that causes bad things to happen and in all cases the response is the result of bad programming, not the fundamental core of the topic itself.
There is a reason why this practice has stood the test of time, why thousands of developers have used it, and why people cannot easily come up with an alternative (without fudging bad code)... yet the author of the article just seems oblivious to it all... he seems more focused on defending his articles than he genuinely does in raising a valid concern for programmers to think on.
|
|
|
|
|
About the original implementation in the CMS: Not mine. Broken, I'd say. But in any case, it's beside the subject - it's not a list.
And yes, I agree. Mostly.
As for the "case to case": There is an excellent Microsoft article on how to return lists, collections and so on (which, OF COURSE, I can't find now), and it separates heavily between public API:s and internal. In internal API:s (literally "internal", I sometimes return null because it simplifies my calling code. In public API:s, not.
And like I said before, my customer orders example was a bad one - it is an internal API. The external API is a property on a customer object, and it always returns a list, as the customer object can't exist for a nonexistent customer...
About the article itself: I think it's OK to challenge existing best practice. It is always OK to challenge things, especially constructively. As in this case, advocating clearer contract definitions. With such, the question would be moot - if it's in the contract, null can be returned, otherwise not.
But if we don't have an explicit contract (i.e. the ability to enforce it) - we use the established implicit contract - return an empty list (or, according to Microsoft, probably a read-only IList<t> subclass). And if you feel that that is inappropriate or dangerous in some marginal situation (patient allergies, again), name the method or property accordingly (KnownAllergiesOrNull or whatever).
But challenging existing best practices? Fully acceptable and required. Nothing is so good that there is no room for improvement. And if you're barking up the wrong tree, the community will let you know - that's what discussions are for...
And yes, being able to formulate contracts and have them validated (in many cases it could probably be done at compile time) would be nice. Not just for lists.
Personally, I think non-nullable objects would be cool, by the way.
Peter the small turnip
(1) It Has To Work. --RFC 1925[^]
|
|
|
|
|
Ah... and about the last paragraph you wrote: I would - in my example - never return null under any other condition than that the customer number was somehow invalid. In the event of any other problem whatsoever, I would throw an exception (or let one bubble up, if I'm having a lazy day - in most cases, that provides enough information to explain the problem - a database issue, for example).
Also, we don't have Kmart in Sweden...
Peter the small turnip
(1) It Has To Work. --RFC 1925[^]
|
|
|
|
|
PeterTheSwede wrote: Ah... and about the last paragraph you wrote: I would - in my example - never return null under any other condition than that the customer number was somehow invalid. In the event of any other problem whatsoever, I would throw an exception (or let one bubble up, if I'm having a lazy day - in most cases, that provides enough information to explain the problem - a database issue, for example).
But you have made up this illogical business rule that says "if you get null, it means the customer doesn't exist". What is worse is you now tackle problems in totally different ways. So if its an invalid customer you get null, but if its anything else you get an exception... so now I have to write 2 completely separate code paths to handle the two different ways you are trying to inform the consumer of issues.
Why the inconsistency? Why create this obscure rule? Why give null an extra meaning when it doesn't need it and shouldn't have it? How is that NOT bad programming?
Null is null, it means nothing and it should always mean that and only that... nothing. Start giving it additional obscure meanings and you start creating coding nightmares.
PeterTheSwede wrote: Also, we don't have Kmart in Sweden...
Hehehe... no problem, IKEA it is then
|
|
|
|
|
Enigmaticatious wrote: But you have made up this illogical business rule that says "if you get null, it means the customer doesn't exist". What is worse is you now tackle problems in totally different ways. So if its an invalid customer you get null, but if its anything else you get an exception... so now I have to write 2 completely separate code paths to handle the two different ways you are trying to inform the consumer of issues.
I don't QUITE agree. I wasn't clear enough. My classes are used by a thin (Razor) presentation layer, and that particulare routine is called when an administrator enters a customer number and wants to see the statistics for the customer. Getting an invalid customer is, in that context, NOT an exception. On the contrary, it should be expected. So a null return for invalid (user) input is a better match for the use case here. An error that should never happen (such as a database comms failure) IS an exception. Or if I find some internal inconsistency in my data, or get invalid parameters to an internal routine that should never encounter that. So yes, the difference between a null and an exception is VERY clear (in code that expects to bee called from the frontend with user data). But sure, it may be seen as a marginal case. And again, a leading "Try" would make it much clearer.
Enigmaticatious wrote: Null is null, it means nothing and it should always mean that and only that... nothing. Start giving it additional obscure meanings and you start creating coding nightmares.
Well, formally, null means "missing or inapplicable information", or "unknown" for short - it was invented for exactly that case. In my example, an empty list means "the customer you requested has no orders", whereas null means "I don't know if that customer has any orders, because the customer isn't in this system - perhaps it has not been exported from your business system yet, or you entered the wrong numer". Those two cases exactly match the thinking behind introducing the conecpt null in the first place. So I don't think I'm giving it new meaning, I'm just using the existing one...
IKEA? Nah... I've got a bus drivers license. I'll make use of that instead... maybe. But after 30 years in this business it's still fun, so I think I'll stick around a while longer...
Peter the small turnip
(1) It Has To Work. --RFC 1925[^]
|
|
|
|
|
PeterTheSwede wrote: I don't QUITE agree. I wasn't clear enough. My classes are used by a thin (Razor) presentation layer, and that particulare routine is called when an administrator enters a customer number and wants to see the statistics for the customer. Getting an invalid customer is, in that context, NOT an exception. On the contrary, it should be expected
Yes but because it is expected, it should mean that you CHECK for that customer before doing anything else and failing that you wouldn't even attempt to try any other code.
For the sake of saving a bit of code or an extra call (do you really need to save that trillionth of a nanosecond?) you overlap two purposes into the same function and make that function work specifically for its intended target. This is called "breaking encapsulation" as you are looking at who consumes the method and writing the code to suit the caller. Its a violation of one of the 4 primary pillars of object orientation.
PeterTheSwede wrote: So a null return for invalid (user) input is a better match for the use case here
No its not. Checking for the customers existence is a much better match for the use case here, because as you said, it would be expected to not exist and therefore needs to be checked prior to being used.
I get the impression you are focusing on efficiency over everything else. This isn't best practice, its cutting corners. There is no problem with doing this, I understand that its necessary... but it doesn't belong in a discussion for best practices.
PeterTheSwede wrote: But sure, it may be seen as a marginal case. And again, a leading "Try" would make it much clearer
It still doesn't belong in a discussion about best practice of nulls over empty lists. Best Practice discussions are about generalizations not marginal cases where you cut corners to try and save a few lines of code and a few cycles of a quad core processor blazing away at 240MHz. Is it really worth that fraction of a second to do something that badly?
PeterTheSwede wrote: <layer>Well, formally, null means "missing or inapplicable information", or "unknown" for short - it was invented for exactly that case. In my example, an empty list means "the customer you requested has no orders", whereas null means "I don't know if that customer has any orders, because the customer isn't in this system
You may read that into it, but I see it in a totally different light (which may explain the difference in our opinion).
If I have a container, and I ask you to put the contents of your pockets in there and you tell me your pockets are empty, then I can look in that container and see it is empty. If I were to then walk over to the wall and start talking to the wall and ask it to put the contents of its pockets in there does it magically make my container disappear? No it doesn't.
The fact that the customer doesn't exist doesn't mean the construct you were using to collect target items ceases to exist, it just means it has nothing to put in there.
YOU are putting the implied rule that it must mean there isn't a customer and therefore the list suddenly ceases to exist in order to tell you this and I believe that is completely invalid. What if the customer does exist but they dont have any pockets so they are unable to give you anything if they wanted to? Is it a null again because the pockets don't exist?
You aren't using the existing meaning... what you are doing is the same mistake I keep repeating. A list is an object, an object can be null, so you think it is therefore valid for a method whose signature returns a list to validly return a null... thats wrong. A list may be an object but just because it CAN be null, doesn't mean it is valid to be null. Its a container, and you are asking to fill that container. If there is a problem even attempting to fill the container then you report exactly what that problem is (ie exception), but in all cases, the container never stops being a container.
I have to ask, are you able to see this difference? because if not, then that explains why we are going back and forth. I would appreciate it if you could acknowledge this, and if you do see it, explain why the container suddenly ceases to exist.
PeterTheSwede wrote: IKEA? Nah... I've got a bus drivers license. I'll make use of that instead... maybe. But after 30 years in this business it's still fun, so I think I'll stick around a while longer...
At least you can understand humor when you read it... Apparently I told you to go get a job at KMart... I haven't the heart to try and explain to Christian the difference between a direct comment said to someone and a conditional statement that is only implied to illustrate a point with a dash of humor.... it seems lost on him
|
|
|
|
|
Haha you're right. I didn't focus enough on lists, obviously.
Well OK, I'm on the band waggon.
Like I noted somewhere else, the example I took was from an internal API, which is only used by me, from one place - when lazyloading the order history for the customer. No hit in cache, no hit on customer, return null. In which case the customer object knows that it is under creation (has no customer number, for one thing), initializes it's backer with an empty list and then goes on about is business.
Still thinking about that case of the known allergies of a patient, though. I think I would throw an exception, because I would have flags saying if this and that information was available or not, and if the other coder ignores that and touches the list... boom.
On humor: I've been doing this stuff for 30 years and been in innumerable heated discussions about programming. We all love them, and we all have our opinions. Disagreement and dislike are two very different things...
Ok, you express yourself a bit harshly at times, but I think I take the "tone" the right way...
Peter the small turnip
(1) It Has To Work. --RFC 1925[^]
|
|
|
|
|
Enigmaticatious wrote: The only way that the continuing execution of the program can be "extremely harmful" is if the developer purposely made it extremely harmful. There is absolutely no situation under which "normal" execution of code would be harmful.
I am totally speechless. Maybe I misunderstand your comment. If not, I wonder how anyone can make such an incredibly wrong statement. Just search the net and you will find numerous examples of harmful outcomes because the code missed to make a difference between "we have data" and "we don't have data" (i.e. the client code didn't check for an empty list).
Enigmaticatious wrote: How about you give an example of something "extremely harmful".
In my previous comment I gave you two links to examples!!!
Anyway, here is another example:
Suppose the following function signature in a medical application:
List<Allergy> getAllergiesOfPatient()
Now imagine the method returns an empty list because an allergy test has not yet been done, but the doctor interprets the empty list as "The patient doesn't have any allergies".
On the other hand, if the method returns null then a null pointer error will very probably be thrown (if the client code doesn't check for null), but at least the same risk of misinterpretation doesn't exist.
The main point of why it is better to return null is this: The average risk for bad outcomes in case of a software bug (i.e. the client code forgets to check for null or an empty list) is less high if functions return null instead of an empty list.
Now, before you shout again: "this is just bad programming": Yes, the method should return an object with more information. But there is a lot of 'bad code' out there, and what we need is a programming environment that helps us to limit bad outcomes as far as this is technically possible.
Enigmaticatious wrote: Returning a NULL instead of an empty list would get you EXACTLY the same outcome.
Totally wrong!!!
Enigmaticatious wrote: I believe it is respectful to be honest
I believe it is respectful to be honest in a respectful way.
Enigmaticatious wrote: I proved you incorrect
I think you proved absolutely nothing. You just write long comments without any source code examples and which, in my opinion, are difficult to digest and understand, at least for average people like me.
If you want to prove something then I suggest you proceed as follows:
- show us a piece of bad source code
- explain why you think this code is bad
- show us how you think the code should be written
- explain why you think your version is better
If you need examples of constructive feedback, then please have a look at all the other comments posted, including those from people who disagree with me.
Enigmaticatious wrote: it is my humble opinion you are doing it to write articles instead of helping people... but that is an opinion and it could be wrong
Yes, totally wrong.
|
|
|
|
|
ChristianNeumanns wrote: I am totally speechless. Maybe I misunderstand your comment. If not, I wonder how anyone can make such an incredibly wrong statement. Just search the net and you will find numerous examples of harmful outcomes because the code missed to make a difference between "we have data" and "we don't have data" (i.e. the client code didn't check for an empty list).
Yes you have completely misunderstood my comment. It requires a developer to CODE it that way, it doesn't become extremely harmful just because you chose to use an empty list instead of a null. That is the point I was trying to make. You added "extremely harmful" to try and falsely exaggerate your own point of view when in reality ANY code can be harmful because that is how it can be written.
Or do you think that you cannot possibly get "extremely harmful" code if you return a null? Its impossible for a developer to do something horrendously wrong if only they returned a null instead of an empty list?
Is this very simple distinction lost on you?
ChristianNeumanns wrote: Now imagine the method returns an empty list because an allergy test has not yet been done, but the doctor interprets the empty list as "The patient doesn't have any allergies".
Immediately you have failed right here...
The doctor is the USER, not the programmer. For a user to misinterpret something means the developer didn't do his job properly.
An empty list means exactly that... No allergies are recorded. Nobody could possibly know if the patient actually has them and just didn't list them or not, even you returning a null is making the presumption that somehow and on some interface there is a part which says "are your allergies recorded", otherwise how would you even know to return an empty list or a null?
So again, completely idiotic example, and it is bad programming.. The fact you have gone back to harp on about bad code being out there as a valid reason to change best practice is astoundingly stupefying. You dont write best practices or even CHALLENGE best practices by first accepting bad code as the foundation of your reason for doing it.
Do I need to say it again? Bad programmer, shame on you! (I will keep saying it until you stop saying that because there is bad code out there we should make our best practices conform to it instead of stamping out bad code THROUGH THE USE OF THOSE BEST PRACTICES).
ChristianNeumanns wrote: Totally wrong!!!
Prove it. You make a wildly unsubstantiated statement that the "average risk for bad outcomes" is somehow better for nulls, I would like to see your source on that. I would hazard a guess you are completely wrong there. The NullException error is one of the most often returned exceptions, so by you now adding to this plethora of bugs with yet another NullException instead of returning a "NoAlergiesRecordedException" which I would hazard a guess is fairly unique and far easier to spot.
But please, show me this proof of the average risk for bad outcomes being empty lists, I would love to see it
ChristianNeumanns wrote: I believe it is respectful to be honest in a respectful way.
Let me ask you. If you had to choose, would you prefer a respectful lie or a disrespectful truth?
ChristianNeumanns wrote: I think you proved absolutely nothing. You just write long comments without any source code examples and which, in my opinion, are difficult to digest and understand, at least for average people like me.
If you want to prove something then I suggest you proceed as follows:
- show us a piece of bad source code
- explain why you think this code is bad
- show us how you think the code should be written
- explain why you think your version is better
If you need examples of constructive feedback, then please have a look at all the other comments posted, including those from people who disagree with me.
I think you just hit the nail on the head...
"An average person like me"
That you somehow fail to see the long standing conventional wisdom of a best practice that I am sure started before your career (and will most likely still be going when its over) may be because you are just not able to comprehend it. That you are unable to follow the simple logic of my statements confirms this.
I am not trying to prove anything... YOU are. Perhaps you should take your own advice? You are the one saying "extremely harmful" code results only when you return an empty list but not a NULL, so please.. show me code that isn't the direct result of bad programming, that because an empty list was used it is extremely harmful? Its not my statement... its yours.
So if you believe that you want to prove something (again your words) then do what you believe is the process to follow.
Have you not noticed that you get very vague with exaggerated terms whenever you are talking about the best practice? That you specifically use examples that are slanted towards the answer you want it to be and that you use sensational terms whenever it suits? There is a reason for that.... You are more interested in writing articles than actually finding a more productive practice. Not very objective.
ChristianNeumanns wrote: Yes, totally wrong.
Are we reading the same comments? How many people are in complete disagreement with you? How many are confused?
I am not totally wrong. The best practice has stood for decades, your attempt to challenge it is pitiful and poor, the fact it is "respectfully" written confuses people into thinking it has some validity, this creates more confusion and some people may sadly listen to you... hence you are doing a disservice in writing this kind of ill-considered posturing and thus the only clear answer to conclude is that the reason you took 6 articles to do it means you are more interested in writing articles than helping people.
I actually stand by that comment now... and I doubt I am wrong at all. You may "think" you are helping people, but I think subconsciously you are very proud of this article and it is your hubris which is preventing you from seeing how utterly bad it is.
In terms of laying out articles, and format and succinctness of explanation I think its awesome, your thoroughness is very impressive (and I mean that)... its just that your programming skills are severely lacking and your inability to grasp concepts of programming practices has lead you to create this monstrosity all because you have failed the one very simple understanding:
"Just because a list is an object, and objects can be null... DOES NOT mean that it is valid to return a null instead of a list for functions whose signature does so"
The second you realize both this and the fact that best practices do NOT revolve around how much bad code is out there, you may actually have an epiphany....
But I still think more harm is done with this article than good.
|
|
|
|
|
A good discussion point - I'm in the return empty list camp in general, but, like all things, it really depends on the context.
I would draw issue with your example regarding the empty directory listing and having to check the documentation. Well, practice TDD and this issue would be solved very early on.
I'm a c# developer and Microsoft's own recommendation is to always return an empty list as you quite rightly stated - so this is what I do, not because I don't understand the implications, but because it's a way of standardising.
When you mention that you have to rely on the 3rd party developer (or even a developer in your own team) to have the discipline to return an empty list in all cases largely relies on the fact that they haven't read articles of this nature that deem it necessary to turn the apple cart upside down.
While I agree that there are good reasons to question everything in programming, sometimes all it does is help propagate the "undisciplined" development practices that have quite rightly highlighted.
in the scenario that a less experienced developer or junior developer or even a senior developer reads an article such as this, or even this article and, due to the compelling arguments believes that this is the best approach, what happens? He now sets all his list returning methods to return null instead of an empty list so what do we end up with? Articles such as this that tell us that we can't always trust that a method will return an empty list??
...and an article this is an example of Self fulfilling prophecy!
Good arguments none-the-less, but I'll stick to my standard thanks.
|
|
|
|
|
namydnac wrote: practice TDD and this issue would be solved very early on
I agree.
However, practice shows that some people simply don't write tests, because it takes time, discipline, and a good amount of experience to write good tests that cover all corner cases. The point of the article series is that deliberately using null and a language with compile-time null-safety helps to detect some bugs at compile-time.
namydnac wrote: He now sets all his list returning methods to return null instead of an empty list so what do we end up with?
Yes, this is bad.
That's why I included the following note in the final conclusion of part 2[^]:
"Sometimes, we don't have the choice. For example, if we work in an environment where 'return an empty collection and not null' is the rule and applied by all members of the team, then we should do the same, even if we are convinced that returning null is better. Swimming against the tide creates inconsistencies and leads to other evil problems."
|
|
|
|
|
For me, the point when I'm programming is: What is easier?
For me easy is:
public List<string> GetList()
{
List<string> rest = new List<string>();
return rest;
}
This is my opinion and my way to do....
|
|
|
|
|
maxcel wrote: For me easy is: ...
Yes, this is easy.
But it is also error-prone because:
1. We should always return an immutable list (and not a mutable one), unless there is a specific reason not to do so.
The rationale is explained in chapter 'Example 1: Return a Non-empty List', sub-chapter 'Java version' of part 4[^]
2. It is better to return null (instead of an empty list) in case of "no data"
The reasons are explained in part 2[^]
|
|
|
|
|
It would be easier for everyone to follow your posts
And thank you for a very nice post
|
|
|
|
|
Good point.
I'll fix that in the coming days.
Thank you.
|
|
|
|
|