Divide List into groups

Question

I have a set of Service, and I want to partition them into ServiceClass, and have a reference from a Service to the Service class its belong. I am storing the reference in Dictionary<Service, ServiceClass>.

In the code below, I want to split into five service classes. I feel the code below is not well written, such as the naming of variable, but I am not sure how to improve it, can anyone help?

int group = 5;
List<Service> service = FileIO.ReadFeatureFile(@"D:\C Drive\Desktop\input.txt");
int numPerClass = service.Count / group;
Dictionary<Service, ServiceClass> ServiceToServiceClass = new Dictionary<Service, ServiceClass>();

for (int i = 0; i < group - 1;i++ )
{
    List<Service> serviceGrp = service.GetRange(0, numPerClass);
    ServiceClass sc=new ServiceClass(serviceGrp);
    foreach(Service service1 in serviceGrp)
    {
        ServiceToServiceClass.Add(service1, sc);
    }
    service.RemoveRange(0, numPerClass);
}

ServiceClass sc1 = new ServiceClass(service);
foreach (Service service1 in service)
{
    ServiceToServiceClass.Add(service1, sc1);
}

Answer 1

First off, as dreza said, work on your variable names. Secondly, you should really think about using more than one method for solving these types of problems.

All of my suggestions are assuming this is inside a class meant to hold this information.

Now to your code.

The line int group = 5; should be a constant. I would also rename it to something like numberOfServiceGroups. So the code line looks like:

const int numberOfServiceGroups = 5;

I would move the dictionary to a class variable and instantiate it in the constructor. I would also rename it to something like _serviceLookup. The _ signifying it is a class variable

Instead of using GetRange and RemoveRange, I would change it to use the linq statements Skip([int items]).Take([int items]) and because you are using a for loop, the Skip count can be calculated:

for (var i = 0; i < numberOfServiceGroups; i++)
{
    var servicesToProcess = service.Skip(i * servicesPerGroup).Take(servicesPerGroup);
    // More on this in a minute
}

The nice thing about Take() is that it will not blow-up if you go over bounds, it will just return what is left. Doing this will eliminate the need to have the catch loop at the end of your method.

I would move the processing of the services into the group to another method. This reduces nesting.

private void ProcessGroup(IList<Service> services)
{
    var serviceClass = new ServiceClass(services);
    foreach (var currentService in services)
    {
        _serviceLookup.Add(currentService, serviceClass);
    }
}

So your method now looks like:

private void Group()
{
    const int numberOfServiceGroups = 5;

    List<Service> services = FileIO.ReadFeatureFile(@"D:\C Drive\Desktop\input.txt");
    var servicesPerGroup = service.Count/numberOfServiceGroups;
    var serviceToServiceClass = new Dictionary<Service, ServiceClass>();

    for (var i = 0; i < numberOfServiceGroups; i++)
    {
        var servicesToProcess = services.Skip(i * servicesPerGroup).Take(servicesPerGroup);
        ProcessGroup(servicesToProcess.ToList());
    }
}

And your class:

public class ClassName
{
    public IDictionary<Service, ServiceClass> _serviceLookup;

    public ClassName()
    {
        _serviceLookup = new Dictionary<Service, ServiceClass>();
    } 

    private void Group()
    {
        const int numberOfServiceGroups = 5;

        List<Service> services = FileIO.ReadFeatureFile(@"D:\C Drive\Desktop\input.txt");
        var servicesPerGroup = service.Count/numberOfServiceGroups;

        for (var i = 0; i < numberOfServiceGroups; i++)
        {
            var servicesToProcess = services.Skip(i * servicesPerGroup).Take(servicesPerGroup);
            ProcessGroup(servicesToProcess.ToList());
        }
    }

    private void ProcessGroup(IList<Service> services)
    {
        var serviceClass = new ServiceClass(services);
        foreach (var currentService in services)
        {
            _serviceLookup.Add(currentService, serviceClass);
        }
    }
}

This should be a good start. I see a few more things I'd like to do, but they are beyond this scope.

Also note, I have not tested this due to missing classes / information.