Reduce nested for loops and if statements using Iterator Blocks?

Question

Is it possible to reduce this long nested for loops and if conditions to increase its readability and to optimize it for future reference.

While writing code for my scheduling app, I ended up with a method as shown below. Really, I have a data structure like this. Here, I checks - Is there any stages (Inside LCycle) which using same Tool at same time and If its found so, another method LCycleTimeShift is called to make a rearrangment.

But I want to check whether the new arrangement is adaptable and the for loop counter is reset so that it will check the new arrangment again. I think this is not the better way to write the code for better readabilty. A little research on the topic found that Enumerators can help me in this. But I don't know how can I accomplish this with the following code.

public List<LCycle> ToolArrangment(List<LCycle> TimeLineInit)
{
 for (int i = 0; i < TimeLineInit.Count; i++)//Each LIfeCycles In TimeLine
 {
 for (int j = 0; j < TimeLineInit[i].Stage.Count; j++)//Each Stages inTimeLine
 {
 for (int k = 0; k < i; k++)//Each L esvd =ifeCycles Upto Current LifeCycle
 {
 for (int l = 0; l < TimeLineInit[k].Stage.Count; l++)//Each Stages of (LifeCycle upto current LifeCycle) 
 {
 for (int m = 0; m < TimeLineInit[i].Stage[j].ToolList.Count; m++)//each tools in stage of timelkine
 {
 for (int n = 0; n < TimeLineInit[k].Stage[l].ToolList.Count;n++ )// Each tools In that stage (for loop outer of outer)
 {
 if (TimeLineInit[i].Stage[j].ToolList[m].ToolName == TimeLineInit[k].Stage[l].ToolList[n].ToolName)//If both tools are same (satidfying above for loop conditions)
 {
 if (IsTimeOverLaps(TimeLineInit[i].Stage[j].StageSpan, TimeLineInit[k].Stage[l].StageSpan))
 {//tool using at same time.
 Stage ReplaceStage = TimeLineInit[i].Stage[j].DeepCopy();//Taking Copy of stage Span to make time shift
 Double TimeDifference=(ReplaceStage.StageSpan.ToTime-ReplaceStage.StageSpan.FromTime).TotalMinutes;//Calculating required time shift
 ReplaceStage.StageSpan.FromTime=TimeLineInit[k].Stage[l].StageSpan.ToTime;//FromTime changed accordingly
 ReplaceStage.StageSpan.ToTime=ReplaceStage.StageSpan.ToTime.AddMinutes(TimeDifference);//To Time Changed accordingly
 LCycleTimeShift(TimeLineInit[i], ReplaceStage);//passing refernce
 j = 0; k = 0; l = 0; m = 0; n = 0;//Counter Reset to validate the new arrangment
 }
 }
 }
 }
 }
 }
 }
 }
 return TimeLineInit;
}

• Daniel Imms: o.0 do you really need that many for loops?
• almaz: Does it matter which of 2 overlapping stages actually gets rescheduled, or you're allowed to change any of them?
• Subin Jacob: yes I need @Tyriar
• dreza: That's some serious for loop nesting
• Lars-Erik: This smells like feature envy (c2.com/cgi/wiki?FeatureEnvySmell) and it's breaking the Law of Demeter (c2.com/cgi/wiki?LawOfDemeter). Clean it up, move logic into respective classes. The inner logic most definately belongs in Stage or Tool, and some of the fors probably belong in LCycle. The code you pasted should read something like foreach(var cycle in TimeLineInit) cycle.MakeTimeShifts()

Answer 1

First, having this many nested loops indicates that your algorithm could have really bad time complexity. And with the way you're resetting the indexes, I'm not even sure your code is guaranteed to complete. There may be a more efficient algorithm to do what you want, but I'm not going to try to find that.

Second, the i, j, k naming convention for loop variables is okay if you have two or maybe three nested loops. But with six levels, it makes your code much less readable (and more error-prone). You should consider using locals with reasonable names instead of expressions like TimeLineInit[i].Stage[j].ToolList[m]. Or even better, foreach loops, which basically do the same with less code.

Third, I think the code you're using for resetting is wrong. For example, when i = 0 and you reset the loop variables, you end up running the innermost loop with i = 0 and k = 0, which should never happen (since the invariant is k < i).

Fourth, I think that a method that takes something as a parameter, modifies it and then returns it is a code smell. One exception would be a fluent interface, but that doesn't seem to be your case, since the method is not an extension method.

Fifth, you should use normal .Net naming conventions (e.g. camelCase for local variables, instead of PascalCase).

