Is this structure good for methods?

Looking for some opinions on the structure of this method, namely the
way errors are handled and reported:

http://pastebin.com/711366

My view is this is quite a good method of trapping and moving through a
flow of data.

Anyone suggest a better way?

Steven



*** Sent via Developersdex http://www.developersdex.com ***

re: Is this structure good for methods?

Steven Blair <steven.blair@btinternet.com> wrote:
[color=blue]
> Looking for some opinions on the structure of this method, namely the
> way errors are handled and reported:
>
> http://pastebin.com/711366
>
> My view is this is quite a good method of trapping and moving through a
> flow of data.
>
> Anyone suggest a better way?[/color]

I strongly suggest you look up exceptions, exception handling, and see
how they work.

-- Barry

re: Is this structure good for methods?

I know about exceptions.
They are used frequently in my applications (when required)
But this cod example, in my opinion is not suitable for wrapping around
try, catch blocks.
None of the methods in the example will throw an exception.



*** Sent via Developersdex http://www.developersdex.com ***

re: Is this structure good for methods?

Steven Blair <steven.blair@btinternet.com> wrote:
[color=blue]
> I know about exceptions.
> They are used frequently in my applications (when required)
> But this cod example, in my opinion is not suitable for wrapping around
> try, catch blocks.
> None of the methods in the example will throw an exception.[/color]

You don't know that - more importantly, you shouldn't know that, by
principle of encapsulation. Somebody modifying the code a year later may
throw an exception, or may in some other way break this invariant.

I, personally, would write your code like this:

---8<---
public override string ProcessMessage()
{
try
{
ParseMessage();
GetProductDetails();
return "whatever output";
}
catch (Exception ex)
{
LogException(ex);
return "whatever output";
}
}
--->8---

Where ParseMessage() and GetProductDetails() throw exceptions if there
is an error - and I would even prefer not catching the exception at all,
and letting another method further up the call stack catch the
exception.

-- Barry

re: Is this structure good for methods?

the problem is, this method could end up having a number og other
methods calls in it.

I just dont think:

try
{
//Method1
//Method2
//Method3
//Method4
//Method5
//Method6
}
catch(Exception)
{
//Which method threw the exception?
}


I am not sure this is the best structure.



*** Sent via Developersdex http://www.developersdex.com ***

re: Is this structure good for methods?

Steven Blair <steven.blair@btinternet.com> wrote:
[color=blue]
> the problem is, this method could end up having a number og other
> methods calls in it.
>
> I just dont think:
>
> try
> {
> //Method1
> //Method2
> //Method3
> //Method4
> //Method5
> //Method6
> }
> catch(Exception)
> {
> //Which method threw the exception?[/color]

It is in the stack trace associated with the exception, so if you log
the exception you do in fact know which method it occurred in.
[color=blue]
> }
>
> I am not sure this is the best structure.[/color]

That's by design, of course: one of the goals of exception handling is
to centralize your error handling. If you need to know the source of the
exception, you could attribute the exception at the point it's thrown.
If that won't work out, you could consider adding another layer of
indirection:

---8<---
class App
{
delegate void Method();

class WrappedException : Exception
{
private object _source;

public WrappedException(Exception inner, object source)
: base(inner)
{
_source = source;
}

public object Source
{
get { return _source; }
}
}

static void Wrap(object source, Method method)
{
try
{
method();
}
catch (Exception ex)
{
throw new WrappedException(ex, source);
}
}

static void Main()
{
try
{
Wrap("source 0", delegate { Method0(); });

Wrap("source 1", delegate
{
Method1();
Method2();
});

Wrap("source 2", delegate
{
Method4();
Method5();
});
}
catch (WrappedException ex)
{
Log(ex.Source);
}
}
}
--->8---

That's totally off the top of my head, use at your own risk, etc.

-- Barry

re: Is this structure good for methods?

I'm going to ask a stupid question, because I've been wondering for a
while... if exceptions are to be thrown in exceptional circumstances,
is there any pattern for dealing with situations where you need to
return information on the status of a procedure?

An example might be a login screen where your UI needs to report that
a) login was successful, b) the password was incorrect, or c) the
username was not recogised.

Barring the security implications of this, is there any better way than
returning an enum? What if you need to return data from a method, but
you also want information on the state of that method? Are you back to
the OP's technique?

What if you don't want to throw an exception because, as in the example
above, certain classes of error are expected and aren't fatal to an
app?

