There Was an Old Lady - generate lyrics to a cumulation song

Question

This is a problem from exercism.io. View the repo here (contains full example of song).

Problem statement:

Generate the lyrics of the song 'I Know an Old Lady Who Swallowed a Fly'.

While you could copy/paste the lyrics, or read them from a file, this problem is much more interesting if you approach it algorithmically.

---

This problem is quite similar to my previous question of 99 bottles. I've adopted what I learned from that review with the goal of writing idiomatic javascript and clean code.

Being new to javascript, I'm wondering if anything sticks out that might be improved.

---

food-chain.js

const FoodChain = module.exports = {
  FOOD_CHAIN: {
    'fly': "",
    'spider': "It wriggled and jiggled and tickled inside her.\n",
    'bird': "How absurd to swallow a bird!\n",
    'cat': "Imagine that, to swallow a cat!\n",
    'dog': "What a hog, to swallow a dog!\n",
    'goat': "Just opened her throat and swallowed a goat!\n",
    'cow': "I don't know how she swallowed a cow!\n",
    'horse': "She's dead, of course!\n",
  },

  getKeyAtIndex(index) {
    return Object.keys(this.FOOD_CHAIN)[index];
  },

  getVerseAtKey(key) {
    return this.FOOD_CHAIN[key];
  },

  verse(number) {
    const food = Food.getFood(number - 1);

    return (
      `I know an old lady who swallowed a ${food.name}.\n` +
      `${food.verse}` +
      `${this.getRepeatedVerses(food.rank)}`
      );
  },

  getRepeatedVerses(rank) {
    return [...this.reciteRepeatedVerses(rank)].join('\n');
  },

  *reciteRepeatedVerses(rank) {
    let current = rank;

    while (current >= 0) {
      const food = Food.getFood(current--);
      if (food.name === 'horse') break;
      yield food.repeatedVerse;
    }
  },

  verses(...range) {
    this.validateArguments(...range);
    return [...this.reciteRangeOfVerses(...range)].join('\n') + '\n';
  },

  *reciteRangeOfVerses(starting, ending = 0) {
    let current = starting;

    while (current <= ending) {
      yield this.verse(current++);
    }
  },

  validateArguments(...args) {
    if (args.some((arg) => arg < 0)) {
      throw new RangeError('Invalid arguments: cannot be negative.');
    }
    if (args.length > 2) {
      throw new RangeError('Invalid arguments: max 2.');
    }
    if (args.length == 2 && args[0] > args[1]) {
      throw new RangeError('Invalid arguments: must be in increasing order.');
    }
  },
}

class Food {
  constructor(rank) {
    this.rank = rank;
  }

  static getFood(rank) {
    const name = FoodChain.getKeyAtIndex(rank);
    switch (name) {
      case 'fly':
        return new Fly(rank);
      case 'bird':
        return new Bird(rank);
      default:
        return new Food(rank);
    }
  }

  get name() {
    return FoodChain.getKeyAtIndex(this.rank);
  }

  get verse() {
    return FoodChain.getVerseAtKey(this.name);
  }

  get predecessor() {
    return FoodChain.getKeyAtIndex(this.rank - 1);
  }

  get repeatedVerse() {
    return `She swallowed the ${this.name} to catch the ${this.predecessor}.`;
  }
}

class Fly extends Food {
  constructor(rank) {
    super(rank);
  }

  get repeatedVerse() {
    return "I don't know why she swallowed the fly. Perhaps she'll die.\n";
  }
}

class Bird extends Food {
  constructor(rank) {
    super(rank);
  }

  get repeatedVerse() {
    return 'She swallowed the bird to catch the spider that wriggled and jiggled and tickled inside her.';
  }
}

food-chain.spec.js

const song = require('./food-chain');

