|
Would you check for NULL there?
|
|
|
|
|
That doesn't make any sense.
If that event was called, the both the textbox AND the text are NOT NULL.
The user can't make the text null. Maybe some other process can, so if that's possible, then yes, null check.
What am I missing?
If it's not broken, fix it until it is
|
|
|
|
|
|
I updated my reponse
If it's not broken, fix it until it is
|
|
|
|
|
Kevin Marois wrote: if that's possible
The method has no of knowing.
I fact I can call textBox1_TextChanged ( null , null ) any time I like -- how many apps would blow up if I did?
It's just not really a worthwhile thing against which to protect so I won't "ALWAYS Null Check Object References".
|
|
|
|
|
In 30 years of coding I have NEVER seen a textbox changed event receive null parameters. If this occurs, then something else is wrong.
If it's not broken, fix it until it is
|
|
|
|
|
No. You should make a null check only if you know that you may call the function/method with something that legitimately could cause a null return.
In all other cases, you WANT a null reference exception - otherwise, you may never figure out that you're calling the method with garbage. The null exception enables you to find the root cause of the problem (the garbage).
For example, if a function (like in my latest comment) that gets orders for a customer returns null for invalid customers, but returns an empty list for valid customers with no orders - no null check. Give it an invalid customer, and you NEED to get slapped into finding out why.
Peter the small turnip
(1) It Has To Work. --RFC 1925[^]
|
|
|
|
|
Your argument seems to spectacularly fall apart with the following statement:
Quote: If the function returns an empty list and the client code does the wrong thing, then the outcome is always undefined (see second column).
In this context 'undefined' means that the outcome is unpredictable and can vary from totally harmless to extremely harmful. We will look at examples later.
This is a crucial point!
What exactly is your definition of undefined? How can the outcome be unpredictable when the very code that follows it will almost ALWAYS attempt to iterate over the empty list, thus doing NOTHING. If a developer has put something horrendous following lines of code then it isn't the fault of whether you used NULL or an empty list, it is the fault of a bad developer who somehow managed to stuff up something as simple as checking an empty list.
So without even reading further your initial premise is unfathomably flawed, it relies on a specific non-intuitive case of bad programming to make your personal view point seem to stand out.
I would like you to consider something... If a method call says that it returns a LIST, then IT SHOULD RETURN A LIST! The fact that the language supports the use of nulls in place of objects does NOT constitute a failure of the rule to return an empty list, but instead the failure of programmers to actually adhere to the signature that they themselves have indicated.
If instead they wished to support a NULL instead of an empty list, then they should have created their own object class which contained a list and thus returned null in that way.
I truly hate when people try to come up with "answers" which all revolve around accepting bad programming and then turning that bad programming into practices that SUPPORT it, instead of stamping it out violently wherever it is found. The only reason this is even a topic is because bad programmers will ignore the very well defined rule of always returning an empty list. It is only due to NOT following this that the situation arises... please don't encourage them by saying "hey guys, remember how you ignored that rule? Don't worry, I am going to make it correct for you"
Bad programmer! Shame on you!
|
|
|
|
|
Enigmaticatious wrote: What exactly is your definition of undefined?
If you carefully re-read your comment (i.e. my quoted text) you might discover that the answer is there already.
Enigmaticatious wrote: How can the outcome be unpredictable when the very code that follows it will almost ALWAYS attempt to iterate over the empty list, thus doing NOTHING.
That's exactly one of the points of the article: Doing nothing and just continuing program execution leads to unpredictable outcomes that can vary from totally harmless to extremely harmful.
It's like in real life. Doing nothing in a given situation might be appropriate, but is often the worst we can do. Imagine: You are sick and you do nothing.
For examples of what can happen in applications, there is an illuminating comment posted by a user in a Stackoverflow question and quoted in chapter 'The Null Object Pattern' in Why We Should Love 'null'[^] . Moreover part 2[^] includes concrete source code examples that demonstrate this point.
Enigmaticatious wrote: I would like you to consider something... If a method call says that it returns a LIST, then IT SHOULD RETURN A LIST!
So: If a method says that it returns a date, then it should return a date, right?
What should method getFirstDeliveryDateOfProduct return, then, if the product hasn't been delivered yet?
'null' has been invented to cover these cases, and it should be used for collections as well - for the reasons explained in the article series (especially part 2[^].
Enigmaticatious wrote: I truly hate ...
I truly like well mannered and respectful people.
We are all here to learn and exchange and discuss ideas. It is perfectly ok to argue and disagree - if there is a good reason to do so and if we explain why we believe that X is better than Y. But all communication should be made in a friendly and respectful way. Harsh words are counterproductive and reduce karma.
Enigmaticatious wrote: Bad programmer! Shame on you!
Thank you.
Now everybody knows how brilliant you are.
|
|
|
|
|
ChristianNeumanns wrote: If you carefully re-read your comment (i.e. my quoted text) you might discover that the answer is there already.
If you read my response you would have seen that I was talking specifically about your non-defined definition of undefined.
ChristianNeumanns wrote: That's exactly one of the points of the article: Doing nothing and just continuing program execution leads to unpredictable outcomes that can vary from totally harmless to extremely harmful.
And I completely disagree with that. 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.
As I said (which clearly you ignored, or it didn't fit your point?), the most likely outcome is to then attempt to iterate over the results, which if you return an empty list would do NOTHING.
How about you give an example of something "extremely harmful". I assume it would be easy to do so seeing as you are well versed in it being a highly likely outcome?
There is even no point in trying to explain when your "concrete example" starts with the comment "Is there no orders or is something wrong with my computer". Returning a NULL instead of an empty list would get you EXACTLY the same outcome. If there was a problem with your computer it would raise an exception, after all... problems with your computer are "exceptional". Now if you were to return a NULL in the even there is a problem instead of an empty list (which I believe is the point you are trying to make), then again you have bad programming that has allowed the code to return a NOT APPLICABLE value and yet not bothered to tell you WHY it is absolutely nothing. You still don't know which it was, because clearly the method call is flawed if it neither raises an exception nor specifically tells you that in the event of something going wrong it will return null instead of an empty list which tells you there are actually no records. And especially when you make the point of saying that you cannot trust what comes from a 3rd party as to which approach they used, you would be even more confused. The answer however is NOT in determining whether to use empty lists or to use nulls... it comes in bad programming and bad programmers.
If you don't know EXACTLY what that piece of code does in all cases, I would say your unit tests are a failure and you shouldn't be programming if you cannot trust the code you use.
Exceptions get raised when something goes wrong, the signature clearly states it will return a list. If there are no records you will get an empty list, if there is a problem you will get a very well worded exception explaining what it is.
Why is that such a hard concept to grasp?
ChristianNeumanns wrote: So: If a method says that it returns a date, then it should return a date, right?
What should method getFirstDeliveryDateOfProduct return, then, if the product hasn't been delivered yet?
'null' has been invented to cover these cases, and it should be used for collections as well - for the reasons explained in the article series (especially part 2[^].
Are you serious???
It returns a
DateTime?
Again, the signature CLEARLY explains the expected results.
Again, you are confusing the fact that a list is a type of objects and objects can be null with the fact that a method returning a list has been badly programmed because someone people don't get this.
Again, I say that trying to "examine each approach and compare" when the situation only arises due to bad programming is making the situation worse because you are falsely giving legitimacy to bad programming.
By your own statement "If everything always returned a list we would be alright, or if everything always returned a null we would be alright" is incorrect. If we always returned null we would forever be forced to do additional code to first check that it was null before using it. The current best practice is the most efficient code using the ACTUAL signature that is clearly defined in its syntax.
ChristianNeumanns wrote: I truly like well mannered and respectful people.
And I like being efficient and to the point... being overly "well mannered" simply for the sake of making someone else feel warm and fuzzy about themselves.
I also like articles which are objectively informative and not purposely slanted towards a specific outcome that just happens to agree with the author... which in all respect and the most sincerest earnest hopes of mutually beneficial productivity... is plain wrong. (see how being well mannered just adds more unnecessary words that don't change the end result?)
I would much prefer a blunt and brazen approach that was always on the money and correct over the most well mannered and respectful idiot (and I am honestly not calling you an idiot, I dont think you are... just somehow misguided with article writing over validity of content). I would take honesty over fake manners any day of the week... I believe it is respectful to be honest, not to be nice just for the sake of being nice.
ChristianNeumanns wrote:
We are all here to learn and exchange and discuss ideas. It is perfectly ok to argue and disagree - if there is a good reason to do so and if we explain why we believe that X is better than Y. But all communication should be made in a friendly and respectful way. Harsh words are counterproductive and reduce karma.
I believe I have a good reason to disagree, I showed points, I proved you incorrect, I disproved counter point, I ignored your waffle about being sick (has no relation at all to the question and only attempts to deviate from the issue, this is programming, it isn't complex human biomechanical functionality in a homogenous environment), and I think as well mannered and respectful as your arguments are... they are MORE counter productive because of how flawed they are.
People come looking for answers and they have been given a best practice which you are now attempting to bring down for what reason? Because there is a legitimate reason to question it? Or just so you can write a half dozen articles for their own sake?
Any harshness of my words will be gone tomorrow... but this monstrosity of bad programming will still be here next week unfortunately and sadly some people will read it and not understanding how it is slanted towards your own point of view may regrettably take it on board.
ChristianNeumanns wrote: Thank you.
Now everybody knows how brilliant you are.
Nope not me... established wisdom called a "best practice". Was there for a reason, stood the test of time for a reason, and has continued to stand that same test. It should be obvious... but clearly it isn't.
I do believe what you have written is bad programming, I state my reasons are that you are ignoring obvious constructs, superimposing object nullability onto lists invalidly, ignore struct signature definitions, unable to see a simple nullable data type solution, validating bad programming by presuming others have written it in a poor way and then made it sound legitimate to compare the two approaches as if they are somehow equal to begin with when they are not.
I say shame on you because I think this does a disservice to people trying to learn and 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, but I respect you enough to be honest to you. I believe you will be better served by truth than by pretty placation.
An old Taoist saying goes "That which is beautiful is not always true, and that which is true is not always beautiful"
Very fitting
|
|
|
|
|
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[^]
|
|
|
|
|