Would a class to wrap the command make sense in this situation?

I humbly await enlightenment.

re: Is this structure good for methods?

Pathogenix@gmail.com wrote:
[color=blue]
> if exceptions are to be thrown in exceptional circumstances,[/color]

What counts as exceptional is of course up to debate, and different
languages down the years have had different penalties for exceptions, so
I don't think there's a universal consensus on this.
[color=blue]
> is there any pattern for dealing with situations where you need to
> return information on the status of a procedure?[/color]

A reasonable pattern can be seen in Int32.TryParse() and friends.
[color=blue]
> An example might be a login screen where your UI needs to report that
> a) login was successful, b) the password was incorrect, or c) the
> username was not recogised.[/color]

I think it's reasonable in this case to have a method returning boolean
here - in particular, because revealing the exact nature of the
authentication failure is a security weakness. A malicious attacker can
validate usernames without knowing passwords if the response is
different for unknown usernames versus incorrect passwords.
[color=blue]
> Barring the security implications of this, is there any better way than
> returning an enum?[/color]

It entirely depends on how much information you need. If you need lots
of information, it may warrant a rich object graph.
[color=blue]
> What if you don't want to throw an exception because, as in the example
> above, certain classes of error are expected and aren't fatal to an
> app?[/color]

If an error is expected, then you'll want to test for it imperatively -
but you'll still want your mainline case to throw exceptions if the
caller didn't call the corresponding "test" routine. One of the nicer
things about exceptions is that they're harder to ignore than return
values.
[color=blue]
> Would a class to wrap the command make sense in this situation?[/color]

It depends on the circumstances - whatever reduces the total effort of
producing a correct and maintainable implementation that satisfies the
requirements should suffice, I reckon :).
[color=blue]
> I humbly await enlightenment.[/color]

Methinks you protest too much!

-- Barry

re: Is this structure good for methods?

I suppose TryParse is a reasonable example where you have a state and a
possible return value, only they're swapped around. My question, I
suppose, is what do you do if you have more than two possible states
for a method?

An enum seems right, but I'm wondering if maybe I'm seeing a problem
that doesn't exist. Meditate on this, I shall.

I think the answer, basically, is bool/out for forgiving methods,
enum/out for forgiving methods with more than two final states; and for
the remainder, use exceptions, or split the method up so you don't have
the requirement to return two sets of information, or use a command
wrapping class with State and Value properties.

That's quite a few options to have open for the rare case when I want
to do this. I should stop worrying and learn to love the code.

I think I avoid exceptions too much, I have some deep unfounded fear of
errors in the system, especially errors that cause a magic goto :)

I also think I'm either not quite grokking something, or I'm not asking
the right question. I'll stop hijacking the thread, anyway.

-- flinky wisty pomm

re: Is this structure good for methods?

In fact, one last hijack, if people will indulge it, with a more
concrete example.

I have an ecommerce application, www.WidgetStore.com with a whole bunch
of stored procs to CRUD, each of which returns error codes to denote
success or reasons for failure.

J Random User tries to add a Widget to his shopping cart, but while he
was making coffee, some other user bought the last one. The proc for
updating carts and stock quantities fails because business logic is
violated (you can only add a product to your cart if it is in stock).

Now, it FEELS intuitively wrong to throw an exception to handle this
situation, and there're myriad examples I could give where business
logic is violated in an expected situation, and there is no failure of
code.

Is this mere superstition, or is another pattern more suited to this
kind of state reporting?

re: Is this structure good for methods?

Pathogenix@gmail.com wrote:
[color=blue]
> In fact, one last hijack, if people will indulge it, with a more
> concrete example.
>
> I have an ecommerce application, www.WidgetStore.com with a whole bunch
> of stored procs to CRUD, each of which returns error codes to denote
> success or reasons for failure.
>
> J Random User tries to add a Widget to his shopping cart, but while he
> was making coffee, some other user bought the last one. The proc for
> updating carts and stock quantities fails because business logic is
> violated (you can only add a product to your cart if it is in stock).[/color]

If you have transactional semantics for your business logic, (for
example, you can only add stuff to your basket if it is in stock, and
the two go together relatively atomically), you can take advantage of
exceptions and properly structured try/catch blocks to roll-back any
changes.

The try/catch blocks would look somewhat like:

---8<---
// do stuff A
try
{
// do stuff B
}
catch
{
// roll back stuff A
throw;
}
--->8---

Whether this would make your business code simpler, I don't know.

-- Barry