Log Visitor Count in Asp.net MVC

Question

I have a table to Log visitor Count Like :

public class VisitorLog:BaseEntity
{
    public int Id { get; set; }
    public DateTime VisitedOn { get; set; }
    public string LocationIP { get; set; }
    public string BrowserName { get; set; }

    public VisitorLog()
    {
        VisitedOn = DateTime.Now;
    }
}

and in Global.asax I have this :

protected void Application_BeginRequest(object sender,EventArgs e)
{
    if (new HttpRequestWrapper(Request).IsAjaxRequest()) return;

    foreach (var task in ApplicationObjectFactory.Container.GetAllInstances<IRunOnEachRequest>())
    {
        task.Execute();
    }
}

and Implementation in Service Layer is like below :

public bool Add(string locationId, string browserName)
{
    if (string.IsNullOrEmpty(locationId) || string.IsNullOrEmpty(browserName)) throw new ArgumentNullException("Visitor Log");
  return  Add(new AddVisitLogViewModel { BrowserName=browserName,LocationIP=locationId.ToLower()});
}

public bool Add(AddVisitLogViewModel visitorLogViewModel)
{

    if (visitorLogViewModel == null) throw new  ArgumentNullException("visitorLog");
    if (Any(row => row.LocationIP == visitorLogViewModel.LocationIP && DbFunctions.TruncateTime(row.VisitedOn) ==DbFunctions.TruncateTime(DateTime.Now))) return false;
    var model = Mapper.Map<VisitorLog>(visitorLogViewModel);
    AddItem(model);
    return true;
}

In Application_BeginRequest I have to check request is Ajax or Action Call and others , is there a better way to check this or Improve these code ?

Answer 1

First of all I'd make code in Application_BeginRequest little bit more self-explicative:

if (!IsAjaxRequest())
    ExecuteAllRunOnEachRequestHandlers();

You have to introduce two small utility functions but it will make the intent clear when reading Application_BeginRequest (which may grow over time) without going in the details.

private bool IsAjaxRequest()
    => new HttpRequestWrapper(Request).IsAjaxRequest();

private void ExecuteAllRunOnEachRequestHandlers()
{
    foreach (var task in GetAllHandlers<IRunOnEachRequest>())
        task.Execute();

    IEnumerable<T> GetAllHandlers<T>()
        => ApplicationObjectFactory.Container.GetAllInstances<T>();
}

It may happen that you need to ignore other types of requests, in this case simply change IsAjaxRequest() to IsPageVisit() (or whatever else expresses the concept you want to capture) and remove the negation.

In Add(string, string) you always throw ArgumentNullException but which parameter is wrong is an important information that is lost. Also to be String.Empty and to be null are different things:

if (locationId == null)
    throw new ArgumentNullException(nameof(locationId));

if (String.IsNullOrWhiteSpace(locationId))
    throw new ArgumentException("...", nameof(locationId));

if (browserName == null)
    throw new ArgumentNullException(nameof(browserName));

if (String.IsNullOrWhiteSpace(browserName))
    throw new ArgumentException("...", nameof(browserName));

Vertical space is free, do not pack your code in one line, it hurts readability.

return Add(new AddVisitLogViewModel 
{
     BrowserName = browserName,
     LocationIP = locationId.ToLower()
});

In Add(AddVisitLogViewModel) you may apply same principles:

public bool Add(AddVisitLogViewModel visitorLogViewModel)
{
    if (visitorLogViewModel == null)
        throw new  ArgumentNullException(nameof(visitorLog));

    if (HasVisistedToday(visitorLogViewModel)
        return false;

    AddItem(Mapper.Map<VisitorLog>(visitorLogViewModel));

    return true;
}

Where HasVisitedToday() is your actual code with a given name to quickly understand what it does (and probably to match an entry in your specifications).

Note the use of nameof() to avoid string constants.

Side note: if Add(AddVisitLogViewModel) isn't invoked outside your assembly (its container class is internal) then you might want to replace parameters validation with assertions:

Debug.Assert(visitorLogViewModel != null);

Answer 2

I'm not quite sure why the of "little lines of code is good" sticks so well ... The code presented here is basically incomprehensible because it doesn't have enough linebreaks:

public bool Add(string locationId, string browserName)
{
    if (string.IsNullOrEmpty(locationId) || string.IsNullOrEmpty(browserName))
    {
         throw new ArgumentNullException("Visitor Log");
    }
    return Add(new AddVisitLogViewModel {
         BrowserName = browserName,
         LocationIP = locationId.ToLower()
    });
}

This is already much more readable and directly shows new problems with this code. Instead of throwing an Exception with an unhelpful message, you should strive to give the most information possible in an Exception message. Consider:

if (string.IsNullOrEmpty(locationId))
    throw new ArgumentNullException(nameof(locationId));
if (string.IsNullOrEmpty(browserName))
    throw new ArgumentNullException(nameof(browserName));

Next thing that makes me cringe is the initialization of AddVisitLogViewModel. Why does this use property initialization? Firstly this is only acting as a simple DTO (which makes the ViewModel name misleading) and secondly the instantiator shouldn't need to know the property names, but only the constructor args. Consider instead:

return Add(new VisitDTO(browserName, locationId.ToLower()));

Applying the same ideas to the overloaded Add method results in:

public bool Add(VisitDTO visitDto) 
{
    if (visitDto == null) 
        throw new ArgumentNullException(nameof(visitDto));

    if (Any(row => row.LocationIP == visitDto.LocationIP
            && DbFunctions.TruncateTime(row.VisitedOn) == DbFunctions.TruncateTime(DateTime.Now)))
        return false

    var model = Mapper.Map<VisitorLog>(visitDto);
    AddItem(model);
    return true;
}

Now that we're at this stage, there's another problem that surfaces. This code checks uniqueness constraints on the application level. That's generally speaking a stupid idea, because of the transfer overhead from the additional query both seeing the roundtrip time and the data transfer time (which should be pretty huge, given that log-tables have a tendency to grow large).

Additionally checking the row time against DateTime.Now is bad, because that DateTime.Now is not captured for the duration of iteration. Instead you should compare against a value computed once.
Since you should check the constraints in the database in the first place, I won't provide code to do that though...