I love these, although I am still unsure as to how clean they make the code, or if they are going to make me a lazy coder by using them as opposed to what I have been doing.
I came across the null dereference because I was reviewing some code on my favorite code review site, CodeReview. The code is being used to track downloaded applications in a set of applications that are being downloaded at one time.
if (lastRuntimeDetails != null) //at least one application was already downloaded if (lastRuntimeDetails.Ip != runtimeDetails.Ip || lastRuntimeDetails.Port != runtimeDetails.Port) _requestExecutor?.Disconnect();
As I was reviewing this code there was an awkward if statement that was nested without brackets (yuck) so that was my first line of business. The objects that were being compared were being created from a custom class called RuntimeDetails, the first object that is created is called lastRuntimeDetails and is being used to track the last application to make sure that the same application doesn't get downloaded a second time and to keep track of the IP Address and Port for each download.
Anyway, at first I wanted to get rid of the outer if statement, it looked to me like it was extraneous, at first I was thinking that if the properties were null that they just wouldn't be equal to each other and give a false value to the condition statement.
Comments were made about Null Reference Exceptions and a lot of thought went into how to do this, I kept thinking about the null properties, and that's when I found Null Dereference Operator, so I set up some tests to make sure that I had a good grasp on things
I created two classes, one to create objects and the other to return comparisons on the objects
public class Class1 { public string One { get; set; } public string Two { get; set; } public Class1() { } } public static class Class2 { public static bool returnBoolean(Class1 inputA, Class1 inputB) { return (inputA?.One == inputB.One); } public static bool checkOnNullProperty(Class1 inputA, Class1 inputB) { return (inputA.One == inputB.One); } }
and then I eventually came up with the following tests
[TestMethod] public void TestMethod1() { var input1 = new Class1() { One = "string" }; var input2 = new Class1() { Two = "string B Two" }; var test = Class2.returnBoolean(input1, input2); Assert.IsFalse(test); } [TestMethod] public void TestMethod2() { var input1 = new Class1() { One = "string" }; var input2 = new Class1() { One = "string B Two" }; var test = Class2.returnBoolean(input1, input2); Assert.IsFalse(test); } [TestMethod] public void TestMethod3() { var input1 = new Class1() { One = "string" }; var input2 = new Class1() { One = "string" }; var test = Class2.returnBoolean(input1, input2); Assert.IsTrue(test); } [TestMethod] [ExpectedException(typeof(NullReferenceException))] public void TestMethod4() { var input1 = new Class1() { One = "string" }; Class1 input2 = null; var test = Class2.returnBoolean(input1, input2); Assert.IsFalse(test); } [TestMethod] [ExpectedException(typeof(NullReferenceException))] public void TestMethod5() { var input1 = new Class1() { One = "string" }; Class1 input2 = null; var test = Class2.checkOnNullProperty(input1, input2); Assert.IsFalse(test); } [TestMethod] [ExpectedException(typeof(NullReferenceException))] public void TestMethod6() { Class1 input1 = null; Class1 input2 = new Class1() { One = "string" }; var test = Class2.checkOnNullProperty(input1, input2); Assert.IsFalse(test); }
Someone pointed out that if the object itself is null it will throw a Null Reference Exception, once they said that I looked at the code again and sure enough the container was created but no object was placed inside of it.
RuntimeDetails lastRuntimeDetails = null;
So my initial thought of removing that outer if statement was wrong, but now I had new information that could be used to make sure that the Properties didn't throw a Null Reference Exception if the object was created but the property were null.
So here is the code that I suggested to replace the original code.
if (lastRuntimeDetails != null) { if (lastRuntimeDetails?.Ip != runtimeDetails?.Ip || lastRuntimeDetails?.Port != runtimeDetails?.Port) { _requestExecutor?.Disconnect(); } }
The object could be created with null properties now and then we could remove the outer if statement entirely. I believe that the better solution is to set the properties on creation to some default and then when they are assigned they will be different from the default so the comparison could be made without the need to check for nulls thus removing the chance that future development would have to worry about checking for nulls. or that the initial creation of the object, instead of being set the object to a null object, we create the object with null properties and then if the object's properties are null then they will give a false when compared to anything other than null (using the Dereference)
The Code Review Question in Question
and
My Answer