describe('Food Chain', function () {

  it('fly', function () {
    const expected = 'I know an old lady who swallowed a fly.\nI don\'t know why she swallowed the fly. Perhaps she\'ll die.\n';

    expect(song.verse(1)).toEqual(expected);
  });

  it('spider', function () {
    const expected = 'I know an old lady who swallowed a spider.\nIt wriggled and jiggled and tickled inside her.\n' +
      'She swallowed the spider to catch the fly.\n' + 'I don\'t know why she swallowed the fly. Perhaps she\'ll die.\n';

    expect(song.verse(2)).toEqual(expected);
  });

  it('bird', function () {
    const expected = 'I know an old lady who swallowed a bird.\n' +
      'How absurd to swallow a bird!\n' +
      'She swallowed the bird to catch the spider that wriggled and jiggled and tickled inside her.\n' +
      'She swallowed the spider to catch the fly.\n' +
      'I don\'t know why she swallowed the fly. Perhaps she\'ll die.\n';

    expect(song.verse(3)).toEqual(expected);
  });

  it('cat', function () {
    const expected = 'I know an old lady who swallowed a cat.\n' +
      'Imagine that, to swallow a cat!\n' +
      'She swallowed the cat to catch the bird.\n' +
      'She swallowed the bird to catch the spider that wriggled and jiggled and tickled inside her.\n' +
      'She swallowed the spider to catch the fly.\n' +
      'I don\'t know why she swallowed the fly. ' +
      'Perhaps she\'ll die.\n';

    expect(song.verse(4)).toEqual(expected);
  });

  it('dog', function () {
    const expected = 'I know an old lady who swallowed a dog.\n' +
      'What a hog, to swallow a dog!\n' +
      'She swallowed the dog to catch the cat.\n' +
      'She swallowed the cat to catch the bird.\n' +
      'She swallowed the bird to catch the spider that wriggled and jiggled and tickled inside her.\n' +
      'She swallowed the spider to catch the fly.\n' +
      'I don\'t know why she swallowed the fly. ' +
      'Perhaps she\'ll die.\n';

    expect(song.verse(5)).toEqual(expected);
  });

  it('goat', function () {
    const expected = 'I know an old lady who swallowed a goat.\n' +
      'Just opened her throat and swallowed a goat!\n' +
      'She swallowed the goat to catch the dog.\n' +
      'She swallowed the dog to catch the cat.\n' +
      'She swallowed the cat to catch the bird.\n' +
      'She swallowed the bird to catch the spider that wriggled and jiggled and tickled inside her.\n' +
      'She swallowed the spider to catch the fly.\n' +
      'I don\'t know why she swallowed the fly. ' +
      'Perhaps she\'ll die.\n';

    expect(song.verse(6)).toEqual(expected);
  });

  it('cow', function () {
    const expected = 'I know an old lady who swallowed a cow.\n' +
      'I don\'t know how she swallowed a cow!\n' +
      'She swallowed the cow to catch the goat.\n' +
      'She swallowed the goat to catch the dog.\n' +
      'She swallowed the dog to catch the cat.\n' +
      'She swallowed the cat to catch the bird.\n' +
      'She swallowed the bird to catch the spider that wriggled and jiggled and tickled inside her.\n' +
      'She swallowed the spider to catch the fly.\n' +
      'I don\'t know why she swallowed the fly. ' +
      'Perhaps she\'ll die.\n';

    expect(song.verse(7)).toEqual(expected);
  });

  it('horse', function () {
    const expected = 'I know an old lady who swallowed a horse.\n' + 'She\'s dead, of course!\n';

    expect(song.verse(8)).toEqual(expected);
  });

  it('multiple verses', function () {
    let expected = '';

    expected += 'I know an old lady who swallowed a fly.\nI don\'t know why she swallowed the fly. Perhaps she\'ll die.\n\n';
    expected += 'I know an old lady who swallowed a spider.\nIt wriggled and jiggled and tickled inside her.\n' +
      'She swallowed the spider to catch the fly.\n' +
      'I don\'t know why she swallowed the fly. Perhaps she\'ll die.\n\n';

    expect(song.verses(1, 2)).toEqual(expected);
  });

  it('the whole song', function () {
    let expected = '';

    expected += 'I know an old lady who swallowed a fly.\nI don\'t know why she swallowed the fly. Perhaps she\'ll die.\n\n';
    expected += 'I know an old lady who swallowed a spider.\nIt wriggled and jiggled and tickled inside her.\n' +
    'She swallowed the spider to catch the fly.\n' +
    'I don\'t know why she swallowed the fly. Perhaps she\'ll die.\n\n';
    expected += 'I know an old lady who swallowed a bird.\n' +
    'How absurd to swallow a bird!\n' +
    'She swallowed the bird to catch the spider that wriggled and jiggled and tickled inside her.\n' +
    'She swallowed the spider to catch the fly.\n' +
    'I don\'t know why she swallowed the fly. Perhaps she\'ll die.\n\n';
     expected += 'I know an old lady who swallowed a cat.\n' +
    'Imagine that, to swallow a cat!\n' +
    'She swallowed the cat to catch the bird.\n' +
    'She swallowed the bird to catch the spider that wriggled and jiggled and tickled inside her.\n' +
    'She swallowed the spider to catch the fly.\n' +
    'I don\'t know why she swallowed the fly. ' +
    'Perhaps she\'ll die.\n\n';
    expected += 'I know an old lady who swallowed a dog.\n' +
    'What a hog, to swallow a dog!\n' +
    'She swallowed the dog to catch the cat.\n' +
    'She swallowed the cat to catch the bird.\n' +
    'She swallowed the bird to catch the spider that wriggled and jiggled and tickled inside her.\n' +
    'She swallowed the spider to catch the fly.\n' +
    'I don\'t know why she swallowed the fly. ' +
    'Perhaps she\'ll die.\n\n';
    expected += 'I know an old lady who swallowed a goat.\n' +
    'Just opened her throat and swallowed a goat!\n' +
    'She swallowed the goat to catch the dog.\n' +
    'She swallowed the dog to catch the cat.\n' +
    'She swallowed the cat to catch the bird.\n' +
    'She swallowed the bird to catch the spider that wriggled and jiggled and tickled inside her.\n' +
    'She swallowed the spider to catch the fly.\n' +
    'I don\'t know why she swallowed the fly. ' +
    'Perhaps she\'ll die.\n\n';
    expected += 'I know an old lady who swallowed a cow.\n' +
    'I don\'t know how she swallowed a cow!\n' +
    'She swallowed the cow to catch the goat.\n' +
    'She swallowed the goat to catch the dog.\n' +
    'She swallowed the dog to catch the cat.\n' +
    'She swallowed the cat to catch the bird.\n' +
    'She swallowed the bird to catch the spider that wriggled and jiggled and tickled inside her.\n' +
    'She swallowed the spider to catch the fly.\n' +
    'I don\'t know why she swallowed the fly. ' +
    'Perhaps she\'ll die.\n\n';
    expected += 'I know an old lady who swallowed a horse.\n' + 'She\'s dead, of course!\n\n';

    expect(song.verses(1, 8)).toEqual(expected);
  });

  it('throws when given too many arguments', function() {
    const expected = new RangeError('Invalid arguments: max 2.');
    expect(() => song.verses(0, 2, 3)).toThrow(expected);
  });

  it("throws when given 2 arguments that aren't in increasing order", function() {
    const expected = new RangeError('Invalid arguments: must be in increasing order.');
    expect(() => song.verses(8, 1)).toThrow(expected);
  });

  it("throws when given a negative argument", function() {
    const expected = new RangeError('Invalid arguments: cannot be negative.');
    expect(() => song.verses(-1)).toThrow(expected);
  });

  it("throws when given arguments containing a negative value", function() {
    const expected = new RangeError('Invalid arguments: cannot be negative.');
    expect(() => song.verses(-8, 1)).toThrow(expected);
  });
});

