RSPEC-S112 invalidly includes useful exceptions

Clean Code Report False-positive / False-negative...
,
1

IndexOutOfRangeException & NullReferenceException Do not qualify as General Exceptions.
These exception are 100% specific to a specific purpose, use, reason, and situation.

NullReferenceException:

public class MyClass 
{
  public MyType MyProp {get; set;}
  
  public void DoSomething(object arg) 
  {
    if (arg == null) throw new ArgumentNullException(); //this is ok
    if (this.MyProp == null) throw new ArgumentNullException(); //this is NOT okay.
    if (this.MyProp == null) throw new NullReferenceException(); //According to RSPEC-112 this is also not okay,    
  }
}

I SImply want to make sure that it “shouldn’t” happen that the MyProp is null, but if it is, that is an exceptional situation, which needs to be indentified with an Exception. I believe the appropriate for this situation is “ArgumentNullException”, but tradition has defined “argument” as “method parameter only”. So the second best would be “NullReferenceException”. But simply having the generic “Object reference not set to an instance of an object”, i want to customize the message for the situation so my error handler that exports the error to a log can tell me right then and there what the situation is, without having to look it up in a stack trace. It would be nice if there was a “PropertyNullException” but there isn’t. I could invent one, but now I’ve made a specific exception that needs to be supported in our code base, which is in effect reinventing the wheel while creating more code base to support.

IndexOutOfRangeException:
Even more than the NRE, this has a very distinct meaning: “the index is out of range”. I’m not allowed to tell whoever called my method that the index was out of range? I’m not good enough or smart enough to pick the situation where saying, “Hey, dude, the index is out of range,” is appropriate! Are you kidding me!!!
This is not a “general” exception it is the very definition of specific.

Object[] A = GetObjectAArray();
Object[] B = GetObjectBArray();
return A.Select((c, i) => (c, i))
   .ToDictionary(x => x.c.ToString(), x => B[x.i]);

Now, if B is larger than A no problem. But if B is less than A, you will get an IndexOutOfRangeException. A very generic index out of range exception which has no bearing or meaning up on the situation. The Message is what is generic about that exception.
However, the intent in this situation is to provide a reasonable error message so the IndexOutOfRange problem isn’t solved in the code. I get a generic Out of Range error, i’m thinking somebody didn’t check their indexer’s correctly. But sometimes, it’s the external input variables that lead to this scenario, so instead of again “rewriting the wheel”, I reuse the IndexOutOfRange , but provide a more appropriate error message to the cause of the IOOR…

So the correct, NON code smell version looks like this:

Object[] A = GetObjectAArray();
Object[] B = GetObjectBArray();
if (B.Length < A.Length) throw new IndexOutOfRangeException($"My Custom Error Message for describing How B was shorter than A");
return A.Select((c, i) => (c, i))
   .ToDictionary(x => x.c.ToString(), x => B[x.i]);

I don’t have to invent new exception classes, the usage is 100% accurate, because the index would have been out of range had i let the next statement execute, yet the exception message is now 100% clear for the scenario. (You know, i think that’s why they have constructors that allow programmers to create them, unlike the SqlException, which I’d love to be able to create, but I can’t because the constructor is protected/private. Hey Polymorphism, isn’t it amazing!)

RSPEC-S112 would have me rewrite that same scenario like this:

try 
{
   Object[] A = GetObjectAArray();
   Object[] B = GetObjectBArray();
   return A.Select((c, i) => (c, i))
      .ToDictionary(x => x.c.ToString(), x => B[x.i]);
}
catch (IndexOutOfRangeException ex) 
{
   throw new MyWrapperException("my custom error message", ex);
}

Now that is EXTREMELY expensive since I’m catching only to throw. Catching to throw is a WAY worse codesmell than resusing an exception that is exactly the exception I intended to use.

Those rules and specs are about exceptions like System.Exception or System.ApplicationException. Those make sense. They are as general as can be.

But IndexOutOfRange and NullReference? Those are specific, appropriate exceptions that only insanity would expect me to reinvent for a situation that directly applies.
It is one thing to discuss an actual “code smell”. it is another cause more code smells because of bad interpretation of coding concepts.

VS2019
SonarAnalyzer.CSharp.7.13.0.8313
but the rule RSPEC-S112 is global to all the supported tools, so version of Sonar Qube doesn’t matter.

2

Hi @JaedenRuiner,

As you can see in the Microsoft docs the NullReferenceException is used when

The exception that is thrown when there is an attempt to dereference a null object reference.

Since you’re not dereferencing your MyProp yet, this is not a relevant exception for you. You should throw InvalidOperationException, because your object state is invalid. See InvalidOperationException.

In your second example with

if (B.Length < A.Length) throw new IndexOutOfRangeException($"My Custom Error Message for describing How B was shorter than A");

you also haven’t really tried to access any index yet. You have invalid state of your program at this point, so you should throw InvalidOperationException here as well.

So both exceptions behave as expected and as specified. In case this rule doesn’t help you with your specific code base, you can simply deactivate the rule.