Exception handling

Jan 28, 2012 at 9:49 PM

I want to discuss with you one more thing - Dave asked me today (or maybe I should say wrote a comment for me in the code), what to do if program element passed as an argument to document factory create method is null - should we return null or throw an exception.

I think this is very important question. What will our plugin do, i.e. when user has saved the file, but there are compilation errors - like one closing bracket in a wrong place, or no space between method name and return type ("public voidDoSomething" instead of "public void DoSomething") - can the parser handle such situations? Or what should I do, when Lucene directory path for the Indexer doesn't exist (when I receive IOException)

I believe we should always throw an exception in all the places where we expect a valid data - i.e. I see no reason why I could receive an empty ProgramElement object. We also need to report these errors to the user - am I right? Here's the question for you - can we use output window from visual studio for the error logging? Or maybe our UI, where we show the search results can contain an execution log? I have no experience with VS plugins and I don't know what we should do in case of invalid data, not planned exceptions etc.

Let me know how in your opinion we should handle these situations, so that we choose one way to that and use it in all the places in our code

Jan 29, 2012 at 4:14 AM
I think we may want to be a bit lax about reporting certain types of problems to the user. For instance, I may want to search my project even if I have compilation errors in one or more files without Sando complaining about it. Of course there are going to be some fatal errors that we can not overcome, which probably warrant a message to the output window. I implemented the Parser with this philosophy in mind, throwing a ParserException only when I face fatal issues (e.g. cannot find the SrcML executable).

I'm new to VS extensions so cannot properly address your questions about where/how to log the errors.

Thanks for raising this concern.

On Jan 28, 2012, at 4:49 PM, lordlothar wrote:

From: lordlothar

I want to discuss with you one more thing - Dave asked me today (or maybe I should say wrote a comment for me in the code), what to do if program element passed as an argument to document factory create method is null - should we return null or throw an exception.

I think this is very important question. What will our plugin do, i.e. when user has saved the file, but there are compilation errors - like one closing bracket in a wrong place, or no space between method name and return type ("public voidDoSomething" instead of "public void DoSomething") - can the parser handle such situations? Or what should I do, when Lucene directory path for the Indexer doesn't exist (when I receive IOException)

I believe we should always throw an exception in all the places where we expect a valid data - i.e. I see no reason why I could receive an empty ProgramElement object. We also need to report these errors to the user - am I right? Here's the question for you - can we use output window from visual studio for the error logging? Or maybe our UI, where we show the search results can contain an execution log? I have no experience with VS plugins and I don't know what we should do in case of invalid data, not planned exceptions etc.

Let me know how in your opinion we should handle these situations, so that we choose one way to that and use it in all the places in our code


Jan 29, 2012 at 11:36 AM

