6
\$\begingroup\$

So I have this code in my program, and there are multiple variations of it. Just looking at it, I feel like there must be a way to make it more efficient, but I can't think of anything. Just looking at it, does anyone have any ideas?

Essentially, what the code does is it determines which level of the text that it is currently parsing. If it it is in level 0, it adds that body of text to the aMainSection ArrayList.

If it it in level 1, it adds that body of text to aChildSection of aMainSection (An ArrayList within the aMainSection ArrayList). Then it sets the aMainSection text to 'parent text' within a new ArrayList.

etc.

Notes:

  • aMainSection & aChildSection are arrayLists of type Section (A custom class)
  • levelCount is just an array of type int, size 10 (abstract number)

Code

// Adds the passed in heading text to the appropriate section
    public static void addSectionText(String text)
    {   
        if (currentLevel == 0)
        {
            aMainSection.get(levelCount[0] - 1)
            .addSection(text);
        }
        else if (currentLevel == 1)
        {
            aMainSection.get(levelCount[0] - 1)
            .aChildSection.get(levelCount[1] - 1)
            .addSection(text);

            aMainSection.get(levelCount[0] - 1)
            .aChildSection.get(levelCount[1] - 1)
            .addParentSection(
                    aMainSection.get(levelCount[0] - 1)
                    .getSection());
        }
        else if (currentLevel == 2)
        {
            aMainSection.get(levelCount[0] - 1)

            .aChildSection.get(levelCount[1] - 1)
            .aChildSection.get(levelCount[2] - 1)
            .addSection(text);

            aMainSection.get(levelCount[0] - 1)
            .aChildSection.get(levelCount[1] - 1)
            .aChildSection.get(levelCount[2] - 1)
            .addParentSection(
                    aMainSection.get(levelCount[0] - 1)
                    .getSection());

            aMainSection.get(levelCount[0] - 1)
            .aChildSection.get(levelCount[1] - 1)
            .aChildSection.get(levelCount[2] - 1)
            .addParentSection(
                    aMainSection.get(levelCount[0] - 1)
                    .aChildSection.get(levelCount[1] - 1)
                    .getSection());
        }   
        else if (currentLevel == 3)
        {
            aMainSection.get(levelCount[0] - 1)
            .aChildSection.get(levelCount[1] - 1)
            .aChildSection.get(levelCount[2] - 1)
            .aChildSection.get(levelCount[3] - 1)
            .addSection(text);

            aMainSection.get(levelCount[0] - 1)
            .aChildSection.get(levelCount[1] - 1)
            .aChildSection.get(levelCount[2] - 1)
            .aChildSection.get(levelCount[3] - 1)
            .addParentSection(
                    aMainSection.get(levelCount[0] - 1)
                    .getSection());

            aMainSection.get(levelCount[0] - 1)
            .aChildSection.get(levelCount[1] - 1)
            .aChildSection.get(levelCount[2] - 1)
            .aChildSection.get(levelCount[3] - 1)
            .addParentSection(
                    aMainSection.get(levelCount[0] - 1)
                    .aChildSection.get(levelCount[1] - 1)
                    .getSection());

            aMainSection.get(levelCount[0] - 1)
            .aChildSection.get(levelCount[1] - 1)
            .aChildSection.get(levelCount[2] - 1)
            .aChildSection.get(levelCount[3] - 1)
            .addParentSection(
                    aMainSection.get(levelCount[0] - 1)
                    .aChildSection.get(levelCount[1] - 1)
                    .aChildSection.get(levelCount[2] - 1)
                    .getSection());
        }       
        else if (currentLevel >= 4)
        {
            aMainSection.get(levelCount[0] - 1)
            .aChildSection.get(levelCount[1] - 1)
            .aChildSection.get(levelCount[2] - 1)
            .aChildSection.get(levelCount[3] - 1)
            .aChildSection.get(levelCount[4] - 1)
            .addSection(text);

            aMainSection.get(levelCount[0] - 1)
            .aChildSection.get(levelCount[1] - 1)
            .aChildSection.get(levelCount[2] - 1)
            .aChildSection.get(levelCount[3] - 1)
            .aChildSection.get(levelCount[4] - 1)
            .addParentSection(
                    aMainSection.get(levelCount[0] - 1)
                    .getSection());

            aMainSection.get(levelCount[0] - 1)
            .aChildSection.get(levelCount[1] - 1)
            .aChildSection.get(levelCount[2] - 1)
            .aChildSection.get(levelCount[3] - 1)
            .aChildSection.get(levelCount[4] - 1)
            .addParentSection(
                    aMainSection.get(levelCount[0] - 1)
                    .aChildSection.get(levelCount[1] - 1)
                    .getSection());

            aMainSection.get(levelCount[0] - 1)
            .aChildSection.get(levelCount[1] - 1)
            .aChildSection.get(levelCount[2] - 1)
            .aChildSection.get(levelCount[3] - 1)
            .aChildSection.get(levelCount[4] - 1)
            .addParentSection(
                    aMainSection.get(levelCount[0] - 1)
                    .aChildSection.get(levelCount[1] - 1)
                    .aChildSection.get(levelCount[2] - 1)
                    .getSection());

            aMainSection.get(levelCount[0] - 1)
            .aChildSection.get(levelCount[1] - 1)
            .aChildSection.get(levelCount[2] - 1)
            .aChildSection.get(levelCount[3] - 1)
            .aChildSection.get(levelCount[4] - 1)
            .addParentSection(
                    aMainSection.get(levelCount[0] - 1)
                    .aChildSection.get(levelCount[1] - 1)
                    .aChildSection.get(levelCount[2] - 1)
                    .aChildSection.get(levelCount[3] - 1)
                    .getSection());
        }
    }

