Noting discrepancies between scheduled and actual attendance of sessions

Question

I ask as I'm mostly self taught, and don't have a team to be able to get experience/challenge or ask.

The Requirement

I have a register per week. (ie the kind you mark attendance on), with sub sessions (known here as services). Ie, Morning, afternoon, evening etc. It is indicated whether someone is scheduled and/or whether they attended. (Someone can attend a different one to which they were scheduled.)

Coming from a UI, which posts a whole register; Into a API, through a BLL, into a repo, then a SQL Server database. A Register has attendees. Attendees have Services. Register → Attendees → Services.

Existing/Problematic

The existing method has issues, update & insert statements buried within two loops, which I know smells, hence I'm trying to remedy, but I'm not sure of the best way to do it.

Currently it was designed to…

• Loop through the Attendees;
  • Then within that, loop through the Services.
  • Then within that:
    • Update the scheduled Service and
    • Insert if someone attended a different Service.

With upto 50 people per register week, and typically 5 possible Services per day, a couple of database hits per one, we quickly have over 500 hits on the database per register save. This is obviously hitting the database a lot!

First thoughts

I'm trying to get my classes to adhere more to the SRP. What I was considering, in the BLL project, was this. (Where these are 2 lists of Services, the BLL getting the current register state, and the one submitted to the API call.) Which exposed something like:

public interface IRegisterChangeComparer
{
    List<Service> GetAdded();
    List<Service> GetRemoved();
    List<Service> GetChangedServices();
}

public class RegisterChangeComparer : IRegisterChangeComparer
{
    private List<Service> SubmittedServices;
    private List<Service> ExistingServices;

    public RegisterChangeComparer(List<Service> submittedServices, List<Service> existingServices)
    {
        SubmittedServices = submittedServices;
        ExistingServices = existingServices;
    }

    public List<Service> GetAdded()
    {
        List<Service> newlyAdded = new List<Service>();
        foreach (Service service in SubmittedServices.Where(s => !s.IsScheduled))
        {
            // Compare on Date, Service Type
            if (!ExistingServices.Exists(e => e.ServiceDate == service.ServiceDate && e.TypeId == service.TypeId))
            {
                newlyAdded.Add(service);
            }
        }
        return newlyAdded;
    }

    public List<Service> GetRemoved()
    {
        List<Service> removedServices = new List<Service>();
        foreach (Service service in ExistingServices.Where(s => !s.IsScheduled))
        {
            // Compare on Date, Service Type
            if (SubmittedServices.Exists(e => e.ServiceDate == service.ServiceDate && e.TypeId == service.TypeId))
            {
                removedServices.Add(service);
            }
        }
        return removedServices;
    }

    public List<Service> GetChangedServices()
    {
        List<Service> changedServices = new List<Service>();
        foreach (Service service in SubmittedServices)
        {
            //Find matching record from database data
            Service matchedService = ExistingServices.FirstOrDefault(e => e.ServiceDate == service.ServiceDate && e.TypeId == service.TypeId);

            if (matchedService == null) continue;

            if (service.IsAttended != matchedService.IsAttended && service.IsScheduled) 
            {
                changedServices.Add(service);
            }
        }
        return changedServices;
    }
}

Then pass that to the repo accordingly to save, but with grouped/less hits on the database.

---

Here is the Service Class. It probably ought to be using the Id for find/comparison.

public class Service
    {
        public int Id { get; set; }
        public int ServiceUserId { get; set; }
        public int SupplierId { get; set; }
        public int TypeId { get; set; }
        public DateTime ServiceDate { get; set; }
        public decimal Units { get; set; }
        public int PONumber { get; set; }
        public decimal Rate { get; set; }
        public bool IsScheduled { get; set; }
        public bool IsAttended { get; set; }
        public DateTime CreatedDate { get; set; }
        public DateTime LastUpdatedeDate { get; set; }
    }

---

Initially I'm after pointers to whether I'm going about it the right way or whether there's a better way to pursue. Any particular patterns to choose. I just want to improve and write better code.

Answer 1

I agree with all things said by @TopinFrassi and want to add some additional notes.

If you have the same code repeated multiple times extract it to a method. Define method like this