Answer 1

Things you did well

A big thank you for including the tests with the code and defining them so well. They cover almost all cases already and allowed me to refactor your code without any worries.

The function names and variables have very good names. At first reading, I didn't even notice that because good names don't make me think, only bad names do.

I also didn't notice that your code was formatted properly. Most beginners don't care about that, but I find it very important.

Defining a minimal API

Your module exports too much. The only functions that should be exported are verse and verses. Therefore, your module should be structured as follows:

function verses(...) { ... }

function verse(...) { ... }

// other functions

module.exports = {
    verse: verse,
    verses: verses
};

Property order of an object

There is no guarantee that the foods in FOOD_CHAIN are stored in the same order as you have listed them.

See https://stackoverflow.com/questions/5525795/does-javascript-guarantee-object-property-order.

To fix this, use an array to define the data:

const FOOD_CHAIN = [
    ['fly', ''],
    ['spider', 'It wriggled and jiggled and tickled inside her.\n'],
    ['bird', 'How absurd to swallow a bird!\n'],
    ['cat', 'Imagine that, to swallow a cat!\n'],
    ['dog', 'What a hog, to swallow a dog!\n'],
    ['goat', 'Just opened her throat and swallowed a goat!\n'],
    ['cow', 'I don\'t know how she swallowed a cow!\n'],
    ['horse', 'She\'s dead, of course!\n'],
];

