nylki / lindenmayer

Feature complete classic L-System library (branching, context sensitive, parametric) & multi-purpose modern L-System/LSystem implementation that can take javascript functions as productions. It is not opinionated about how you do visualisations.
MIT License
183 stars 14 forks source link

Make LSystem an overrideable ES6 class #13

Closed multimeric closed 5 years ago

multimeric commented 7 years ago

I was attempting to use the LSystem class as follows

import LSystem from 'lindenmayer'

export class SimpleTreeSystem extends LSystem {
    //Override the parent constructor to use a specific system
    constructor() {
        super({
            axiom: ['X'],
            productions: {
                X: () => [
                    {symbol: 'F', value: 1},
                    '[',
                    {symbol: '+', value: 20},
                    'X',
                    ']',
                    {symbol: 'F', value: 1},
                    '[',
                    {symbol: '+', value: -20},
                    'X',
                    ']',
                    {symbol: '+', value: 20},
                    'X'
                ],
                F: ({part}) => [{symbol: 'F', value: part.value * 2}]
            }
        })
    }

    /**
     * Override the normal iterate method so that by default it iterates 7 times
     * @param {number} iterations The number of times to iterate the L-System
     */
    iterate(iterations){
        const n = 7
        return super.iterate(iterations || n)
    }
}

However since the LSystem function simply sets method properties on the object instead of its prototype, that means that my custom iterate implementation is overwritten by the LSystem implementation.

Could the LSystem function be rewritten to a class?

nylki commented 7 years ago

Hey again @TMiguelT, You are keeping me on my toes, I like that. ;) First of all, LSystem is no class in the ES6 sense, therefore what you try above can simply not work.

You can use JS' prototype-based inheritance to implement a custom iterate (or more easily define a new one with a new name):

function SimpleTreeSystem(params) {
  LSystem.call(this, params);
  this.superIterate = this.iterate;
  this.iterate = iterations => this.superIterate(iterations || 7);
}

SimpleTreeSystem.prototype = Object.create(LSystem.prototype);
//SimpleTreeSystem.prototype.constructor = SimpleTreeSystem;

let test = new SimpleTreeSystem({
  axiom: "F",
    productions: {
        "F": "FF"
    }
})

test.setAxiom("F");
console.log(test.iterate()); // outputs FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF

(also refer to: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Inheritance_and_the_prototype_chain)

nylki commented 7 years ago

I realize that ES6 class syntax looks much nicer and makes some things more easy and I actually considered using ES6 classes in the beginning. I decided against it because I didn't see the benefit over existing JS object inheritance which I began to really like and wanted to keep things simple. ES6 classes are after all only syntactical sugar on top of prototype-based inheritance and transpile back into it.

However, this is a topic I am really split about. I really like the better readability of the new class syntax and it would make some things easier, like in your example. But I am also aware of arguments made against the use of the class syntax in its current incarnation (not all arguments apply in our case of course).

Again, this became such a long reply. However, I feel that it can only help to make my initial motivations and reasonings clear to you and other users of this library.

I'd much rather focus on improving the actual L-System implementation right now. Having said that, if you think it does make sense to use ES6 class syntax here please fork the repo, make the proper changes and if you like you can and create a pull-request for me to review and possibly convince with enough arguments. :)

multimeric commented 7 years ago

Yep! I'm happy to write a PR for this (it might take me a bit to get around to it!).

I could use the method you've shown, but that would mean not treating my class as a class. Although I'm not sure SimpleTreeSystem.prototype = Object.create(LSystem.prototype); will have any effect, since I don't think the LSystem.prototype actually has anything in it.

It seems like you've gone for a half factory function (since you're not using prototypes), half ES5 class (you still need to call it using this)? But I can see some advantages of rewriting to an ES6 class. For starters it would make your code a lot simpler, but also it would allow us users to use polymorphism where we write a function that takes an LSystem class or instance, and it calls iterate on that instance, meaning that we can override iterate in the child class and everything will still work the same.

I'm incredibly unconvinced by the class hatred. No doubt classes are less flexible than factories or even manually using prototypes, but they are much easier to write, to read, to understand coming from an OO background, and they make polymorphic behaviour incredibly easy and concise. Yes I could rewrite my code to use a factory but the code would be much longer and much less clear.

nylki commented 6 years ago

I am closing this for now. Feel free to keep commenting here if you have something to contribute to the discussion.

multimeric commented 6 years ago

So I take it you don't want LSystem to be a class? I'm still happy to write a PR

nylki commented 6 years ago

@TMiguelT Hey there! I assumed you lost interest, thats why I closed this. I'll reopen then.

If you if have a clean, working implementation I am happy to evaluate it and possibly merge it, if it meets my expectations.

What I'd additionally like, is to have a proper use case, that make use of the class-syntax to warrant the change. This should be something more interesting than overwriting default values. They should be well documented with examples for new users to benefit from. Also, ideally there should be additional unit tests covering the new syntax.

multimeric commented 6 years ago

I mean, off the top of my head, the advantages of a class would be:

multimeric commented 5 years ago

Closed by #21.