Ad
  • Default User Avatar

    Great kata as usual, nklein :)

    Only thing I would add is to state explicitly in the details is that defmethod should replace any existing method with that signature rather than being undefined behaviour. I don't recall if removeMethod was used in a method chain in one of the tests, but if it did I would also state that that needs to be chainable as well.

    Also, could the method names be defmethod and undefmethod or addMethod and removeMethod? Currently they feel a bit inconsitent.

  • Default User Avatar

    That's fair enough, I guess I was getting caught up in the "shares as many nodes as possible" in the append spec and completely ignored the fact that it still needs to be a linked list. :)

  • Default User Avatar

    I'm failing the l4.remove(first) is l4.tail() test because I'm pairing lists whenever I append them to prevent unnecessary flattening:

    var empty = new EmptyList();
    var a = empty.push('a');
    var ba = a.push('b');
    var ca = a.push('c');
    var caba = ca.append(ba); // creates new pair (ca, ba)
    var aba1 = caba.remove('c'); // creates new pair  (a, ba)
    var aba2 = caba.tail(); // creates new pair  (a, ba)
    console.log(aba1 === aba2); // false because of different pairs even though both reference the same nodes
    

    In practice, I'm duplicating less data than if I were to flatten the list (I'd need to clone ca). Is this something that could be taken into account either in the tests or description by specifying that you must create a flat list when appending? I can cache or flatten the pairs to get around the error, but this seems like a thing others might bump up against as well.