3

Hi I'm looking at some old c# code and noticing a lot of code like this:

void SomeFunction()
{
    if (key.Length != Dimensions)
    {
        throw new KeySizeException();
    }
    else
    {
        SomeOtherFunction();
    }
}

I want to know if there can ever be a case where the else block is even necessary? Can I safely shorten the code to this with no repercussions?

void SomeFunction()
{
    if (key.Length != Dimensions)
    {
        throw new KeySizeException();
    }

    SomeOtherFunction();
}

By default the exception should throw the program flow out of this method right? But I'm just wondering if there's a way in DotNet to tweak how unhandled exceptions are um handled, that would cause the 2nd implementation to work differently from the first?

CodeAndCats
  • 7,508
  • 9
  • 42
  • 60
  • Thechnically they are the same. You can decide on the basis of readability. Some people find the else aids reading. – H H Jun 18 '11 at 10:10
  • Cheers, personally I find it more readable without it. Since it's littered through-out this old code I was starting to wonder if there could be side-effects to rewriting it. – CodeAndCats Jun 18 '11 at 10:48

5 Answers5

5

You do not need the 'else' block. It is redundant. If you use a refactoring tool like 'Reshaper' or 'JustCode' such redundant code elements are usually pointed out.

alwayslearning
  • 4,493
  • 6
  • 35
  • 47
  • Great, I didn't think there would be side effects to changing it but seeing it done over and over in this code I've inherited made me wonder. Knowing that refactor tools point this out to is just the reassurance I wanted, thanks. – CodeAndCats Jun 18 '11 at 10:49
3

The throw is an explicit terminal in that code block, the method call will effectively end at that point. This means the else block is redundant and can be removed.

Matthew Abbott
  • 60,571
  • 9
  • 104
  • 129
1

The two are completely equivalent.

Petar Ivanov
  • 91,536
  • 11
  • 82
  • 95
0

As others have said the two pieces of code are equivalent.

I thought I'd some additional thoughts though.

Firstly, the code as shown essentially implements a wrapper method (SomeFunction) that works as a guard clause for SomeOtherFunction. I'd be wary of doing that - when your KeySizeException is caught you will not know just from the stack trace that SomeOtherFunction was involved at all. It also means that you have no visibility of this requirement of SomeOtherFunction through simple code inspection of that method.

Additionally, you might conside refectoring these types of code to .Net 4.0 Code Contracts - they could give much easier to read code.

Final thought - in cases like yours I'm sometimes tempted to leave the else. This makes it 100% clear to someone else that the if/else behaviour is intended.

David Hall
  • 32,624
  • 10
  • 90
  • 127
  • Of course, I'm guessing that your real code looks quite different from your example - please ignore if so :) – David Hall Jun 18 '11 at 10:17
  • Hi David thanks for the advice. You're right in your comment though, I just dummied up this example, the real code is different. – CodeAndCats Jun 18 '11 at 10:45
0

In C# both works in the same way. I think you are thinking if you handle the exception (rather than throwing it), how to get rid of executing the second statement?

void SomeFunction() 
{     
   if (key.Length != Dimensions)     
   {  
       throw new KeySizeException(); //Halt the execution of SomeFunction method
   }
      SomeOtherFunction(); 
} 

If you handle and do not want to execute SomeOtherFunction, you can just return as below.

void SomeFunction() 
{     
   if (key.Length != Dimensions)     
   {  
       HandleMyException(); 
       return;    // Returns and halt the execution of SomeFunction method.
   }
      SomeOtherFunction(); 
} 
CharithJ
  • 46,289
  • 20
  • 116
  • 131