What do you think about using code contracts for argument checking (http://msdn.microsoft.com/en-us/library/dd264808.aspx) and custom exceptions for situations like IOException catched? I'm going to add SandoException abstract class in the Core project and create IndexerException in Indexer project - we could use such inheritance structure to separate the exceptions we throw (for example only such can be shown to the user in the log) from the ones we don't expect - what do you think?

Jan 29, 2012 at 5:29 PM
Sounds like a nice plan to me

On Jan 29, 2012, at 6:36 AM, "lordlothar" <notifications@codeplex.com> wrote:

From: lordlothar

What do you think about using code contracts for argument checking (http://msdn.microsoft.com/en-us/library/dd264808.aspx) and custom exceptions for situations like IOException catched? I'm going to add SandoException abstract class in the Core project and create IndexerException in Indexer project - we could use such inheritance structure to separate the exceptions we throw (for example only such can be shown to the user in the log) from the ones we don't expect - what do you think?

Jan 29, 2012 at 10:11 PM

I've submitted the code with the exception classes, code contracts usage and translations (http://sando.codeplex.com/SourceControl/changeset/changes/47f7e62d65f8)

I won't create a review for that as here's the appropriate discussion.

Let me describe what I've done:

  1. Exceptions - I've created base SandoException class for plugin exceptions along with the IndexerException subclass. Because some (most?) of these exceptions (if occurred) will be shown to the user, I thought that the message itself should be translated to the appropriate language (as we are probably want to support multiple languages) - that's why you can pass 3 arguments to the constructor: message code (that will be used to find the appropriate resource), inner exception and additional format arguments (if message contains String.Format placeholders, i.e. "{0}").
  2. Translations - I've created a new Translation project with an enum with translation codes, and the static class Translator which translates the codes to the messages, using resources. For now translation code enumeration contains only the exception codes. I use the following naming convention: type of message, module, message code, i.e. Exception_Indexer_LuceneIndexAlreadyOpened or Exception_General_IOException. I've placed language specific labels in the Translations.resx file.
  3. Code contracts - I've introduced code contracts in DocumentIndexer class - in order to configure it, you need to install the latest Code Contracts library (http://msdn.microsoft.com/en-us/devlabs/dd491992) - it gives you the additional Code Contracts tab on the project configuration page, where you need to turn on contract checking.

Please, feel free to ask me anything - if you have any comments/concerns/improvements, etc. about these changes, let me know - I hope I did my best, but if not, I'll apply any changes that make this code better

Jan 30, 2012 at 8:56 PM
Edited Jan 30, 2012 at 9:06 PM

This all sounds good.  

As far as surfacing exceptions to the user, as Kosta mentions, I think we need to be conservative and only show the user exceptions when absolutely necessary (e.g., when SrcML or something foundation isn't available).  For instance, it would be painful if Sando was experiencing a 100s of parsing problems during indexing and Sando popped up 100s of dialogs to notify the user of that ;)

However, all the components except for the UI don't have to worry about this too much... just through SandoExceptions like lordlothar suggests and the UI plugin can decide whether to show the user an error, ignore it, or log it somewhere for the user to review later.  Hope this makes sense!

Jan 30, 2012 at 9:01 PM
davidcshepherd wrote:

This all sounds good.

As far as surfacing exceptions to the user, as Kosta mentions, I think we need to be conservative and only show the user exceptions when absolutely necessary (e.g., when SrcML or something foundation isn't available).  However, all the components except for the UI don't have to worry about this too much... just through SandoExceptions (as specified above) as needed and the UI plugin can decide whether to show the user an error, ignore it, or log it somewhere for the user to review later.

  So should I delete IndexerException and stay with only one custom exception type for the whole project?

I'm working currently on the contract failed event, so that I can get rid of throwing custom exception type in Requires method - I try to somehow catch this exception and it doesn't work for now - I'm going to check the code when I manage to fix it

Jan 30, 2012 at 9:08 PM

Thanks for adding the Translations project! It is important to tackle the internationalization problem early so we don't have to go through the painful process of fixing it later.  For anyone that adds a new string literal to the project please consider using the Translation project instead of a local constant or the literal itself.

Jan 30, 2012 at 9:11 PM
lordlothar wrote:
davidcshepherd wrote:

This all sounds good.

As far as surfacing exceptions to the user, as Kosta mentions, I think we need to be conservative and only show the user exceptions when absolutely necessary (e.g., when SrcML or something foundation isn't available).  However, all the components except for the UI don't have to worry about this too much... just through SandoExceptions (as specified above) as needed and the UI plugin can decide whether to show the user an error, ignore it, or log it somewhere for the user to review later.

  So should I delete IndexerException and stay with only one custom exception type for the whole project?

No, I think it is good to have the IndexerException (and other subclasses, as needed).  I didn't mean to suggest only using the SandoException... using subclasses of the SandoException to specify exactly what's going wrong seems better.  Is that what you think too or do you want to go with the single exception type?

Jan 30, 2012 at 9:12 PM
lordlothar wrote:

I'm working currently on the contract failed event, so that I can get rid of throwing custom exception type in Requires method - I try to somehow catch this exception and it doesn't work for now - I'm going to check the code when I manage to fix it

Haha... yes, I had some troubles setting up this Code Contracts stuff too!  However, I think it is worth it as we get to use cutting edge technology (i.e., Code Contracts is still just in DevLabs), which is half of the fun of open source anyway.  Anyway... good luck!

Jan 30, 2012 at 9:28 PM
Edited Jan 30, 2012 at 9:30 PM

 

davidcshepherd wrote:

No, I think it is good to have the IndexerException (and other subclasses, as needed).  I didn't mean to suggest only using the SandoException... using subclasses of the SandoException to specify exactly what's going wrong seems better.  Is that what you think too or do you want to go with the single exception type?

I definitely want go with subclasses - thanks for feedback!

One comment on code contracts for all of you guys - it is better when you use methods like "Requires", not "Requires<>" (the second one is implemented for backward compatibility and allows you to throw custom exception type when contract failed). The usage of the first one can be difficult, because ContractException class is not a public one - you cannot catch this exception. So if you would like to test the code (i.e. in unit test) with contract failing for some of the requirements, you should use ContractFailed event:

Contract.ContractFailed += (sender, e) =>
{
      e.SetHandled(); //this line says that the exception is handled - execution continues normally
      e.SetUnwind();  //this line prevents executing code after the contract checking
      contractFailed = true; //this is a private bool variable (you can use different approach),
                                      //a flag, which can be used for Asserts - example below
};

 Example of unit test:

Analyzer analyzer = new SimpleAnalyzer();
DocumentIndexer target = new DocumentIndexer("C:/Windows/Temp", analyzer);
try
{
      target.AddDocument(null);
}
//catch is still needed, because unwinding inside the ContractFailed event causes the exception to be thrown.
//However, at this moment we have our flag set
catch 
{
}
Assert.True(contractFailed, "Contract should fail!"); //contract failed - the flag is set if contract fails
target.Dispose();

If you have any questions - feel free to ask. The code is copied from DocumentIndexerTest class