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?

• dfhwze: It would be nice if you could replace the Blah dummy data with some real data from your working environment, so we get an idea how you would use this function.
• dfhwze: @200_success you suggest a state machine solution to this problem?
• 200_success: @dfhwze No, I'm just tagging the question according to the author's own characterization of the code as a state automaton.

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

• Merc: Picked this one mainly for the fantastic rewrite at the bottom. Thanks for this!

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.