Any help or thoughts would be appreciated.

\$\endgroup\$
  • \$\begingroup\$ I do not have time right now but what you need is a recursive function to call to dig into its child and call itself o dig into that child until there are no children. If I have a 15 minutes later and it hasnt been answered Ill try to write it for you. \$\endgroup\$ – Chad Jul 21 '11 at 17:15
  • \$\begingroup\$ @Chad I was just wondering if you had time to post your recursive function? I would love to see a recursive implementation of this. \$\endgroup\$ – This 0ne Pr0grammer Jul 21 '11 at 19:12
4
\$\begingroup\$

That much repetition does have a smell to it. Can you simplify it by saving and reusing intermediate values similar to what I've shown below?

    else if (currentLevel == 3)
    {
        Section level0Section = aMainSection.get(levelCount[0] - 1);
        Section level1Section = level0Section.aChildSection.get(levelCount[1] - 1);
        Section level2Section = level1Section.aChildSection.get(levelCount[2] - 1);
        Section level3Section = level2Section.aChildSection.get(levelCount[3] - 1);
        level3Section.addSection(text);

        level3Section.addParentSection(level0Section.getSection());

        level3Section.addParentSection(level1Section.getSection());

        level3Section.addParentSection(level2Section.getSection());
   }        
\$\endgroup\$
  • \$\begingroup\$ Not just a smell, it stinks :) \$\endgroup\$ – Boris Jul 21 '11 at 18:20
  • \$\begingroup\$ @jimreed That definitely seems like a good way to go about it. Definitely cuts down on a good chunk of code. I just wonder if there is a more recursive alternative. Still, thank you very much for your help! \$\endgroup\$ – This 0ne Pr0grammer Jul 21 '11 at 19:11
  • \$\begingroup\$ level0Section, level1Section in itself is a code smell, and it smells like an avoided array (or List): level[0].section and so on. \$\endgroup\$ – user unknown Jul 22 '11 at 3:16
  • \$\begingroup\$ @user unknown, what would be the benefit of making that change? Just changing level0Section...level3Section to level[0]....level[2] seems kind of unnecessarily tedious, as they both result in the exact same output. \$\endgroup\$ – This 0ne Pr0grammer Jul 22 '11 at 13:07
  • \$\begingroup\$ There are repeated calls, which could be simplified to setSectionAFromLevelBChildC (int a, int b, int c) { level[a].section = level[b].section.aChildSection.get (levelCount (c) - 1); } and invocation {setSectionAFromLevelBChildC (1, 0, 1); }` and so on. Maybe I'm wrong, but a pattern can then more easily identified. The code is a bit long and missing too much information which I would have to mock up (are there side effects?), to test a suggestion for sure. \$\endgroup\$ – user unknown Jul 22 '11 at 13:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.