HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Reduce nested for loops and if statements using Iterator Blocks?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
blocksiteratorstatementsloopsnestedreduceforusingand

Problem

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 ToolArrangment(List 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.

Solution

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 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.

Code Snippets

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);
    }
}

Context

StackExchange Code Review Q#24303, answer score: 5

Revisions (0)

No revisions yet.