Proper exception handling

[ghost]

I know that there was a previous issue regarding this which has since been closed, but no real solution was posted. I've been doing some tinkering and some thinking, as well as looking at the suggested solutions, and I think I have what I consider to be a good idea.

One of the suggestions was to wrap all requests in a single object containing the error code, error reason (from the API), as well as the requested object. This idea is a good one, but would require a massive overhaul to the current framework. It's a good idea, but not necessarily an elegant one.

The other idea, which I like the more I think about it, is to store the error in the WowExplorer class itself. Since the framework as it stands right now throws a custom exception containing an ErrorDetail object for just about everything, implementation would be as simple as adding an property of type ErrorDetail to the WowExplorer class, and wrapping all calls in the class in a try/catch. If the request bombs, we grab the ErrorDetail object from the exception, set it in the WowExplorer class, and put the onus on the developer to check this property whenever a call is made.

I think this is best because it requires very little rewriting of the framework as it stands (outside of wrapping all existing calls in a try/catch), and it requires nothing more of the developer consuming this framework than checking the error detail property after every call.

The idea is to fail gracefully and avoid YSOD's whenever possible. If anyone has any comments/suggestions/criticisms with this idea, please post them here. I'll give this issue a few days to generate some conversation before I begin implementing on my end.

Thanks for your time. I know it's a bit TL;DR, but I've always been a very thorough communicator, and like to make sure I address all points effectively.

[Dongorath]

Looks a lot like my first proposal in issue #13, I like it.

One further question is what will calls which fail return ? A null object ? An empty one ?
Developper wise, I'd prefer a null one (easier to test and more evident that the call failed).

Then, there is a third solution : having TryGet* functions (TryGetCharacter, TryGetGuild, etc.) following the same pattern as the Dictionary<Tkey, TVal>.TryGetValue, e.g. :
bool TryGetCharacter(Region region, string realm, string name, CharacterOptions characterOptions, out Character character)
If this function returns false (whatever the object returned in the out parameter), the developper can go check the ErrorDetail in WowExplorer.
I don't know if it's a good solution, but it's now on the table !

Further more, I think there are a few exceptions that should be raised il all cases, such as connectivity problems.

[ghost]

Yeah, I think it was your solution that I latched on to when I was mulling this over. And I do like the TryGet idea a lot, but I'm also not a fan of "out" parameters in methods, primarily because I've seen them get overused, so I tend to stay away from them.

A failed call would likely return null. As a developer, it's also easier to check and more evident of a failed call. But then, so is the TryGet solution as well. Both achieve the same result. I'll do some pondering on that one.

[ghost]

Well, I've had a few days to wrap my head around this particular situation, and I have decided to go with the following:

All "GetXXX" calls will be replaced with a "TryGetXXX" call using an "out" parameter of the requested type. What will happen is that if an exception is thrown, it will be set in a new property of the WowExplorer object which will contain the exception, response code, and a relevant error message. These error messages will be tailored to the type of response code (500, 404, 302, etc.).

It will be the developers responsibility to check if the "out" parameter is null before using the requested object. If the object is null, the WowExplorer object should have a populated ErrorDetail property. If the requested object is not null, it should be safe to assume that the call succeeded.

[ghost]

Closing this issue out as this was addressed in my recent pull request that was merged. Please update your forks from the master branch.