Poprawną obsługa wyjątków

Witam kolegów programistów, mam pewne pytanie natury techniczno-estetycznej.

Załóżmy że posiadam taki kod:

Config.Properties["script"] = textScript.Text;//script
Config.Properties["desired"] = textDesired.Text; //desired
Config.Properties["api"] = comboApi.Text;//api
Config.Properties["p"] = comboP.Text;//p
Config.Properties["p mode"] = comboMode.Text;//p mode
Config.Properties["aa"] = comboAA.Text;//aa
Config.Properties["procskip"] = comboProcskip.Text;//procskip
Config.Properties["disableoverlay"] = checkOverlay.Checked.ToString();
Config.Properties["forcefullscreen"] = checkFullscreen.Checked.ToString();
Config.Properties["safecapture"] = checkSafecap.Checked.ToString();
Config.Properties["steamfix"] = checkSteamFix.Checked.ToString();
Config.Properties["defaulttemp"] = checkDefaultTemp.Checked.ToString(); 

Klasa Config posiada jedno pole właściwości które jest słownikiem i służy po prostu do trzymania danych i zapisywania ich do pliku z ustawieniami w stylu zmienna=wartość. I teraz zakładamy że przy ładowaniu takiego pliku, chce pobrać wartość tej zmiennej odwołując się np. tak:

framesPath = Config.Properties["framesPath"];

Jako że istnieje szansa braku tej wartości w pliku z configiem to poleci wtedy wyjątek. I moje pytanie brzmi w jaki sposób najlepiej obsłużyć tą sytuacje?
Czy mam ifem sprawdzać każdą pojedyńczą wartość czy istnieje i dopiero ją odczytywać tzn:

if (Config.Properties.ContainsKey("framesPath"))
framesPath = Config.Properties["framesPath"];
if (Config.Properties.ContainsKey("Path"))
Path = Config.Properties["Path"];

if (Config.Properties.ContainsKey("TempPath"))
TempPath= Config.Properties["TempPath"]; 

itd.
czy też zrobic blok try catch na każdą jedną zmienna?

try
{
framesPath = Config.Properties["framesPath"];
}
catch(blabla)
try
{
        path = Config.Properties["path"];
}
catch(blabla)

Oba sposoby wydają mi się trochę nadmiarowe i pisać całe konstrukcje z ifami za każdym odczytaniem zmiennej jest imho trochę bez sensu.

a nie lepiej dodać metodę getPropertyWithDefaultValue(String name, String defaultValue); wtedy jak nie będzie danej wartości to zwrócisz wartość domyślną.
Ja zwykle coś takiego robię z configami.

No jest to jakieś rozwiązanie, w tym wypadku zadziała. Ale co w innych sytuacjach gdzie właśnie masz kilkanaście podobnych linijek gdzie z każdej może wylecieć wyjątek, a nie można dać wszystkiego w 1 blok try catch bo jak w połowie poleci wyjątek to reszta zostanie ominięta... co w takich sytuacjach robić?

Zacznijmy od tego: co zrobisz z wyjątkiem, jak zamierzasz go obsłużyć? W ogóle możesz go sensownie obsłużyć?

To może coś takiego:

public string GetPropertyValue(string key, string defaultValue = null)
{
   try
   {
      return Config.Properties.ContainsKey(key) 
                     ? Config.Properties[key] 
                     : defaultValue;
   }
   catch (Exception e)
   {
      //w tym miejscu zapis do logu//
      return defaultValue;
   }
}

dasdsasdasdsa napisał(a):

Zacznijmy od tego: co zrobisz z wyjątkiem, jak zamierzasz go obsłużyć? W ogóle możesz go sensownie obsłużyć?

Załóżmy że nie chce go jakoś sensownie obsługiwać, tylko zalogować błąd i kontynuować. Powiedzmy że mam kilka różnych (powiedzmy z 10) metod które wywołuje jedna po drugiej i które są niezależne od siebie i nie mają na siebie wpływu. Wsadzacie wtedy te wszystkie metody w 1 blok try catch (tyle że jak sie wywali po pierwszej metodzie to pozostałe 9 są ignorowane) czy też każdą funkcje opakowujecie w osobny try-catch? Chciałbym ogólnie wiedzieć jak sobie radzicie z problemem obsługi wielu wyjątków. Bo przecież w pewnym momencie kodu łapania i obsługi wyjątków będzie o wiele więcej niż samych zwykłych metod.

