CODEWITHSUNDEEP
c#
Niels J.
Niels J.

Reputation: 81

Parameter validation, or let it fail?

Aprroach 1:

public static void SendMail(string from, string to, string subject, string body)
{
   if(String.IsNullOrWhiteSpace(from))
      throw new ArgumentNullOrWhiteSpaceException("from");

   if(String.IsNullOrWhiteSpace(to))
      throw new ArgumentNullOrWhiteSpaceException("to");

   var msg = new MailMessage(from, to, subject, body) { IsBodyHtml = true };

   using(var smtp = new SmtpClient())
      smtp.Send(msg);
}

Approach 2:

public static void SendMail(string from, string to, string subject, string body)
{
   var msg = new MailMessage(from, to, subject, body) { IsBodyHtml = true };

   using(var smtp = new SmtpClient())
      smtp.Send(msg);
}

Why would I validate the parameters in approach 1, and not just wait for MailMessage to throw an exception (approach 2), telling me that I have passed a empty from or to value, to the constructor?

So why would I throw my own exception?

Upvotes: 8

Views: 255

Answers (4)

Filip
Filip

Reputation: 652

smtp.SendMail will throw an InvalidOperationException (in the system namespace)

In this you are throwing a more suitable exception type, from the type type it easier to understand and catch the exception. InvalidOperationException is a very general class. By throwing your own exception the code is more readable and the same exception can be handled even if you change the method at a later point, for example by using another mailclient.

Upvotes: 1

Simon
Simon

Reputation: 6363

In general, I think that throwing your own exception is justified when you can give more relevant information than the function you're calling can (either to handle a more specifix exception in your code or to return a better error message to the user).

In this case it seems that you wouldn't add any information that Send() couldn't.

Upvotes: 1

Nahum
Nahum

Reputation: 7197

erm... are you SURE in 100% that var msg = new MailMessage(from, to, subject, body) { IsBodyHtml = true };

will throw those exceptions? approach 2 is undefined behavior. how should this be unit tested?

what does SendMail("","","") do? extacly? its not clear from the second approach.

you COULD add a comment. but WHY? thats not clean code

arroach 1 clearly defines where the function is going to fail. and how it is going to handle failures. your code should make clear what is does, not comments.

P.S.

throw new ArgumentNullOrWhiteSpaceException("from"); is throwed in your SendMail function, closest to the source of the problem.

if you use approach 2 only god knows how deep in your call hirerchy it will be caught if at all.

also you could improve this by writting somethinf like:

ArgumentNullOrWhiteSpaceException("from - this is usually caused if poo is not bared by poo in goo"); ` that could simplify your life a few month later.

Upvotes: 0

Ant P
Ant P

Reputation: 25231

The reason for this is quite simple - it makes it easier to debug.

For any given method (and this is truer of more complex methods) that requires a non-null parameter, it's going to be much easier for someone debugging an exception scenario to see an explicit exception from SendMail saying, "hey, 'from' is null; I need it not to be," than it is to have some method call within SendMail (or even some nested method call within that) throw a NullReferenceException (which, ultimately, is what will happen if none of the methods in question perform null checks).

Then, you have the scenario whereby - 6 months down the line - you decide that SendMail needs to do something else; e.g. (as a trivial example) set some kind of audit flag in a database. Now, if you just let the method fall over, you have an invalid flag (or you may do, depending on the order of things inside your method). Much better to say "actually, if my parameters are invalid, just fail immediately," rather than let the method carry on and have potential side effects.

Upvotes: 4

Related Questions