views:

466

answers:

8

Hi all,

I have some code that reads 10 registry keys, sometimes the values are not present sometimes the keys are not present, sometimes the value isn't boolean etc etc. How should I add error handling to this, currently it is placed in one big try{} catch{} but if the second value I read fails then the rest are not read as the program jumps to catch{}, I could add a try{} catch{} for each but I'm guessing there is a better way. How would you handle this? I'm asking as I regularly come across similar problems and my own solution is to add a try{} catch{}.

Thanks for the help.

A: 

Refactor the code that reads the values into its own function that handles the errors how you want them to be handled.

Ed Marty
+2  A: 

First, swallowing exceptions is generally a bad idea - could you not write a method that checks the keys etc for existance, and returns the value if one?

If that absolutely, positively isn't possible, you can refactor the code into multiple calls to a single method that (for each) does a try/catch (swallow):

SomeReturnType foo = HackyMethod(first path);
SomeReturnType bar = HackyMethod(sedond path);

SomeReturnType HackyMethod(string path)
{
    try {} catch {} etc
}
Marc Gravell
A: 

Essentially, yes, you want to define the error-handling on each individual element, not define the error handling on the set of elements. That is, if you want each error to be caught but not cause the process to abort, you should perform error handling on each individual element, not on the group as a whole.

McWafflestix
A: 

This depends on the severity of the errors. Is it meaningful and useful for the program to continue if some of the keys it looks up are missing or have the wrong type? Are some of the keys more important than others?

I would suggest the following:

  • Find all of the keys that you have to have, and put them in a single try{}, with the catch{} that reports the fatal error and initiates cleanup. Execute this block first.

  • Find all of the optional keys and put each of them in their own try{} block, which allows you to recover and go on to the other keys. To make this simpler, you can add a wrapper method that has the necessary try/catch block and error checking, and which takes the key name as a parameter.

JSBangs
A: 

EDIT: changed it all around. :P I suggested a struct or class (previously) but now i'm changing that to a simple string collection.

some pseduo code off the top of my head....

public IEnumerable<string> ReadRegistryKeys()
{
    IEnumerable<string> resultList = new List<string>();
    if (string.IsNullOrEmpty(read_in_key_#1())
    {

        resultList.Add("Failed to load key 'blah'..");
    }

    if (.... read in the next key .. etc.... ) ...

    return resultList == null || resultList.Count <= 0 ? null : resultList;
}

You can use a StringCollection (System.Collections.Specialized?) also, if u feel like it.

Pure.Krome
Kinda off-topic, but make it a class! Mutable structs? never a good idea. Plus it would be over-sized...
Marc Gravell
the problem with this is that you are limiting yourself to returning only errors. If you are going to go down this road, I'd suggest chaining them up in a string builder and if there is anything in the string builder then throw an exception
lomaxx
The reason why i didn't put them in a string builder is if in case u wish to display each error SPERATELY (eg. in a UL on some HTML page). I persoanlly do not like making exceptions if i don't need to. So having a list of expections would be a MASSIVE overkill in this case. Keep it simple, guys!
Pure.Krome
+1  A: 
Dictionary<String,String> regKeys = new Dictionary<String,String>()
{
    { "Key1", String.Empty},
    { "Key2", String.Empty},
    { "Key3", String.Empty}
};

for (int i = 0; i < regKeys.Length; i++)
{
   try
   {
       regKeys[i].Value = ReadFromRegistry(regKeys[i].Key);
   }
   catch (Exception ex)
   {
      Console.WriteLine("Unable to read Key: " + regKeys[i].Key 
         + " Exception: " + ex.Message);
   } 
}
FlySwat
+1  A: 

How are you reading the registry values? The Registry class (Microsoft.Win32.Registry) allows you to read a registry value and return a default value that you specify if the value/name pair does not exist, like this:

object o = Microsoft.Win32.Registry.GetValue(
    @"HKEY_CURRENT_USER\Software\Microsoft\Calc", "layout", "");

The last parameter here is the specified default to be returned if the value name is not found. I made it a blank string, but you can change this to whatever you like.

MusiGenesis
Registry keys == VERY EVIL imo. gag!
Pure.Krome
Hey, he's the one reading the Registry, not me. :)
MusiGenesis
damn - i missed that part of the OP question. Poor guy.
Pure.Krome
Did you think I was suggesting using the Registry class to handle errors? That's pretty funny.
MusiGenesis
A: 

@Marc's answer is the best, but if you absolutely must have a single excpetion that contains the collection of registry keys that had errors you should look at using the Data property of the exception. From the MSDN documentation on this property,

Use the System.Collections.IDictionary object returned by the Data property to store and retrieve supplementary information relevant to the exception. The information is in the form of an arbitrary number of user-defined key/value pairs. The key component of each key/value pair is typically an identifying string, whereas the value component of the pair can be any type of object.

Scott Dorman