Can this be considered Clean Code / Best Practice?

Question

Guys, How are you doing today?

I have the following question because I will follow this strategy for all my helpers (to deal with the DB entities)

Is this considered a good practice or is it going to be unmaintainable later?

   public class HelperArea : AbstractHelper
    {
        event OperationPerformed<Area> OnAreaInserting;
        event OperationPerformed<Area> OnAreaInserted;
        event OperationPerformed<Area> OnExceptionOccured;

     public void Insert(Area element)
        {
            try
            {
                if (OnAreaInserting != null)
                    OnAreaInserting(element);

                DBase.Context.Areas.InsertOnSubmit(new AtlasWFM_Mapping.Mapping.Area
                {
                    areaDescripcion = element.Description,
                    areaNegocioID = element.BusinessID,
                    areaGUID = Guid.NewGuid(),
                    areaEstado = element.Status,
                });

                DBase.Context.SubmitChanges();

                if (OnAreaInserted != null)
                    OnAreaInserted(element);
            }
            catch (Exception ex)
            {
                LogManager.ChangeStrategy(LogginStrategies.EVENT_VIEWER);
                LogManager.LogError(new LogInformation { logErrorType = ErrorType.CRITICAL, logException = ex, logMensaje = "Error inserting Area" });

                if (OnExceptionOccured != null)
                    OnExceptionOccured(elemento);
            }
    }

I want to know if it is a good way to handle the event on the Exception to let subscribers know that there has been an exception inserting that Area. And the way to log the Exception, is is OK to do it this way?

Any suggestion to make it better?

Answer 1

There aren't any comments; you should add some, in order to make sure other people can maintain it, and/or so you can maintain it when you forgot what it was doing.

Blocks like this:

if (OnAreaInserted != null)
    OnAreaInserted(element);

Should become blocks like this:

if (OnAreaInserted != null)
{
    OnAreaInserted(element);
}

Otherwise, this will eventually shoot you in the foot. Trust me.

As far as logging, it's much more typical to not set everything manually when you're logging an error. So, at the top of a file, you'd get a logger.

ILog log = LogManager.GetLogger("logname");

And then later, you'd have a much shorter call to the log :

log.error("Message here", exception);

Answer 2

One thing to add to Dean J's comment. I would just use the EventHandler delegate instead of a custom on. Much easier and gives a neat interface for all the events with sender and T where T should inherit from eventargs.

Answer 3

It depends. If this is where the exception is to be handled (and no higher) then sure. Otherwise, you shouldn't be squelching the exception.

FYI - you have a (possible) critical section issue in your event handling. You use this pattern:

if (OnAreaInserting != null)
    OnAreaInserting(element);

and it is possible for the event to change between the if and the call.

You can avoid this by declaring your events like this (IIRC):

public event EventHandler<AreaEventArgs> AreaInserting = (s, e) => { };

which makes sure that there is always at least on event handler. Then you can just invoke the event without checking for null.

Also you might consider following the proper .NET event pattern of making the event public, calling a protected virtual method name OnWhateverYourEventIsNamed and in that method invoking the event with this and a new event args. To do otherwise is to violate the principle of least astonishment, and that will make your maintenance harder.

Answer 4

This looks good to me the way it is. If LogManager is a static class instead of a property on AbstractHelper, you may consider something like Inversion of Control.

The code is short and clean... pretty simple to follow. (I like it.)

Edit... If I was to change the way your Exception handler works I would probably do something like this

public class ExceptionEventArgs<T> : EventArgs
{
    public ExceptionEventArgs(T input, Exception ex)
    {
        this.Input = input;
        this.Exception = ex;
    }
    public T Input { get; protected set; }
    public Exception Exception { get; protected set; }
}

public class HelperArea  //: AbstractHelper
{
    //...
    public event EventHandler<ExceptionEventArgs<Area>> OnExceptionOccured;

    public void Insert(Area element)
    {
        try
        {
            //...
        }
        catch (Exception ex)
        {
            //...
            if (OnExceptionOccured != null)
                OnExceptionOccured(this, new ExceptionEventArgs<Area>(element, 
                                                                      ex));
        }
    }
}

Answer 5

I would avoid swallowing the exception because you are not adding any value. Let upstream callers handle the exception in the way they know how. If you want to log it do so by:

catch(Exception ex)
{
     // TODO: Log code here
     throw; // rethrow while preserving stack trace 
}

Also check your input - how do you know you have a valid Area object to begin with? All input is evil.

Answer 6

Couple of things to note are

1. can the parameter 'Element' be null
2. Catching a generic exception is not a good thing. Always the specific exceptions should be caught followed by generic.
3. providing alias or using statement for namespace separated types AtlasWFM_Mapping.Mapping.Area