Extract lines under the specified heading

Question

I wrote the following working code, which I just can't stomach:

var data = [
    { id: 0, isHeading: true, description: "A heading" },
    { id: 1, isHeading: false, description: "Blah 1" },
    { id: 2, isHeading: false, description: "Blah 2" },
    { id: 3, isHeading: true, description: "A heading" },
    { id: 4, isHeading: false, description: "Blah 3" },
    { id: 5, isHeading: false, description: "Blah 4" },
    { id: 7, isHeading: false, description: "Blah 5" },
    { id: 8, isHeading: false, description: "Blah 6" },
    { id: 9, isHeading: true, description: "A heading" },
    { id: 10, isHeading: false, description: "Blah 7" },
    { id: 11, isHeading: false, description: "Blah 8" },
    { id: 12, isHeading: false, description: "Blah 9" },
    { id: 13, isHeading: true, description: "A heading" },
    { id: 14, isHeading: false, description: "Blah 10" },
    { id: 15, isHeading: false, description: "Blah 11" },
    { id: 16, isHeading: true, description: "A heading" },
    { id: 17, isHeading: false, description: "Blah 12" },
    { id: 18, isHeading: false, description: "Blah 13" },
    { id: 19, isHeading: false, description: "Blah 14" }
  ]
  function getLines (headingId) {
    var r = []
    var inThere = false
    for (let line of data) {
      if (line.isHeading) {
        if(line.id === headingId) {
          inThere = true
          continue
        } else if (line.id !== headingId && inThere) {
          return r
        }
      }
      if (inThere) r.push(line)
    }
    return r
  }
  getLines(9)

Basically, it's taking a chunk of a list; it's a state automaton where the inThere state is set by a matching heading ID, and the routine is done when either there are no more items, or a new heading comes up.

Is there a much better way of writing this?

Answer 1

• Unless you know of the top of your head every instance that automatic semicolon insertion can fail (there are a few) you should use semicolons.
• Always use {} to delimit a statement block, even if it is one line. It is very easy to overlook the missing {} if you make changes and its not easy to spot the error when you look for the bug.
• The array r can be a constant
• The variable line in he loop can be a constant;
• r is a very poor name, maybe result would be better?
• inThere is a very poor name, maybe headingFound has more meaning?
• getLines has no meaning, maybe sectionByHeadingId ( I don't know what it represents so the section part is a guess)

• You are accessing a global scoped object data try to avoid such access by passing the array as a argument

• Avoid undue complication.

  • Don't execute statement if not needed. The statement if (inThere) can be } else if (inThere)
  • Don't repeat the same return if there is a shorter way. Replace the inner return with break,
  • Don't test a state you know. You test if(line.id === headingId) { then in the following else you check its state again else if (line.id !== headingId The first statement has already determined if the line is heading id you can only get to the else if line.id !== headingId is true

Example

Rewrites your code

function sectionByHeadingId(sections, headingId) {
    const result = [];
    var headingFound = false;
    for (const line of sections) {
        if (line.isHeading) {
            if (line.id === headingId) { 
                headingFound = true;
            } else if (headingFound) { 
                break;
            }
        } else if (headingFound) { 
            result.push(line);
        }
    }
    return result;
}
sectionByHeadingId(data, 9);

Example2

An alternative and simpler solution by using Array.findIndex to find the start of the section you want. Then you need only push lines till the next heading or end of array

function sectionByHeadingId(sections, headingId) {
    const result = [];
    var idx = sections.findIndex(line => line.id === headingId) + 1;
    while (sections[idx] && !sections[idx].isHeading) { 
        result.push(sections[idx++]);
    }
    return result;
}

Answer 2

Review

• Your function skips until the heading is found and than takes element until the next heading. Perhaps we could split these methods up to make them reusable.
• Try to avoid var (undesired scope), prefer let or const instead.
• Prefer a Tree data structure over flat list for hierarchical data.

Refactored for Reusability

Skip items while a predicate is true.

  function skipWhile(items, predicate) {
    let ok = false;
    return items.filter((value, index, array) => {
      if (!ok && !predicate(value)){
        ok = true;
      }
      return ok;
    });
  };

Take items while a predicate is true.

  function takeWhile(items, predicate) {
    let ok = true;
    return items.filter((value, index, array) => {
      if (ok && !predicate(value)){
        ok = false;
      }
      return ok;
    });
  };

The OP method:

 function getLines (headingId) {
    let lines = skipWhile(data, v => v.id != headingId);
    lines.shift(); // we don't want the heading
    lines = takeWhile(lines, v => !v.isHeading);
    return lines;
  }

And verification:

  console.log(getLines(9).map(x => x.description))

Yielding:

(3) [ "Blah 7" , "Blah 8" , "Blah 9" ]

You could also create these methods as prototype extensions on Array or refactor them to become generator functions (with yield syntax). I am sure these methods could get optimized for performance somehow.