Co do propertisów to myślę, że Sarrus dał dobry przykład - czyli wrzucenie obsługi odczytu pojedyńczej wartości do osobnej metody i tam obsługa wyjątków i logowania.
Co do ostatniego Twojego pytania z 10-cioma niezależnymi metodami. To zależy jak ma się zachowywać program. Jeśli jest to np. start programu i podczas niego np. wczytujesz konfigurację programu, tworzysz połaczenie do bazy danych i otwierasz gniazda, to być może chcesz, żeby program zalogował błędy i zakończył działanie jeśli którakolwiek z tych czynności sie nie powiedzie. Wtedy można wrzucić wszystko do jednego try/catch.

Jeśli natomiast chcesz kontynuować działanie programu mimo wszelkich błędów, to możesz wywołanie każdej metody wpakować do osobnego try/catch lub wrzucić każde takie wywołanie do osobnej metody opakowującej - podobnie jak z propertisami - w takiej metodzie przechwytywać dany wyjątek, zalogować go (ewentualnie zwrócić jakis kod błędu) i kontynuować działanie.

Nie mam IDE i jest późno, ale możesz pokombinować w taki sposób...

static class Config
{
	public static PropertiesDictionary Properties = new PropertiesDictionary();
}

public class PropertiesDictionary
{
	private Dictionary<string, string> _values;
	private string DefaultValue = "";
	 
	public string this[string key]
	{
		get 
		{ 
			if ( ! _values.Contains( key ) )
			{
				LogError(key + " not in properties." );
				return this.DefaultValue; 
			}
			
			return _values[key];
		} 
	}
}

Tomek2 napisał(a):

Jeśli natomiast chcesz kontynuować działanie programu mimo wszelkich błędów, to możesz wywołanie każdej metody wpakować do osobnego try/catch lub wrzucić każde takie wywołanie do osobnej metody opakowującej - podobnie jak z propertisami - w takiej metodzie przechwytywać dany wyjątek, zalogować go (ewentualnie zwrócić jakis kod błędu) i kontynuować działanie.

Tego pierwszego chciałbym uniknąć żeby nie robić właśnie 10 bloków try-catch i pisać w każdym prawie to samo. Natomiast jak wrzuce te metody do innej metody opakowującej to fakt calość schowa się do 1 try-catcha tyle że dalej w środku w tej metodzie opakowującej jak wątek poleci np. przy 5 metodzie podrzędnej to reszta się nie wykona.

Widzę że chyba nie ma jednak innego sposobu na to :/

Chodziło mi o to, że w metodzie opakowującej przechwytujesz wyjątek i nie rzucasz go ponownie dalej. Więc w przypadku wyjątku przechwytujesz go, logujesz i wychodzisz z metody opakowującej i wywołujesz wyżej kolejną metodę opakowującą.

Czyli dla każdej metody należałoby stworzyć metodę opakowującą? To w zasadzie w niczym nie rozwiązuje problemu nadmiarowości kodu który przedstawiłem wcześniej.

I tak i nie :)

Nie unikniesz wielu try/catch, ale schowasz je "w kąt". Po prostu ładniej wygląda kod metod opakowujących typu

initConfig()
connectToDB()
startConnections()

i one mają w sobie cały "syf" z przechwytywaniem wyjątków i logowaniem niż wrzucenie tego wszystkiego do jednej metody w wieloma try/catchami.

Spójrz na poniższy kod z przykładem (projekt konsolowy).

using System;
using System.Collections.Generic;