Extra line breaks

In verses, you append an extra newline at the very end. I would omit that because it is not necessary.

Function arguments

The verses function syntactically accepts an arbitrary number of arguments. Actually, only a range of exactly two numbers is valid. This should be expressed in the function signature, which should be verses(starting, ending).

In reciteRangeOfVerses, the default argument of 0 doesn't make any sense.

When validating arguments, be as specific as possible, since that helps the caller of the code.

function verses(starting, ending) {
    if (!(1 <= starting && starting <= foods.length)) {
        throw new RangeError(`Invalid argument: starting must be in range 1..${foods.length}, was ${starting}.`);
    }
    if (!(1 <= ending && ending <= foods.length)) {
        throw new RangeError(`Invalid argument: ending must be in range 1..${foods.length}, was ${ending}.`);
    }
    if (starting > ending) {
        throw new RangeError(`Invalid arguments: must be in increasing order, was ${starting}..${ending}.`);
    }

    …
}

Class structure

Having three different classes for the food types is overkill in this case. A simpler alternative is:

class Food {
    constructor(name, verse, predecessor) {
        this.name = name;
        this.verse = verse;
        this.predecessor = predecessor;
    }

    repeatedVerse() {
        switch (this.name) {
            case 'fly':
                return "I don't know why she swallowed the fly. Perhaps she'll die.\n";
            case 'bird':
                return 'She swallowed the bird to catch the spider that wriggled and jiggled and tickled inside her.';
            default:
                return `She swallowed the ${this.name} to catch the ${this.predecessor.name}.`;
        }
    }
}

This class gets all the data it needs directly, and doesn't need to look it up in some helper data structure.

The complete refactored code

const foodData = [
    ['fly', ''],
    ['spider', 'It wriggled and jiggled and tickled inside her.\n'],
    ['bird', 'How absurd to swallow a bird!\n'],
    ['cat', 'Imagine that, to swallow a cat!\n'],
    ['dog', 'What a hog, to swallow a dog!\n'],
    ['goat', 'Just opened her throat and swallowed a goat!\n'],
    ['cow', 'I don\'t know how she swallowed a cow!\n'],
    ['horse', 'She\'s dead, of course!\n'],
];

const foods = [];   // initialized later

function init() {
    let predecessor = null;
    for (let foodDatum of foodData) {
        let food = new Food(foodDatum[0], foodDatum[1], predecessor);
        foods.push(food);
        predecessor = food;
    }
}

