Optimizing location updates when adding a new object

Question

I'm trying to keep a bunch of Objects called Machine in their proper order. I was wondering if it'd be possible to further optimize the function HandleLocationCount for performance.

Any general feedback is of course also welcome. The function HandleLocationCount is only executed when a new Machine object is added.

The Machine object is below, it references a Section object, which acts as a container for the various machines.

public class Machine
{
 [Key]
 public int ID { get; set; }
 public string OrderNo { get; set; }
 public Section Section { get; set; }
 public int LocationInSection { get; set; }
 public ForcedLocation ForcedLocation { get; set; } = ForcedLocation.Blank;
}

Section object & enum for completeness; it's an enum instead of a boolean for future proofing:

public class Section
{
 [Key]
 public int ID { get; set; }
 public string Name { get; set; }
 public string OrderNo { get; set; }
 public IEnumerable<Machine> MachinesInSection { get; set; }
}
public enum ForcedLocation
{
 /// <summary>
 /// Does nothing for the location.
 /// </summary>
 Blank,
 /// <summary>
 /// This entity is always last in the location.
 /// </summary>
 Keep_Last,
}

The function that calls the HandleLocationCount function looks like this; a user is able to manually set the location if needed.

 public Machine Add(Machine entity)
 {
 if (entity.LocationInSection == 0)
 {
 IEnumerable<Machine> AmountMachines = GetMachinesByOrderNoAndSection(entity.OrderNo, entity.Section);
 entity.LocationInSection = handleLocationCount(AmountMachines);
 }
 _machineRepository.Add(entity);
 return entity;
 }

The function that returns the index just searches every machine for their order number & name, the repository itself is just an interface that has a generic index function

public IEnumerable<Machine> GetMachinesByOrderNoAndSection(string orderno, Section section)
 {
 return _machineRepository.Index()
 .Where(m => m.OrderNo == orderno && m.Section == section);
 }

And the code for the location looks like this, it updates older machines if this is needed; else just passes the new location for the machine

 private int HandleLocationCount(IEnumerable<Machine> AmountOfMachines)
 {
 int newLocation = AmountOfMachines.Count() + 1;
 foreach (var Machine in AmountOfMachines)
 {
 int oldLocation = Machine.LocationInSection;
 switch (Machine.ForcedLocation)
 {
 case ForcedLocation.Blank:
 break;
 case ForcedLocation.Keep_Last:
 Machine.LocationInSection = AmountOfMachines.Count() + 1;
 break;
 default:
 break;
 }
 if (Machine.LocationInSection != oldLocation)
 {
 newLocation = oldLocation;
 Update(Machine);
 }
 }
 return newLocation;
 }

• Heslacher: How many machine's can be there with ForcedLocation == ForcedLocation.Keep_Last ?
• RDAxRoadkill: There can be max one per section

Answer 1

The method GetMachinesByOrderNoAndSection returns an IEnumerable<Machine>. Depending on how the method is implemented, iterating the enumeration may query the database. In HandleLocationCount you are iterating it and also calling .Count() on it several times. The LINQ Count extension method might also iterate the enumeration.

Make sure to iterate it only once, e.g. by working on a list crated with var list = AmountOfMachines.ToList();, if something like this is not already done in GetMachinesByOrderNoAndSection.

Also, you could use two distinct variables for storing newLocation and AmountOfMachines.Count() + 1 and call the latter only once.

---

I am not sure to understand the logic right. But you seem to assign the same AmountOfMachines.Count() + 1 to all the Keep_Last machines. If this is the case, you could simply assign them a constant number like Int32.MaxValue. They would be kept last once and for all.

Another possible approach is to partition the number range into two ranges:

1. [1 .. Int32.MaxValue / 2] for Blank.
2. [Int32.MaxValue / 2 + 1 .. Int32.MaxValue] for Keep_Last.

Adavantages:

• The machines stay in their respective group.
• They are ordered.
• Adding a machine at the end of a group does not require the LocationInSection of others to be updated.

Instead of getting the count, you would get the maximium value in the two groups.

int nextBlank = machineList
 .Where(m => m.ForcedLocation == ForcedLocation.Blank)
 .DefaultIfEmpty(0)
 .Max() + 1;
int nextKeepLast = machineList
 .Where(m => m.ForcedLocation == ForcedLocation.Keep_Last)
 .DefaultIfEmpty(Int32.MaxValue / 2)
 .Max() + 1;

• RDAxRoadkill: I will add the GetMachinesByOrderNoAndSection function to my question, would it be better to cast it to a list right away when querying the database for this?
• RDAxRoadkill: Additionally, the Location number is used later in the program to get the next or previous machine if it's needed for calculations. I will try the MaxValue, but I do believe that'd cause issues
• Olivier Jacot-Descombes: The IQueryable and IEnumerable perform a lazy evaluation. This can prevent loading a lot of data into memory at once. Lazy evaluation gets the records being iterated just in time. But since you must get the count and perform a foreach, both iterating, you need a list. So, it depends on the number of machines. Just a few dozen? No problem with returning a list. 10s or 100s of thousands? Not ideal. If the number is really huge making 2 queries would be better. One query just getting the COUNT(*) returning only one record and another one returning all the machines in a IEnumerable<>.
• RDAxRoadkill: The expected amount of machines per order number is somewhere between 20-50, however for the entire solution I'm probably looking at a few thousand machines. Would it be recommended to split the queries? I thought I'd be better to just use one query for getting everything and only then splitting things.
• Olivier Jacot-Descombes: Since this methods returns always only one order and one section, have it return a List<Machine>. You can then get the number of machines through the list.Count property (note, not .Count()!). Splitting the query is then superfluous.