Sixth, you should do all your time calculations in TimeSpans, instead of doubles representing minutes, because it's simpler.

Seventh, I think you could make your code more readable using LINQ.

The whole code would look something like:

public void ToolArrangment(List<LCycle> timeLineInit)
{
 foreach (var lifeCycle1 in timeLineInit)
 {
 bool repeat;
 do
 {
 repeat = false;
 var stages = (from stage1 in lifeCycle1.Stage
 from lifeCycle2 in timeLineInit.TakeWhile(lc2 => lc2 != lifeCycle1)
 from stage2 in lifeCycle2.Stage
 from tool1 in stage1.ToolList
 from tool2 in stage2.ToolList
 where tool1.ToolName == tool2.ToolName
 where IsTimeOverLaps(stage1.StageSpan, stage2.StageSpan)
 select new { stage1, stage2 })
 .FirstOrDefault();
 if (stages != null)
 {
 var replaceStage = stages.stage1.DeepCopy();
 var timeDifference = replaceStage.StageSpan.ToTime - replaceStage.StageSpan.FromTime;
 replaceStage.StageSpan.FromTime = stages.stage2.StageSpan.ToTime;
 replaceStage.StageSpan.ToTime = replaceStage.StageSpan.ToTime + timeDifference;
 LCycleTimeShift(lifeCycle1, replaceStage);
 repeat = true;
 }
 } while (repeat);
 }
}

---

I gave it some thought and I think a much more efficient algorithm to do the same is not that difficult. You would have a hash table indexed by ToolName and for each tool, you would keep a list of all known stages that use it. You would walk though all tools in all stages in all timelines and add them to the list of stages for that tool. If you couldn't do that because of an overlap, you would move the stage, just like you do right now (if I understand your code correctly).

I haven't though out all the details, but I think something like this should work and be much faster than your solution.

• Subin Jacob: What the meaning of .FirstOrDefault();
• Subin Jacob: I didn't understand the fourth statement. Were you speaking about LCycleTimeShift?
• svick: @SubinJacob You always want to take the first pair of stages that fulfills the conditions, modify the structure accordingly and then start from the beginning. FirstOrDefault() is there to get that first pair. If there is no matching pair, it will return null, which lets it exist the do-while cycle.
• svick: @SubinJacob No, I was speaking about ToolArrangment(). It takes the whole list as a parameter, modifies some objects inside it and then returns it. That's a pattern that doesn't make much sense, I think.
• Subin Jacob: But why that pattern doesn't make sense? passing a large data structure is not recommended? why? (I think, you were asking to send only the object that needed for rearrangement)

Answer 2

This does not reduce nesting per se, but it reduces clutter for all loop variables that are not really used:

public List<LCycle> ToolArrangment(List<LCycle> TimeLineInit)
{
 for (int i = 0; i < TimeLineInit.Count; i++) //Each LIfeCycles In TimeLine
 {
 stages:
 for (int k = 0; k < i; k++) //Each LifeCycles Upto Current LifeCycle
 foreach (var stageA in TimeLineInit[i].Stage)
 foreach (var stageB in TimeLineInit[k].Stage)
 foreach (var toolA in stageA.ToolList)
 foreach (var toolB in stageB.ToolList)
 {
 if ( //If both tools are same (satidfying above for loop conditions)
 toolA.ToolName == toolB.ToolName
 //tool using at same time.
 && IsTimeOverLaps(stageA.StageSpan, stageB.StageSpan))
 {
 Stage ReplaceStage = stageA.DeepCopy(); //Taking Copy of stage Span to make time shift
 Double TimeDifference = (ReplaceStage.StageSpan.ToTime - ReplaceStage.StageSpan.FromTime).TotalMinutes;
 //Calculating required time shift
 ReplaceStage.StageSpan.FromTime = stageB.StageSpan.ToTime; //FromTime changed accordingly
 ReplaceStage.StageSpan.ToTime = ReplaceStage.StageSpan.ToTime.AddMinutes(TimeDifference); //To Time Changed accordingly
 LCycleTimeShift(TimeLineInit[i], ReplaceStage); //passing refernce
 goto stages;
 }
 }
 }
 return TimeLineInit;
}

• TeaDrivenDev: xkcd.com/292 ;-)
• zzandy: @GCATNM: Yeah, but that's a rare specimen of a good goto. Seriously it's the reason it's left in the language.
• Subin Jacob: foreach is recommended for better readabilty! rite?
• zzandy: @SubinJacob: Yes, IMHO foreach yields code that's a lot more readable. Consider item vs. MyWordlyNamedCollectionsCollection[k].Items[i].