function verses(starting, ending) {
    if (!(1 <= starting && starting <= foods.length)) {
        throw new RangeError(`Invalid argument: starting must be in range 1..${foods.length}, was ${starting}.`);
    }
    if (!(1 <= ending && ending <= foods.length)) {
        throw new RangeError(`Invalid argument: ending must be in range 1..${foods.length}, was ${ending}.`);
    }
    if (starting > ending) {
        throw new RangeError(`Invalid arguments: must be in increasing order, was ${starting}..${ending}.`);
    }

    function* reciteRangeOfVerses() {
        for (let current = starting; current <= ending; current++) {
            yield verse(current);
        }
    }

    return [...reciteRangeOfVerses(starting, ending)].join('\n');
}

function verse(number) {
    const food = foods[number - 1];
    return (
        `I know an old lady who swallowed a ${food.name}.\n` +
        `${food.verse}` +
        `${getRepeatedVerses(food)}`
    );
}

function getRepeatedVerses(food) {
    function* reciteRepeatedVerses() {
        for (let current = food; current !== null; current = current.predecessor) {
            if (current.name === 'horse')
                break;
            yield current.repeatedVerse();
        }
    }

    return [...reciteRepeatedVerses()].join('\n');
}

class Food {
    constructor(name, verse, predecessor) {
        this.name = name;
        this.verse = verse;
        this.predecessor = predecessor;
    }

    repeatedVerse() {
        switch (this.name) {
            case 'fly':
                return "I don't know why she swallowed the fly. Perhaps she'll die.\n";
            case 'bird':
                return 'She swallowed the bird to catch the spider that wriggled and jiggled and tickled inside her.';
            default:
                return `She swallowed the ${this.name} to catch the ${this.predecessor.name}.`;
        }
    }
}

init();

module.exports = {
    verse: verse,
    verses: verses
};

Source code layout in tests

You wrote the string literals in the tests inconsistently. To properly reflect the actual text, all string literals should be broken at newlines, and only there.

Your current code:

it('the whole song', function () {
    let expected = '';

    expected += 'I know an old lady who swallowed a fly.\nI don\'t know why she swallowed the fly. Perhaps she\'ll die.\n\n';
    expected += 'I know an old lady who swallowed a spider.\nIt wriggled and jiggled and tickled inside her.\n' +

My suggestion:

it('the whole song', function () {
    const expected = '' +
        'I know an old lady who swallowed a fly.\n' +
        'I don\'t know why she swallowed the fly. Perhaps she\'ll die.\n' +
        '\n' +
        'I know an old lady who swallowed a spider.\n' +

The empty string literal in the first line makes sure that all interesting text lines are aligned properly.

Alternatively:

it('the whole song', function () {
    const expected = [
        'I know an old lady who swallowed a fly.',
        'I don\'t know why she swallowed the fly. Perhaps she\'ll die.',
        '',
        'I know an old lady who swallowed a spider.',
        '…',
        ''].join('\n');

Note the trailing empty string to avoid writing […].join('\n') + '\n', thereby repeating the \n unnecessarily.

Missing test cases

Currently there is no test for passing too large numbers to the verse and verses functions.

Answer 2

Feedback

Nice work using the classes and sub-classes, as well as generators and the keywords const and let where appropriate

Suggestions

Sub-class constructors identical to super constructor

The sub-class constructors (for Fly and Bird) appear to match the super constructor (for Food). Unless you plan to add extra functionality in the sub-class constructors, I would remove those overrides. This is for multiple reasons, including but not limited to:

• If the signature of the super constructor changes, then the sub-class constructors also need to be updated
• If there happened to be a typo or other mistake in those sub-class constructors, that would be an avoidable error.

Memoization

While it likely won't make much difference, the function getKeyAtIndex could be optimized by using memoization - this could be achieved by storing the keys in another variable and then having that function merely reference the array of keys instead of calling Object.keys() in each call. While this may not make much of a difference in this example, it could make a larger difference in a larger project. Take a look at this answer for more information.