namespace ExceptionHandling
{
    class Program
    {
        static void Main(string[] args)
        {
            Config.PropertiesWithException["a"] = "aaa";
            Config.PropertiesWithException["b"] = "bbb";
            Config.PropertiesWithoutException["c"] = "ccc"; 
            Config.PropertiesWithoutException["d"] = "ddd";

            Console.WriteLine("Without exception");
            Console.WriteLine(Config.PropertiesWithoutException["a"]);
            Console.WriteLine(Config.PropertiesWithoutException["b"]);
            Console.WriteLine(Config.PropertiesWithoutException["c"]);
            Console.WriteLine(Config.PropertiesWithoutException["d"]);
            Console.WriteLine(Config.PropertiesWithoutException["e"]);
            Console.WriteLine(Config.PropertiesWithoutException["f"]);

            Console.WriteLine();
            Console.WriteLine("With exception");
            try
            {
                Console.WriteLine(Config.PropertiesWithException["a"]);
                Console.WriteLine(Config.PropertiesWithException["b"]);
                Console.WriteLine(Config.PropertiesWithException["c"]);
                Console.WriteLine(Config.PropertiesWithException["d"]);
                Console.WriteLine(Config.PropertiesWithException["e"]);
                Console.WriteLine(Config.PropertiesWithException["f"]);
            }
            catch (Exception e)
            {
                Console.WriteLine(e.ToString());
            }

            Console.ReadLine();
        }
    }

    static class Log
    {
        static public void Error(string Message)
        {
            Console.WriteLine("Log.Error: " + Message);
        }
    }

    static class Config
    {
        static private Dictionary<object, string> _values;

        static public readonly PropertiesDictionaryWithHandledExceptions PropertiesWithoutException;
        static public Dictionary<object, string> PropertiesWithException { get { return _values; } }

        static Config()
        {
            _values = new Dictionary<object, string>();

            PropertiesWithoutException = new PropertiesDictionaryWithHandledExceptions(_values);
        }
    }

    public class PropertiesDictionaryWithHandledExceptions
    {
        private Dictionary<object, string> _values;
        public string DefaultValue = "N/A";

        public string this[object key]
        {
            set { _values[key] = value; }
            get 
            {
                if ( ! _values.ContainsKey( key ) )
                {
                    Log.Error(key + " key not found.");
                    return this.DefaultValue;
                }
                else
                    return _values[key];
            }
        }

        public PropertiesDictionaryWithHandledExceptions(Dictionary<object, string> values)
        {
            _values = values;
        }

    }
}

Tomek2 napisał(a):

I tak i nie :)

Nie unikniesz wielu try/catch, ale schowasz je "w kąt". Po prostu ładniej wygląda kod metod opakowujących typu

initConfig()
connectToDB()
startConnections()

i one mają w sobie cały "syf" z przechwytywaniem wyjątków i logowaniem niż wrzucenie tego wszystkiego do jednej metody w wieloma try/catchami.

Heh, no niestety chyba jednak nie. No ale dzięki wszystkim za wszelkie porady, posiedzę i spróbuje z tym ukrywaniem wyjątku w środku.

Pozdrawiam

Jeśli metody mają ten sam typ zwracany i listę parametrów, to wystarczy jedna metoda opakowująca, przyjmująca właściwą metodę jako swój parametr.

No to też jest jakaś alternatywa tyle że wydaje mi się że trochę zaciemnia kod, chyba jednak lepsze było te pierwsze rozwiązanie.

PS. Venthias to ja tyle że przypomniało mi się hasło do konta.

@marduk, w jakim sensie mój pomysł zaciemnia kod?

Bo pojawiłoby się mnóstwo konstrukcji typu:

float result = RunMethodWithoutException(DivideNumber(10,0))

i trochę to przysłania i komplikuje według mnie kod.

Ja bym taką metodę nazwał TryExecute. Od razu wiadomo do czego służy, do tego obsługa błędów, która zajmuje relatywnie mało kodu.

Odnośnie ukrywania wyjątków, wpadłem na dość szalony pomysł. Pewnie Ci się spodoba, bo nie ma za dużo pisania >:].
W każdym razie może komuś się przyda.

Disclaimer: to nie jest dobry sposób na zrobienie tego (jeśli ktoś nie zauważył ;P). Powiem więcej, użycie tego w tym przypadku w kodzie prawdopodobnie powinno być karane. Dobrym sposobem jest getPropertyWithDefaultValue podane na początku i wariacje podawane w kolejnych postach. Tym niemniej:

1. Tworzymy proxy:

class PokemonProxy : RealProxy {
    readonly object instance;

