Review: Rock Your Code: Defensive Programming for Microsoft .NET by David McCarter
I saw a post on Twitter from a guy named David McCarter promoting his new book. Rock Your Code: Defensive Programming for Microsoft .NET. I thought the title looked interesting so I bought it.
This book (if approximately 50 pages of A5 can count as a book) was packed with bad advice. It is going to be more effort to explain why than the book deserves, but I will do it.
I'm not sure exactly what he means by that. How do you write a unit test for encapsulation? If you write a test that tries to read a private member of a class then it won't compile.
One might assume he would give an example of how to unit test for encapsulation but he doesn't, so I am none the wiser.
My tip for defensive programming: Always test your ideas at least once.
What will happen when you try to set the dates like so?
You might think it's okay and that you can just set the EndDate first, and that will work for a new instance, but what if you are changing an existing instance?
You certainly don't want to be writing code like this...
Not only is it a lot of extra work, but it also takes a second or two to work out what exactly it is doing...or why it is doing it.
Another problem is deserialization. If you need to serialize this and send it over a wire to be deserialized, can you guarantee the deserializer will set the properties in the correct order? If you retrieve this Order from Entity Framework, can you ensure that EF will set the property values in the correct order? Of course not!
In my section on Chapter 1 I said I would address some code that the author had written in his property setter. This is that code.
The problem with this code is pretty obvious, I think. It is impossible to retrieve an old purchase order from a database. As soon as you get something like Entity Framework to retrieve this order from your database it will throw an ArgumentOutOfRangeException because the DateShop is less than the current moment in time.
Literally the moment you serialize and deserialize it, it is invalid. You can't even create an instance in a client app and send it over the Internet to a Web API (or vice versa), because as soon as 1 tick elapses (one ten-millionth of a second) it is impossible to deserialize the date into an instance of that class.
And that's ignoring the fact that the client machine's system clock might be a second or two different from the server.
The capitalised T on the word "Try" hints that the author is actually writing this source code directly into Microsoft Word, which would explain why some of it doesn't even compile, and why apparently none of it has been tested. But I digress....
This example makes me want to scream out
The author explains that sometimes us programmers forget to wrap our code with catch-all exception handlers. To combat this problem (and to be clear, it is definitely not a problem) we should, wait for it....
TL;DR
Source code doesn't compile. When it does, it doesn't do what the author says it does. The author promotes practices long established as poor.
Chapter 1: OOP
This chapter explains the three pillars of OOP are Encapsulation, Inheritance, and Polymorphism. The author has a tiny 1 or 2 sentence explanation of each.
He then goes on to show an example of some poor source code "from a real in production project" (I hope he has permission from the copyright holder). The source code is a single class with lots of public attributes.
That's the end of the chapter. Honestly, that's it!
One of the things that struck me about this code was the property validation. At first I treated it as illustrative and that he wasn't suggesting you actually write code like this, but having read the rest of the book it is apparent that he sees it as a genuinely valid way to validate a property. I will explain the problem in the section about Chapter 3, Rule #5.
Chapter 2: Introduction to Defensive Programming
Any code that might cause an exception such as accessing files, using objects such as DataSets should always check the object for null so an exception is not thrown.That is practically the entire content of this chapter, other than that he criticises programmers he has worked with in the past for having 1,700 unit tests and not one of them test for encapsulation.
I'm not sure exactly what he means by that. How do you write a unit test for encapsulation? If you write a test that tries to read a private member of a class then it won't compile.
One might assume he would give an example of how to unit test for encapsulation but he doesn't, so I am none the wiser.
Chapter 3: Stop exceptions before they happen
This part of the book is separated into different, very short, sub sections.
Rule #1 - Code
In summary he advises you check objects aren't null, arrays aren't empty, etc rather than making assumptions. I don't have any objection to this advice, it is (as he says) better than letting an unexpected exception occur.
Rule #2 - Argument values
He advises you check argument values on public/protected methods and throw a relevant exception if necessary. Good advice again, but the example is not good.
public Color Average(IEnumerable<color> colors)
{
if (colors?.Count < 1)
throw new ArgumentNullException("colors");
int r = colors.Average(c => c.R).Round();
}
The code clearly hasn't been tested because it won't compile. Count is a method, he is missing his parentheses. Authors really must compile their code first, check it does what they claim it does, and then paste it into their books. He repeats this error in his textual explanation.
It also won't compile because Round() does not exist. In order to get it to compile I changed it to int r = (int)Math.Round(colors.Average(c => c.R));
I would also recommend using nameof(colors) in the exception rather than a literal string. This ensures the text in the exception always matches the name of the parameter if ever someone changes it.
He should not throw an ArgumentNullException when the argument value isn't null, he should throw an ArgumentException.
But this final criticism (so much criticism for such a tiny piece of code) is that even when you fix the compiler error, it doesn't work! If you actually try executing this code and pass in a null value you will find (colors?.Count() < 1)> will never evaluate to true. The reason is that the null coalesce will return a Nullable for null.Count(); the value of which will be null, and null will never be considered to be less than 1!
The author goes on to say:
As you can see in the code above, the if(colors?Count < 1) code validates that the collection is not null and then checks the count, otherwise as soon as the code block hits the first colors.Average, it would cause an exception.But due to the above problem with his "if" statement that is exactly what happens! His solution to avoiding a NullReferenceException doesn't solve the problem. Not only has the author not checked his example code compiles, he hasn't even checked his concept is valid.
My tip for defensive programming: Always test your ideas at least once.
Rule #3 - Enumerations
Because you can cast any integer value as an enum, the author recommends checking that passed values are valid by using System.Enum.IsDefined - I don't have a strong opinion on this recommendation.
Although I was surprised the author didn't recommend throwing a NotImplementedException for unhandled enum values in switch statements. This prevents the developer from adding a new enum state and then accidentally allowing code to simply fall through a switch statement without being explicitly told it may.
Although I was surprised the author didn't recommend throwing a NotImplementedException for unhandled enum values in switch statements. This prevents the developer from adding a new enum state and then accidentally allowing code to simply fall through a switch statement without being explicitly told it may.
Rule #4 - Casting Types
The author recommends that instead of hard casting from one type to another, like this
ErrorLogger logger = new ErrorLogger();
var loggerTest = (ILogger)logger;
you should do this instead
ErrorLogger logger = new ErrorLogger();
var loggerTest = logger as ILogger;
if (loggerTest != null)
{
loggerTest.Save("Test message");
}
else
{
// Handle error in expected interface
}
It begs the question, how do you "Handle error in expected interface"? The correct way to handle such an error would be to throw an InvalidCastException - but that's exactly what hard-casting the type does. The author seems to be encouraging the reader to add lots of boilerplate to their source code that is already implemented for them by the .NET framework.
Rule #5 - Let Type Checking Work for You
This section is awful. The author takes the example he provided in chapter one and shows how he would have written the code in question. The only useful thing he does here is to use the proper .NET types for his properties. The original example used String types for dates etc, the author has changed them to DateTime. Hardly worth mentioning. In fact, not worth mentioning at all.
However, what the author does do, and very poorly so, is to add validation to property setters. For example:
public DateTime OpenedOn
{
get
{
return _openedOn;
}
set
{
if (value <= DateTime.MinValue)
throw new ArgumentOutOfRangeException();
_openedOn = value;
}
}
I don't think the author understands what DateTime.MinValue actually is. It is the lowest value that type can possibly hold. If it were possible to represent a DateTime value as 1 millisecond before DateTime.MinValue then DateTime.MinValue would not actually be DateTime.MinValue.
In other words, the author is protecting against an impossible scenario. He would have realised if only he had tested his hypothesis that it was necessary - as I did before blogging just to make sure I wasn't incorrect in my criticism.
Another example from this same class:
public DateTime? CustomerPickupOn
{
get
{
return _customerPickupOn;
}
set
{
if (this.OpenedOn < DateTime.MinValue)
throw new ArgumentOutOfRange(nameof(OpenedOn));
// Omitted for brevity - validates the value passed and sets the private attribute value
}
}
The author is validating the value of OpenedOn inside the setter for CustomerPickupOn instead of CstomerPickupOn (or, better still, just value). He does the same in his CloseOn property too. Not only is he validating property X when you set a value for property X, he is also validating property Y.
But that's not all, he is duplicating the validation code! If you were to validate in this way (which you definitely should not) then at least have a single private method that performs the validation and call it. You should never duplicate code; it is the sole purpose of the acronym DRY (Don't Repeat Yourself).
At the start of this blog I mentioned the source code the author presented had a poor approach to validation. Now that you have seen how the author proposes you validate the state of your objects I think it is clear that he recommends having business rules validation on property setters.
Business rule validations are things such as "StartDate cannot be after EndDate". The reason you do not put such validation into your property setters is because you are forcing the user of your class to set properties in a specific order. For example, if you did have a class like the following
public class DateRange
{
private DateTime _startDate;
public DateTime StartDate
{
get => _startDate;
set
{
EnsureDateRangeIsValid(value, EndDate, nameof(StartDate));
_startDate = value;
}
}
private DateTime _endDate;
public DateTime EndDate
{
get => _endDate;
set
{
EnsureDateRangeIsValid(StartDate, value, nameof(EndDate));
_endDate = value;
}
}
private void EnsureDateRangeIsValid(DateTime start, DateTime end, string propertyName)
{
if (start > end)
throw new ArgumentException($"{nameof(StartDate) } cannot be after {nameof(EndDate)}", propertyName);
}
}var range = new DateRange();
range.StartDate = DateTime.Today;
range.EndDate = DateTime.Today.AddDays(1);You might think it's okay and that you can just set the EndDate first, and that will work for a new instance, but what if you are changing an existing instance?
range.EndDate = DateTime.Today.AddDays(1);
range.StartDate = DateTime.Today;You certainly don't want to be writing code like this...
if (range.StartDate > DateTime.Today.AddDays(1))
{
range.EndDate = DateTime.Today.AddDays(1);
range.StartDate = DateTime.Today;
}
else
{
range.StartDate = DateTime.Today;
range.EndDate = DateTime.Today.AddDays(1);
}Not only is it a lot of extra work, but it also takes a second or two to work out what exactly it is doing...or why it is doing it.
Another problem is deserialization. If you need to serialize this and send it over a wire to be deserialized, can you guarantee the deserializer will set the properties in the correct order? If you retrieve this Order from Entity Framework, can you ensure that EF will set the property values in the correct order? Of course not!
In my section on Chapter 1 I said I would address some code that the author had written in his property setter. This is that code.
private DateTime _dateShop;
public DateTime DateShop
{
get
{
return _dateShop;
}
set
{
if (value < DateTime.Now)
{
throw new ArgumentOutOfRangeException();
}
}
}The problem with this code is pretty obvious, I think. It is impossible to retrieve an old purchase order from a database. As soon as you get something like Entity Framework to retrieve this order from your database it will throw an ArgumentOutOfRangeException because the DateShop is less than the current moment in time.
Literally the moment you serialize and deserialize it, it is invalid. You can't even create an instance in a client app and send it over the Internet to a Web API (or vice versa), because as soon as 1 tick elapses (one ten-millionth of a second) it is impossible to deserialize the date into an instance of that class.
And that's ignoring the fact that the client machine's system clock might be a second or two different from the server.
Rule #6 - Check Resources
The author recommends pinging Google, Microsoft, and Yahoo in order to ensure you are connected to the Internet before allowing your users to perform actions that require an Internet connection.
This is a huge overhead. I strongly recommend you do not take this advice.
Besides, those sites might be up but the server you want to contact is down. It is far better to attempt your action and then gracefully handle any errors that occur.
Besides, those sites might be up but the server you want to contact is down. It is far better to attempt your action and then gracefully handle any errors that occur.
Rule #7.1 - Validate User Input
Actually, some sound advice here! Literally the first part of the book that I agree with. Essentially it advises to validate user input on both the client and the server, just in case the content has been altered en-route.
In fact, to ensure data is not altered en-route you should use SSL. The real reason you perform validation on the server is that you cannot be 100% certain that it is your client sending the request, and therefore the data might not already be validated.
Make validation easier
I am assuming this is Rule #7.2.
The author recommends you use his open-source library called dotNetTips.Utility. I make some observations on this general subject when I comment on Appendix A.
The author recommends usage of a pattern using this library which looks like this
public bool Contains(T item)
{
Encapsulation.TryValidateParam<ArgumentNullException&t;(item != null, nameof(item));
// Code removed for breavity [SIC]
But he doesn't explain what it does, why it does it, or what it is an alternative to. It seems to be a substitute for the following code
if (item == null)
throw new ArgumentNullException(nameof(item));
Which is actually quicker to type, executes quicker, and doesn't require a 3rd party library.
Dealing with exceptions
If what I have written so far does not put you off reading this book then I will be surprised but, I am sure you won't be surprised when I tell you the book does not get any better!
Rule #1 - The Application Layer
The author says
Make sure to wrap code that could cause an exception (which is just about everything) with try/catch blocks.and then gives the following example code
Try
{
// Author's code not shown
}
catch (Exception ex)
{
LogWriter.WriteException(ex, TraceEventType.Error, "Error updating patient, missing video information.");
}The capitalised T on the word "Try" hints that the author is actually writing this source code directly into Microsoft Word, which would explain why some of it doesn't even compile, and why apparently none of it has been tested. But I digress....
This example makes me want to scream out
NEVER CATCH ALL EXCEPTIONS AND SIMPLY WRITE THEM OUT TO A LOG FILE!Okay, so I feel a bit guilty that perhaps I am presenting a straw-man argument. My good nature makes me want to give the author the benefit of the doubt. Perhaps this was a typing error of some kind? Perhaps he accidentally omitted the line that rethrows the exception?
The author explains that sometimes us programmers forget to wrap our code with catch-all exception handlers. To combat this problem (and to be clear, it is definitely not a problem) we should, wait for it....
Rule #2 - Trap Exceptions Globally
Yes, that's right, we should trap exceptions globally. Here is the author's proposal for handling errors in an ASP.NET application. In your Global.Asax file add the following code to the global exception handler.void Application_Error(object sender, EventArgs e)
{
LogWriter.WriteException(Server.GetLastError(), TraceEventType.Critical, "Global Application Error", "Application error. Please contact admin.");
Server.ClearError()
}Rule #3 - The DLL Layer
The author recommends that DLLs should only catch specific exception types, never exceptions like System.Exception, System.ApplicationException, or System.SystemException.
An example of what he means looks like this
try
{
adminSqlConnection.Open();
}
catch (SqlException ex)
{
adminSqlConnection.Close();
throw new ActivationException("Unable to get activation code. Does user have permission?", ex);
}
I generally agree with this part of the book.
What I think he is trying to advice is that if ever you do have a catch-all exception handler then it should always re-throw the exception. The only time you don't need to re-throw the exception is when you understand the consequences of that exception and know how to deal with it in a way that leaves your application in a known (and acceptable) state. Or perhaps I am being too generous?
Logging
Logging is very important, I agree. I too agree with the advice that you should generally use something like Log4Net (because it is well established). I gave a little chuckle when I read the author had written the following advice.
Check out my logging system (described later in this book) or use a different one like Log4Net. Don’t roll your own!
Source Code Check-in Workflow
The author describes all of the steps he takes before checking source code into a version control system, such as testing etc. I don't have any strong opinions on what he has written here, although I think it is a shame he didn't follow those steps when writing the source for this book.
At one point the author claims he has estimated that by following these steps he has saved one of the companies he worked for over three million US dollars, per year.
Appendix A - dotNetTips.Utility
I am fundamentally against "utility" libraries. Libraries should focus on solving a single problem, that way the consumer can use only exactly what they need. Utility libraries are typically just bloat.
Appendix B - dotNetTips.com Apps
The author promotes an application that clears down cache files, which he claims solves problems such as unit tests no longer working.
He states another benefit as being that it searches your hard disk for source code files and back them up. For some reason I cannot understand, the feature that also backs up files marked with the Archive flag set is called "Turbo backup".
He claims the purpose of this application is as follows
I use this feature to backup source files before retrieving source from source control (since some of these programs wipe out code changes) and at the end of the day.
If the author believes the way to recover older version of source code after changes have been made is to revert to a copy that he manually backed up, then I have to question whether he understands how source control works.
Conclusion
This book is awful. Never before have I spent so much time reviewing a book (and I use the word "book" very loosely).
It's so easy to write something bad; it's often more effort to explain why it is bad. I wouldn't be surprised if my review is actually longer than the book itself.
If you want to be a good programmer then I advise you to read this book, and then avoid doing just about everything it advises you do.
Comments