How to implement auditing in the business layer

I'm trying to implement basic auditing for a system where users can login, change their passwords and emails etc.

The functions I want to audit are all in the business layer and I would like to create an Audit object that stores the datetime the function was called including the result.

I recently attended a conference and one of the sessions was on well-crafted web applications and I am trying to implement some of the ideas. Basically I am using an Enum to return the result of the function and use a switch statement to update the UI in that layer. The functions use an early return which doesn't leave any time for creating, setting and saving the audit.

My question is what approaches do others take when auditing business functions and what approach would you take if you had a function like mine (if you say ditch it I'll listen but i'll be grumpy).

The code looks a little like this:

function Login(string username, string password)
{
 User user = repo.getUser(username, password);

 if (user开发者_开发技巧.failLogic1) { return failLogic1Enum; }
 if (user.failLogic2) { return failLogic2Enum; }
 if (user.failLogic3) { return failLogic3Enum; }
 if (user.failLogic4) { return failLogic4Enum; }

 user.AddAudit(new (Audit(AuditTypeEnum LoginSuccess));
 user.Save();

 return successEnum;
}

I could expand the if statements to create a new audit in each one but then the function starts to get messy. I could do the auditing in the UI layer in the switch statement but that seems wrong.

Is it really bad to stick it all in try catch with a finally and use the finally to create the Audit object and set it's information in there thus solving the early return problem? My impression is that a finally is for cleaning up not auditing.

My name is David, and I'm just trying to be a better code. Thanks.

I can't say I have used it, but this seems like a candidate for Aspect Oriented Programming. Basically, you can inject code in each method call for stuff like logging/auditing/etc in an automated fashion.

Separately, making a try/catch/finally block isn't ideal, but I would run a cost/benefit to see if it is worth it. If you can reasonably refactor the code cheaply so that you don't have to use it, do that. If the cost is exorbitant, I would make the try/finally. I think a lot of people get caught up in the "best solution", but time/money are always constraints, so do what "makes sense".

The issue with an enum is it isn't really extensible. If you add new components later, your Audit framework won't be able to handle the new events.

In our latest system using EF we created a basic POCO for our audit event in the entity namespace:

public class AuditEvent : EntityBase
{
    public string Event { get; set; }
    public virtual AppUser AppUser { get; set; }
    public virtual AppUser AdminUser { get; set; }
    public string Message{get;set;}
    private DateTime _timestamp;

    public DateTime Timestamp
    {
        get { return _timestamp == DateTime.MinValue ? DateTime.UtcNow : _timestamp; }
        set { _timestamp = value; }
    }

    public virtual Company Company { get; set; }
// etc.
    }

In our Task layer, we implemented an abstract base AuditEventTask:

internal abstract class AuditEventTask<TEntity>
{
    internal readonly AuditEvent AuditEvent;

    internal AuditEventTask()
    {
        AuditEvent = InitializeAuditEvent();
    }

    internal void Add(UnitOfWork unitOfWork)
    {
        if (unitOfWork == null)
        {
            throw new ArgumentNullException(Resources.UnitOfWorkRequired_Message);
        }
        new AuditEventRepository(unitOfWork).Add(AuditEvent);
    }

    private AuditEvent InitializeAuditEvent()
    {
        return new AuditEvent {Event = SetEvent(), Timestamp = DateTime.UtcNow};
    }

    internal abstract void Log(UnitOfWork unitOfWork, TEntity entity, string appUserName, string adminUserName);

    protected abstract string SetEvent();
}

Log must be implemented to record the data associated with the event, and SetEvent is implemented to force the derived task to set it's event's type implicitly:

internal class EmailAuditEventTask : AuditEventTask<Email>
{
    internal override void Log(UnitOfWork unitOfWork, Email email, string appUserName, string adminUserName)
    {
        AppUser appUser = new AppUserRepository(unitOfWork).Find(au => au.Email.Equals(appUserName, StringComparison.OrdinalIgnoreCase));
        AuditEvent.AppUser = appUser;
        AuditEvent.Company = appUser.Company;
        AuditEvent.Message = email.EmailType;
        Add(unitOfWork);
    }

    protected override string SetEvent()
    {
        return AuditEvent.SendEmail;
    }
}

The hiccup here is the internal base task - the base task COULD be public so that later additions to the Task namespace could use it - but overall I think that gives you the idea.

When it comes to implementation, our other tasks determine when logging should occur, so in your case:

AuditEventTask task;
if (user.failLogic1) { task = new FailLogin1AuditEventTask(fail 1 params); }
if (user.failLogic2) { task = new FailLogin2AuditEventTask(fail 2 params); }
if (user.failLogic3) { task = new FailLogin3AuditEventTask(etc); }
if (user.failLogic4) { task = new FailLogin4AuditEventTask(etc); }

task.Log();
user.Save();