    private PokemonProxy(object instance)
        : base(instance.GetType()) {
        this.instance = instance;
    }

    public static T Create<T>(T instance) {
        return (T)new PokemonProxy(instance).GetTransparentProxy();
    }

    public override IMessage Invoke(IMessage msg) {
        var call = msg as IMethodCallMessage;
        var method = call.MethodBase as MethodInfo;

        try {
            var result = method.Invoke(instance, call.InArgs);
            return new ReturnMessage(result, null, 0, call.LogicalCallContext, call);
        } catch (TargetInvocationException e) {
            var exc = e.InnerException;
            Console.WriteLine("Pokemon {0} mówi: {1}!", exc.GetType(), exc.Message);
            return new ReturnMessage(null, null, 0, call.LogicalCallContext, call);
        }
    }
}

2. Tworzymy testowy obiekt:

class Test : MarshalByRefObject {
    public void Foo() {
        throw new InvalidOperationException("Nie wykonuj mnie!");
    }

    public void Bar() {
        throw new NotImplementedException("Tu nic nie ma!");
    }

    public string Baz() {
        return "...a ja działam";
    }
}

3. Robimy test:

class Program {
    static void Main(string[] args) {
        var test = new Test();
        var pokemon = PokemonProxy.Create(test);

        pokemon.Foo();
        pokemon.Bar();
        Console.WriteLine(pokemon.Baz());
    }
}

4. Test działa jak się spodziewamy:

Pokemon System.InvalidOperationException mówi: Nie wykonuj mnie!!
Pokemon System.NotImplementedException mówi: Tu nic nie ma!!
...a ja działam

5. Problem - proxy można zrobić tylko na klasach dziedziczących z System.MarshalByRefObject. Dictionary<T, U> nie dziedziczy z MarshalByRefObject. Trzeba zrobić wrapper na dowolny obiekt dziedziczący z marshalByRefObject który można zastosować na dowolnym typie.

6. Pierwsza próba rozwiązania:

class MarshalWrapper<T> : MarshalByRefObject {
    public T Instance { get; set; }

    public MarshalWrapper(T instance) {
        this.Instance = instance;
    }
}

class Program {
    static void Main(string[] args) {
        var config = new Dictionary<string, int>();
        var wrapConfig = new MarshalWrapper<Dictionary<string, int>>(config);
        var pokeConfig = PokemonProxy.Create(wrapConfig);
        int i = pokeConfig.Instance["asdf"];
    }
}

Nie zadziała - .Instance jest wykonywane w proxy, ale już indekser ["asdf"] w Main przez co wyjątek jak leciał tak leci

7. Poprawny wrapper:

class MarshalWrapper<T> : MarshalByRefObject {
    T instance;

    public MarshalWrapper(T instance) {
        this.instance = instance;
    }

    public U Call<U>(Func<T, U> delegated) {
        return delegated(instance);
    }

    public void Call(Action<T> delegated) {
        delegated(instance);
    }
}

8. Cieszymy się działającym kodem (hackiem?):

class Program {
    static void Main(string[] args) {
        var config = new Dictionary<string, string>();
        var wrapConfig = new MarshalWrapper<Dictionary<string, string>>(config);
        var pokeConfig = PokemonProxy.Create(wrapConfig);

        config["a"] = "asdf";
        config["b"] = "qwer";

        Console.WriteLine(pokeConfig.Call(c => c["a"]));
        Console.WriteLine(pokeConfig.Call(c => c["b"]));
        Console.WriteLine(pokeConfig.Call(c => c["c"]));
    }
}

asdf
qwer
Pokemon System.Collections.Generic.KeyNotFoundException mówi: Dany klucz nie był
obecny w słowniku.!

(obecnie przy wyjątku zwracany jest null. Dla typów wartościowych to oczywiście nie zadziała (konwersja null -> value type rzuca wyjątek z wiadomych powodów), ale poprawienie tego nie jest wielkim problemem (znamy typ w proxy, wystarczy wywoływać domyślny konstruktor).

Przerażające rozwiązanie, coś jak armata na komara imho :P Chyba już wolałbym pisać te try catch wszędzie. No ale sposób z GetPropertyWithoutException póki jest jest najlepszy i tego się będę trzymał.