private static Service FindService(IEnumerable<Service> services, Service service)
{
    return services.FirstOrDefault(s => s.ServiceDate == service.ServiceDate &&
                                        s.TypeId == service.TypeId;
}

It will simplify all your methods. Also if you will return IEnumerable<Service> instead of List<Service> they can be even more simpler. For example, GetAdded will be

public IEnumerable<Service> GetAdded()
{
    foreach (var service in SubmittedServices.Where(s => !s.IsScheduled))
    {
        if (FindService(ExistingServices, service) == null)
        {
            yield return service;
        }
    }
}

Or you can use LINQ:

public IEnumerable<Service> GetAdded()
{
    return from service in SubmittedServices
           where !service.IsScheduled &&
                 FindService(ExistingServices, service) == null)
           select service;
}

And another two methods:

public IEnumerable<Service> GetRemoved()
{
    return from service in ExistingServices
           where !service.IsScheduled &&
                 FindService(SubmittedServices, service) != null)
           select service;
}

public IEnumerable<Service> GetChangedServices()
{
    return from service in SubmittedServices
           let matchedService = FindService(ExistingServices, service)
           where matchedService != null &&
                 service.IsAttended != matchedService.IsAttended &&
                 service.IsScheduled
           select service;
}

Answer 2

First, I'd like to say that you're pretty well informed for a self-taught programmer. Still, there are some things to work on.

IRegisterChangeComparer

public interface IRegisterChangeComparer
{
    List<Service> GetAdded();
    List<Service> GetRemoved();
    List<Service> GetChangedServices();
}

You should never expose a List. In fact, in most cases you never want to expose a concrete type when it comes to data structures. Since you return a list, I assume the said list won't be modified by the caller. So you should return IEnumerable<Service>. This limits the ways a user of your class can screw up your code (as you can't add/remove/insert/etc. from an IEnumerable<> without casting it, then it's just asking for trouble).

public interface IRegisterChangeComparer
{
    IEnumerable<Service> GetAdded();
    IEnumerable<Service> GetRemoved();
    IEnumerable<Service> GetChangedServices();
}

RegisterChangeComparer fields

Field members are supposed to be camelCased, which means ExistingServices should be existingServices. Why? To easily differentiate them from the properties. You could make your fields readonly, this way they can't be modifier afer they've been initialized in your constructor. (You could still Add/Remove/etc.. from the list, but the reference cannot be changed). Now, if I look at your code, I see that these lists are never used as lists. I mean that you never use methods/properties that are specific to the List<>, only extension methods defined for IEnumerable<>. You know what that means? It means you should declare a IEnumerable<Service> instead of a List<Service> in your fields. If you think you might want to alter these lists later, you should declare a ICollection<Service.

private readonly IEnumerable<Service> existingServices;
private readonly IEnumerable<Service> submittedServices;

RegisterChangeComparer constructor

First, you should change your parameters to IEnumerable<> for reasons I explained earlier. Second, since your class is public, it means "anyone" can create an instance of RegisterChangeComparer. That means the constructor needs to be "dumb proofed". You should always check your entry parameters for null in public methods/constructor/etc.

public RegisterChangeComparer(IEnumerable<Service> submittedServices, IEnumerable<Service> existingServices)
{
    if(submittedServices == null) throw new ArgumentNullException(nameof(submittedServices);
    if(existingServices == null) throw new ArgumentNullException(nameof(existingServices);

    //We use "this" for the compiler (and you) to make a difference between the parameter and the field
    this.submittedServices = submittedServices;
    this.existingServices = existingServices;
}

RegisterChangeComparer methods

Now, let's get real. Exists isn't an efficient method. It's very slow. I don't have your Service class, which is sad because it'd be helpful at the moment. The question is, what defines the identity of a Service? If the identity if defined by TypeID and ServiceDate (meaning that if two services have the same ID and service date, they must be the same Service), you could override the Equals and GetHashcode methods of the Service class. Then, you could use an HashSet<> data structure inside your class (Instead of IEnumerable) to use the Contains method, which is far faster. Since this is all hypothetical, I don't want to go in too much details.

If this sounds unclear to you, I recommend reading on data structures. Each structure has pros and cons when it comes to its data. For example, the HashSet is very fast to see if it already contains an object.

This is all I can do with the information that you provided.