locked
try/catch block should not be present in private level methods

    Question

  • Hi,

         Can any one please tell me how to make a check that try/catch blocks should not be present in private level methods. If the method is public then try/catch should be mandatory.

    Thanx in advance...

    Wednesday, May 02, 2007 7:28 AM

Answers

  • That seems like an odd rule, however, if you really want to implement this, simply walk the instructions looking for OpCode._Catch:

     



     
    public ProblemCollection Check(Member member)
      {
          Method method = member as Method;
        if (method == null)\
             return null;
     
        for (int i = 0; i < method.Instructions.Length; i++)
          {
              if (methods.InstructionsIdea.OpCode == OpCode._Catch)
              {
                  // We found a catch handler
              }
          }
      }
     

     

     

    Regards

     

    David

    Wednesday, May 02, 2007 7:25 PM
  • While I agree that it's a very odd rule, I suspect that the intent for public methods may be a little more complex than simply finding any catch.  Instead, I'd have to guess that Vikrant probably wants a "wrapping" try/catch in all public methods, which is quite a bit trickier to identify.  The basic logic should probably be more along the lines of:

    1. An exception handler must exist.
    2. A catch statement must be found for the topmost exception handler.
    3. There should not be more than one exception handler at level 0.
    4. No "significant" instructions (e.g.: anything other than OpCode.Nop, OpCode._Locals, or OpCode.Ret) should be located outside the topmost exception handler.

    Assuming that one does not want to generate violations for methods that don't actually contain any "interesting" code, the problem detection would look a bit more like this:

     

    Code Snippet

    private static bool HasProblem(Method method)

    {

        bool nontrivialInstructionsFound = false;

        bool instructionsFoundOutsideHandler = false;

        bool catchFound = false;

        int topLevelTryCount = 0;

        int currentHandlerLevel = 0;

     

        foreach (Instruction instruction in method.Instructions)

        {

            switch (instruction.OpCode)

            {

                case OpCode.Nop:

                case OpCode._Locals:

                case OpCode.Ret:

                    break;

                case OpCode._Try:

                    if (currentHandlerLevel == 0) topLevelTryCount++;

                    currentHandlerLevel++;

                    break;

                case OpCode._Finally:

                case OpCode._Filter:

                    currentHandlerLevel++;

                    break;

                case OpCode._Catch:

                    if (currentHandlerLevel == 0) catchFound = true;

                    currentHandlerLevel++;

                    break;

                case OpCode._EndTry:

                case OpCode._EndHandler:

                case OpCode.Endfinally:

                case OpCode._EndFilter:

                    currentHandlerLevel--;

                    break;

                default:

                    nontrivialInstructionsFound = true;

                    if (currentHandlerLevel == 0) instructionsFoundOutsideHandler = true;

                    break;

            }

        }

     

        bool hasProblem = false;

        if (nontrivialInstructionsFound)

        {

            if ((!catchFound) || instructionsFoundOutsideHandler || (topLevelTryCount > 1)) hasProblem = true;

        }

     

        return hasProblem;

    }

     

    Thursday, May 03, 2007 2:05 PM

All replies

  • That seems like an odd rule, however, if you really want to implement this, simply walk the instructions looking for OpCode._Catch:

     



     
    public ProblemCollection Check(Member member)
      {
          Method method = member as Method;
        if (method == null)\
             return null;
     
        for (int i = 0; i < method.Instructions.Length; i++)
          {
              if (methods.InstructionsIdea.OpCode == OpCode._Catch)
              {
                  // We found a catch handler
              }
          }
      }
     

     

     

    Regards

     

    David

    Wednesday, May 02, 2007 7:25 PM
  • While I agree that it's a very odd rule, I suspect that the intent for public methods may be a little more complex than simply finding any catch.  Instead, I'd have to guess that Vikrant probably wants a "wrapping" try/catch in all public methods, which is quite a bit trickier to identify.  The basic logic should probably be more along the lines of:

    1. An exception handler must exist.
    2. A catch statement must be found for the topmost exception handler.
    3. There should not be more than one exception handler at level 0.
    4. No "significant" instructions (e.g.: anything other than OpCode.Nop, OpCode._Locals, or OpCode.Ret) should be located outside the topmost exception handler.

    Assuming that one does not want to generate violations for methods that don't actually contain any "interesting" code, the problem detection would look a bit more like this:

     

    Code Snippet

    private static bool HasProblem(Method method)

    {

        bool nontrivialInstructionsFound = false;

        bool instructionsFoundOutsideHandler = false;

        bool catchFound = false;

        int topLevelTryCount = 0;

        int currentHandlerLevel = 0;

     

        foreach (Instruction instruction in method.Instructions)

        {

            switch (instruction.OpCode)

            {

                case OpCode.Nop:

                case OpCode._Locals:

                case OpCode.Ret:

                    break;

                case OpCode._Try:

                    if (currentHandlerLevel == 0) topLevelTryCount++;

                    currentHandlerLevel++;

                    break;

                case OpCode._Finally:

                case OpCode._Filter:

                    currentHandlerLevel++;

                    break;

                case OpCode._Catch:

                    if (currentHandlerLevel == 0) catchFound = true;

                    currentHandlerLevel++;

                    break;

                case OpCode._EndTry:

                case OpCode._EndHandler:

                case OpCode.Endfinally:

                case OpCode._EndFilter:

                    currentHandlerLevel--;

                    break;

                default:

                    nontrivialInstructionsFound = true;

                    if (currentHandlerLevel == 0) instructionsFoundOutsideHandler = true;

                    break;

            }

        }

     

        bool hasProblem = false;

        if (nontrivialInstructionsFound)

        {

            if ((!catchFound) || instructionsFoundOutsideHandler || (topLevelTryCount > 1)) hasProblem = true;

        }

     

        return hasProblem;

    }

     

    Thursday, May 03, 2007 2:05 PM