Yesterday I made the following tweet:
Never pass nil to a method, and don’t let your methods accept nil as a parameter value.
— Nick Hodges (@NickHodges) January 19, 2015
and it started and interesting little discussion on Twitter. (I was actually honored that the great Mark Seemann himself entered into the fray…) Naturally, the conversation tried to point out why I was wrong, and all the ways that it was fine to use nil. That’s okay – if there it is one thing developers like better than making pronouncements, it’s finding exceptions and arguing against other developer’s pronouncements.
Well, I remain unconvinced and stand by my statement. No surprise there, I know.
There are two parts to what I said and I’ll argue them one at a time.
First – “never pass nil to a method”. Now people freak out about “never”, but it’s Twitter and you don’t have a lot of room for caveats. First, sometimes I guess you have to pass nil to some methods — ShellExecute comes to mind — but I’d argue that those methods are poorly designed (as I’ll talk more about in a minute), and that you should pass nil only very, very reluctantly. Again, I’m talking about your code, not other’s code. You should never send nil to a place where a valid object will be accepted. Never do that to someone’s code. Returning nil is always a bad design decision.
The Robustness Principle states, in part, that you should be very precise in what you pass, and passing nil is not precise. It misses the mark by as much as you can miss the mark. The target is a valid, working instance of whatever the method is asking for, and passing nil is like turning 180 degrees around and firing your arrow away from the bulls-eye. Nil should not be used as a “signal”, leaving the class you passed it to in a state where they can’t make use of what you’ve given them. You should avoid like the plague allowing a class to be in a state where it can’t do what it is designed to do, and if it asks for a object in a method or constructor, you are obligated to pass a working, constructed object that it can use without error. To do anything else is to invite an exception or worse, an access violation.
The second part is this: “Don’t let your methods – especially your constructors — accept nil as a parameter value”. Your classes should never let themselves get into an unusable state, and blindly accepting nil as a parameter will do that. Instead, your classes should carefully guard against being passed nil and quickly – no, immediately – fail if passed a nil reference. There is even a pattern – the Guard Pattern – that is employed to ensure that a program won’t continue unless things are acceptable. Getting nil is not acceptable:
procedure TMyClass.constructor(aSomeClass: TSomeClass); begin if aSomeClass = nil then begin raise NoNilParametersException.Create('Don''t you dare pass me a nil reference. Pass only valid instances'); end; FSomeClass := aSomeClass; end;
Every single time you accept an object as a method parameter, you need to use the Guard Pattern to prevent your object from being placed in a bad state where it can’t do what it is supposed to do. Fail fast and tell the user exactly what they did wrong and what they need to do to fix it. In fact, if a nil reference sneaks in, you’ll eventually get an access violation when you try to use this nil reference, right? Sure, it’s work, but it is time well spent. Knowing that your references are always valid makes for easier code, and actually can reduce the amount of nil checking that you have to do. Who wants to check for nil every time you use an object?
Don’t pass nil references. Don’t accept nil references. This seems obvious to me and I confess I don’t understand objections to this aphorism that only results in cleaner code. Why would you ever want to accept a nil object? Why would you want your code in a state that is begging for an access violation to occur? Insist that your objects are valid, and never impose invalid objects – nil – on others. Seems blatantly obvious to me.
So I am going to guess that this will not make some of you happy. So here is my challenge: Show me code that you have written where nil is acceptable. Don’t show me code that calls the Win32 API or the VCL, show me code you have yourself written where nil is acceptable and useful as a valid method parameter. I’d love to see it – and then point out the problems that it will cause. 🙂
Worth noting that TStringList.Add and TStringList.AddObject can be folded into the straighforward .Add with:
Function TStringList.Add(S : String; O : TObject=Nil) : Integer;
Which dramatically improves the API by reducing artificially required calls, required by the absence of default parameters.
In fact, .Add is simply a degenerate case of .AddObject with NIL as the object reference – many such cases exist.
I am somewhat confused: Why would someone pass a class as parameter and not an object?
Yep, I played fast and loose with the word “class”. Corrected. Thanks.
Raising exception in a constructor is much worse than passing nil to a constructor. Generally it passes a not properly initialized instance to the destructor and you can easily get a second exception in the destructor.
Huh? There’s nothing wrong at all with raising an exception in a constructor. Why do you think there is?
A case where perhaps the destructor should accept nil pointers in a sane way, huh?
Serg, you are wrong.
Destructors should not assume that object is fully initialized. This is even indicated in Delphi docs: http://docwiki.embarcadero.com/RADStudio/XE7/en/Methods#Destructors
“Destroy must be prepared to dispose of partially constructed objects. Because a constructor sets the fields of a new object to zero or empty values before performing other actions, class-type and pointer-type fields in a partially constructed object are always nil. A destructor should therefore check for nil values before operating on class-type or pointer-type fields. Calling the Free method (defined in TObject) rather than Destroy offers a convenient way to check for nil values before destroying an object.”
This is, like, the first thing to know about destructors.
Basically, any class with an owner should accept nil. Usual operations with these classes notifies the owner when something changes. However, you often want to make copies of these classes without an owner. So, to take examples from VCL: TComponent, TOwnedCollection, etc. And yes (to anticipate an objection), you could have events on these owned classes, but in general that is a bad idea as component users can easily break functionality by trying to use what is a valid public interface.
Also, as Xepol mentioned, list classes should accept nil: TStringList, TDictionary, etc
Most people commenting here fail to notice that Nick means your own code, not someone else’s code (like VCL, Windows API, 3rd Party libraries) most of which have notoriously bad design flaws in them.
Nick tries you to prevent from making these design flaws, hence his statement (emphasis mine) “Don’t let **your** methods – especially your constructors — accept nil as a parameter value”
I used VCL/RTL examples as a shorthand for the design patterns where nil is ok. But his restriction to not use VCL/RTL in examples is a little arbitrary. I would argue that IF you are writing classes and you are NOT descending *in a lot of cases* from TComponent, TPersistent, TStringList, TCollection, etc, you are probably writing your classes wrong. You are deliberately cutting your classes off from all the support code out there; not to mention that your classes cannot be used at design-time which is kind of Delphi’s strong point 🙂
When is passing nil acceptable? Every time when a method is designed to accept optional reference to some other object. But as you noted the method needs to be properly designed so that it handles correctly when nil is passed to it.
I use this heavily in one of my game designs where each ingame unit/crateure and ech item is defined by a class.
And since some units can equip items I rather use external classes than storing all item parameters into the units object. Why? Becouse this alows me data reusability.
So my class design looks something like this:
TIngameUnit = class(TEntity)
private
FWeapon: TWeapon;
protected
procedure SetWeapon(AWeapon: TWeapon);
public
property Weapon: TWeapon read FWeapon write SetWeapon;
procedure Attack(ATarget: TEntity; AWeapon: TWeapon);
end;
implementation
procedure TIngameUnit.Attack(ATarget: TEntity; AWeapon: TWeapon);
begin
if AWeapon = nil then
begin
//unarmed – mele attack
end
else
begin
//armed attack
end;
end;
Well to be honest my actual code is designed a bit differently. In my actual code my IngameUnit has bunch of properties like WeaponRange, WeaponPhyscialdamage, etc.
Each of these peoperties has a getter method which first checks to see if the FWeapon is nil. if it is some default result is returned (rage = 0, PhysicalDamge is calculated from units strenght, etc). But if FWeapon is not nil then the Weapons Range, PhysicalDamage parameters are forwarded.
This design alows me the data reusabiltiy since I don’t have to store all those weapon parameters in each instance of my unit.
Now since I’m not on my computer right now I can’t show you my actual code but I will do this once I get home.
Why dont you implement this by having a IWeaponIntf, implemented by a TWeapon class that returns the property values like Rage, Damage etc), and have a TNoWeapon implementation of IWeaponIntf that delivers the default values.
This would save you all the nil-checking in the propery getters, Delegate it to the interface implementation
I was thinking about using similar approach at the start but then decide not to becouse in certain scenarios Getter methods result depends on other values of the IngameUnit object so I rather stay within the scope of my TIngameUnit class as it makes the code more readable for me.
You can stil accomplish that from within the getter in the Tingameunit… Using the properties of the active IWeaponIntf and using properties off the unit itself.
Obviously passing nil when a valid object is expected is wrong. But I don’t think that’s always what is happening when nil is passed around. I guess a lot of the time nil is a sentinel of some sort. For instance, it may mean, “do default behaviour”. Or perhaps, get the information from your parent.
You are right to object to code that takes a copy of the reference and then later tries to use that reference. But often times code will not copy the reference. It will use that reference to initialise some other state. For instance:
I’m not saying that this is always good design. Rather I’m making the point that when references are passed as parameters, it is not compulsory to take a copy of those references.
Also, where does Nullable fit into this?
I very inclined to agree with the “no nil parameter” statement , Nick, but I would love to hear from you how you would solve the situations that are mentioned here as “situations where it would be valid to use nil”, like i.e. a default TObject=nil parameter
“Show me code that you have written where nil is acceptable”. With all respect, you are reversing the burden of proof. If you make the claim “Don’t use nil”, it is your job to support that statement with proof. I read a lot of opinion and repetition in your post, but very little factual arguments.
+1
Actually, the burden of proof lies with the affirmative. I’m stating a negative. The affirmative position is the one that has to prove itself, since you can’t prove a negative. 😉
Disqus schreef op 22-1-2015 om 16:25:
What a bizarre statement!!
Not at all, my favorite admirer.
I am making a negative statement: Don’t do this.
I don’t have to prove the negative — the burden of proof is always on the affirmative.
If you want to refute me, prove the positive — show that passing nil is a good idea.
“The burden of proof lies with the affirmative.”
Repeating the statement doesn’t make it less bizarre. What’s the difference between these two statements:
– Never pass nil.
– Always pass non-nil.
Wow, given that you always know what you are talking about, I guess I’ll have to completely re-think my understanding of the rules of rhetoric. I’ll have to learn again all about positive and negative assertions, about how to prove a negative, and basically everything I’ve been taught about philosophy.
Thanks for the correction!
From the Wikipedia article on Philosophical burden of proof:
From that same article:
“When the assertion to prove is a negative claim, the burden takes the form of a negative proof, proof of impossibility, or mere evidence of absence. If this negative assertion is in response to a claim made by another party in a debate, asserting the falsehood of the positive claim shifts the burden of proof from the party making the first claim to the one asserting its falsehood, as the position “I do not believe that X is true” is different from the explicit denial “I believe that X is false””
Sure. When somebody asserts something, the burden of proof lies with the person making the assertion.
It looks like you don’t understand that quote that you have provided. I suggest you read it again a little more carefully. Then go back to the top of this article and note that it is you that is making the original assertion. And then we come to @Jan’s comment. Which is bang on. Burden of proof lies with you.
Since you are my personal fact-checker, and you are always right, I’ll concede the point. Thanks for the correction.
FUNCTIONs returning NIL:
FUNCTION FindRecord(Parm1,Parm2 …) : TRecord;
FUNCTION FindGridCell(Parm1,Parm2 …) : TStringGridCell;
If the record/cell can’t be found, it seems perfectly okay to me to return NIL to indicate this…
Hi Eduardo,
I am not arguing that putting a nil on a list might not be a bad design decision, but writers of collection classes need to accept nil.
BTW, *My* preference (and it is only my preference) and thought is your first THouse.Create is more readable. 🙂
Of course, if I wrote your THouse class, I wouldn’t be accepting a roof in the constructor at all (or if I did, I would be copying the passed in roof). The THouse would create an internal TRoof and allow you to assign a Roof to it, e.g.,
property Roof: TRoof read FRoof write SetRoof;
procedure THouse.SetRoof( const Value: TRoof );
begin
FRoof.Assign(Value);
end;
The constructor *could* be written to accept the *literal* owner of the house, which I would argue should be allowed to be nil: THouse.Create( aOwner: TEntity );
Basically, you are saying “use Assert(Assigned(AParam))” for params which you expect to be valid objects – hard to argue here.
But “never” is overkill. Optional params, defaults, lazy init, “NULL”, “not found” – there are tons of valid and good examples.
Your code/docs should explicitly mark which params, fields, etc. can accept nil, and use Assert for everything else.
here we go:
function IsNIL(APtr: Pointer): Boolean;
NIL is an acceptable parameter 😉
Smart ass. 😉
I’ve passed in nil when using recursion to know that the end of the line had been reached. I didn’t like doing it. But the designer wanted it that way.
I know I promised yesterday that I will post my real code but I will break that promise.
There are two reasons for this:
1. After giving a closer look at my code I could probably get rid of the usage of nil in it with the approach that Bas Schouten has suggested to me.
2. Becouse coments don’t show code in friendly format it would be just a pain in the ass looking at it as it is not small.
But I have given this a lot of thought so I will present a different scenario to mr. Hodges and I wanna see how he will get rid of the usage of nil in such scenario.
Let us say you have a class object which contains field which is reference to some internal class object.
Now nirmall you would create this internal class object within the constructor of your class. Right? And you need to do this before you can assign pointer of that object to the field.
So what would you do if for some reason creation of the internal class fails? If this hapens you have no valid object to set the field to point to. And becouse nil is default initialization value for every pointer variable your Field will reamin nil.
So any further code of your that would be accepting Field as parameter will be actually acceptng nil.
How would you avoid that?
Maybe using Thomas Mueller suggestion where every method that would be returning some pointer would actually be doing this by modifying the external variable which you pass as out parameter and then actually returning the bolean value for a function result?
There are two major problem with this:
1. You are using aditional 8 bits of data for storing the bolean result.
2. It breaks code consistency. How does it breaks code consistency?
Take a look at other functions. Most of them returns their result directly and not through output parameter which gives you ability to assing the result of that function to desirable variable or pas it as parameter to another method directly with on line of code.
But with this suggestion you would be forced to use athleast local variable for that output parameter. So you can forget about passing whole method as parameter to another method. And that just complicates things even more.
Also mr. Hodges if the usage of nil is so bad why is that it is used now for so long (I belive it is as old as object oriented programming if not even older) and so often and in basically any higher level programming language that I know?
There are lots of things that are done for a long time before people realize that they are a a bad or inferior practice. There’s a reason no one uses procedural programming anymore…..
@silverwarior:disqus Glad I could be of help!
Thanks for your help but for now I’m sticking with my current approach (using nil). The main reasons for this are:
1. In order to fully integrate your proposal I would have to create a bunch of interfaces which would result in even more code and athleast to me in lesser readability.
2. This could make inheriting from the base TIngameUnit class more difficult. Currently I’m using up to 5 level of inheritance already.
3. And finally since I’m not used to working with interfaces there is a greater chance that I screw something up and introduce aditional bugs in it.
That is why at this time I prefer on better code redability (what is more readable to me) which is extremly important due to large code complexity.
You see for each ingame unit or entity I actually use two classes.
1. In first one I store all the data that is bsaically constant throughout the game and specific to certain unit type like MaxHealth, Unit descpiption, etc.
2. In second class I store all the data that is being changed often like CurrentHealth, Position, Orientation, etc. This class also has aditional field that stores reference to the first class. This is used by aditional properties that this class have which are used to actually forward the data from the first class. All interaction with specific unit is done through this second class.
So since multiple second classes can reference to the same first class this gives me a good data reusability for certain unit type and thus alows me to have huge amount of units in game field without using lots of memory.
With this approach alone I manged to reduce the memory usage for basic units down to about 30% of what would be needed othervise if I would be storing all the data in same clas nad parts of the same data would be multiplied. Infact the more units I have in game field the bigger the better memory saving percentage is.
And if you combine this with ability to equip ingame units with items which are other classes you get even better data reusability. So in the end I managed to reduce the memory requirements to less than 10% of what was originaly required.
So now you might understand better as of why I’m not incliend to use the approach you suggested.
I thought I responded to this but I don’t see it here —
Just because something has been done for a long time doesn’t make it the right thing to do. Things change, we learn and improve, and what was done in the past can become something that should no longer be done.
It is posible that they are better ways but so far I haven’t actually seen you suggest any.
The fact is that like any other variable or data type pointers require some default state for when they are intitialized. And this is nil.
So since there is no guarantee that you will always manage to assign pointer to proper object (creation of that object might fail) you catually can’t totaly avoid the use of nil. Soner or later you wil bump into it. And when you do it only depends on wheeter your code is written correctly to safely handle that or not.
You also claim that the use of nil leads to dangerous and Access Violation prone code.
Yes the chances to raise Access Violation becouse some pointer is nil are great. But that it still better than if the pointer would be uninitialized and therefore point to a random memory location and your code would then make changes to the memory there despite the fact that such memory could belong to compleetly different object.
So mr. Hodges let me ask you a question. What apporach would you rather use to make your code safe?
Simply check to see if the pointer is nill before trying to work with an object to which the pointer should be pointing?
Or perhaps perform some integrity check everytime to see if the poiunter indeed points to the correct object if that is even possible?
I would not accept nil at the point of entry. Refuse it, either by raising an exception or by setting the value to a null object.
I guess it depends on how you view nil. If you view it like null then it’s undefined. But if you attach meaning to it then it can be used. I would prefer having application domain related meaning to things. But some, especially speed freaks, like to go raw as much as possible.
“always” and “never” almost (see what I did there?) always lead to a list of edge case exceptions. It’s like a challenge.
Using “unless you have a really good reason…” might be better.
Unless we’re talking about WITH. NEVER use WITH. ALWAYS avoid using WITH. WITH doesn’t clarify – it obfuscates.
In my mind, I like to start with the absolutes and work backwards from there. Besides, it makes for a more interesting conversation. 😉
You should never do that. 🙂
Hehe
I should note, too, that I did caveat things in the post itself.
Eduardo –
What you are looking for there is the factory pattern, which you can use to create the correct THouse depending on the status of roof.
TStart = class
private
FConnection: TConnection;
public
constructor Create(AConnection: TConnection = nil);
end;
implementation
constructor TStart.Create(AConnection: TConnection = nil);
begin
FConnection := AConnection;
if FConnection = nil then
FConnection := RandomExampleConnection;
FConnection.Connect;
end;
You are doing exactly what I recommend — you aren’t accepting nil, and basically following the Null Object pattern.
http://en.wikipedia.org/wiki/Null_Object_pattern
//there is a case, I think it’s the Observer pattern, wich you have to pass the observer to the notifier (abstract classes):
TNotifier = class
private
FObserver: TObserver;
public
procedure Notify;
constructor Create(AObserver: TObserver)
end;
implementation
constructor TNotifier.Create(AObserver: TObserver);
begin
FObserver := AObserver;
end;
procedure TNotifier.Notify;
begin
if FObserver nil then
FObserver.Update;
end;
// In this case, I also could use a List for the observers, then I wouldn’t need to use the shity “if” in the Notify method, just a “for”, but in the Create I would need to do “if AObserver nil then List.add” going back to your point of view.
The action of returning a thing and checking if it exists are two different actions and thus should be implemented as different functions. Just because you can use retrieving an item and then checking assigned does not mean that is the best way to do it.
I would go as far to argue that nullable reference types are one of the worst characteristics of most modern OOP languages. We abstracted pointers away for a reason! This quote from Tony Hoare, the inventor of the null reference type, seems relevant here.
“I call it my billion-dollar mistake. It was the invention of the null reference in 1965. At that time, I was designing the first comprehensive type system for references in an object oriented language (ALGOL W). My goal was to ensure that all use of references should be absolutely safe, with checking performed automatically by the compiler. But I couldn’t resist the temptation to put in a null reference, simply because it was so easy to implement. This has led to innumerable errors, vulnerabilities, and system crashes, which have probably caused a billion dollars of pain and damage in the last forty years.”
Ryan — exactly! Excellent quote.
First what is your own code? And why rules for your own code should be different than for other code? What if your code is some high performant core library?
It seems to me that you are looking at nil only from higher level code perspective. Code that will and should fail if you pass nil. Code that cannot function properly if you pass nil. But there is also code where nil is valid value (state of object reference) and it is perfectly fine to pass nil to methods that accept nil as valid value, and also return nil as valid value.
Most obvious example for first one is TComponent.Create(AOwner: TComponent) where it is perfectly acceptable for AOwner to be nil. How else you would create some root component if you cannot pass nil to it? Ok, you can have separate parameterless constructor, but that just moves issue you are talking about to another level. You still have to deal with the fact that component Owner can be nil at any given time, and that you should handle nil gracefully, because it is valid value here.
Second example (returning nil from function) would be TCollection.Find(const Name: string): TCollectionItem wher you want to return item that matches Name parameter, but what you should return if such item could not be found. Nil, of course. On some higher level you may want to use null object pattern and not nil, but in base core libraries, creating and returning null object could be performance killer.
Nil should be taken with care, but avoiding nil completely would be like avoiding 0 in math just because you cannot divide with it.