Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This library is a markup parser which is intended to be used in node.js and browser environment. I've decided to use Jasmine for tests.

The library's name is ivy-markup. URL: https://github.com/AlexAtNet/ivy-markup

It would be great to get comments on its usability and code structure.

Main engineering ideas:

  • Parser parses the rest of the stream by trying several possibilities (e.g. try to parse table, then line, then characters)
  • Parser creates a tree of nodes
  • Rendering is two-step: node rendering and HTML tag rendering

Significant code fragments:

parser.js

    // Character ranges:
    (function () {
        /**
         * Reads whitespace characters from the stream.
         */
        Parser.prototype.whitespace = function (state) {
            return state.read(this.whitespaceCharacter);
        };

        /**
         * Reads whitespace character from the stream.
         */
        Parser.prototype.whitespaceCharacter = function (state) {
            if (state.end() || !state.current(/[ \t]/)) {
                return null;
            }
            return state.fetch();
        };

        /**
         * Reads characters from the stream.
         *
         * Reads the current character and then calls the character method
         * until it returns null.
         *
         * Examples (| - state position):
         *  " |asd, qwe. [a|...] " -> {"asd, qwe. "}, " asd, qwe. |[a|...] "
         *  " |e@mail.com!" -> {"e"}, " e|@mail.com!"
         *  " e|@mail.com!" -> {"@mail.com"}, " e@mail.com|!"
         */
        Parser.prototype.characters = function (state) {
            return this.createCharacterNode(
                (this.character(state) || state.fetch() || '') +
                    (state.read(this.character) || '')
            );
        };

        /**
         * Reads character from the stream.
         *
         * Returns:
         *  1. current character when it is uppercase or lowercase letter
         *     from A-Z, digit, comma, point, or white space
         *  2. character after backslash when it is not from #1
         *  3. backslash when it is followed by the character from #1
         *  4. null when the state ends or not #1-#3.
         *
         * Examples (| - state position):
         *  a|sdf -> s, as|df
         *  a|\sdf -> \, a\|sdf
         *  a|\\sdf -> \, a\\|sdf
         *  a|\.sdf -> ., a\.|sdf
         *  asdf| -> null, asdf|
         *
         * Called from:
         *  characters
         */
        Parser.prototype.character = function (state) {
            if (state.end()) {
                return null;
            }
            if (state.current(/[a-z\d\.\, ]/i)) {
                return state.fetch();
            }
            if (state.current('\\')) {
                state.move();
                if (state.end() || state.current(/[a-z\d]/i)) {
                    return '\\';
                }
                return state.fetch();
            }
            return null;
        };
    }());

renderer.js

    Renderer.prototype.node = function (node, callback) {
        var tag = this.tag.bind(this);

        switch (node.type) {
        case 'plugin':
            this.plugins[node.plugin.name].render(node, callback);
            break;
        case 'heading':
            this.nodes(node.nodes, function (result) {
                callback(tag('h' + node.level, { id : node.id }, result));
            }.bind(this));
            break;
        case 'list':
            this.nodes(node.nodes, function (result) {
                callback(tag({
                    'unordered' : 'ul',
                    'ordered' : 'ol'
                }[node.list], result));
            });
            break;
        ...
        case 'table':
            this.nodes(node.nodes, function (result) {
                callback(tag('table', { 'class' : 'table' }, result));
            });
            break;
        case 'table-row':
            this.nodes(node.nodes, function (result) {
                callback(tag('tr', result));
            });
            break;
        ...

index.js

'use strict';

/*global module, window, require */

var noConflict, ivy;

ivy = function (markup, done) {
    (function (markup, done) {
        var parser = new this.Parser(),
            tags = new this.Tags(),
            renderer = new this.Renderer(tags);
        renderer.render(parser.parse(markup), done);
    }.call(ivy, markup, done));
};

if (('undefined' !== typeof module) && undefined !== module.exports) {
    ivy.Parser = require('./parser.js');
    ivy.Tags = require('./tags.js');
    ivy.Renderer = require('./tags.js');
    module.exports = ivy;
} else if ('undefined' !== typeof window) {
    if (undefined === window.ivy) {
        noConflict = (function (previous) {
            return function () {
                window.ivy = previous;
                return ivy;
            };
        }(window.ivy));
    } else {
        noConflict = (function (previous) {
            return function () {
                window.ivy = previous;
                return ivy;
            };
        }(window.ivy()));
    }
    ivy.noConflict = noConflict;
    ivy.Parser = window.ivy.Parser;
    ivy.Tags = window.ivy.Tags;
    ivy.Renderer = window.ivy.Renderer;
    window.ivy = ivy;
}
share|improve this question
    
My first comment would be: Don't make people include 4 separate script files to use your library. Why not provide a fifth script that just includes the other 4 scripts when it runs? –  aroth Feb 1 '13 at 3:37
    
Thanks! I've added the minified version of the library. –  AlexAtNet Feb 1 '13 at 4:12
    
Per the FAQ, your question needs to contain actual code for review here and not a link to the code. codereview.stackexchange.com/faq –  MECU Feb 1 '13 at 4:14
    
Sorry. I've added some significant code examples and engineering ideas. –  AlexAtNet Feb 1 '13 at 4:34
    
If your script is supposed to run in the browser and node per your description I'd suggest you use this rather than window in index.js –  megawac Jan 24 at 22:56

1 Answer 1

up vote 2 down vote accepted

I've stared at the code for a while;

  • I assuming you wrote whitespaceCharacter to mimic the separation between characters and character. However, since you only refer once to whitespaceCharacter I would put that function inside Parser.prototype.whitespace.

  • The massive case statement in renderer.js gives me the heebeegeebees, one missing break and you are on the hook for some interesting debugging. Personally, I would probably go for something like this ( because node.type is a string):

    var nodeTypeHandlers = {
    
      plugin : function(){
        this.plugins[node.plugin.name].render(node, callback);
      }, 
      heading: function(){
        this.nodes(node.nodes, function (result) {
          callback(tag('h' + node.level, { id : node.id }, result));
        }.bind(this));
      },
      table: function(){
        this.nodes(node.nodes, function (result) {
          callback(tag('table', { 'class' : 'table' }, result));
        });
      },
      table-row: function(){
        this.nodes(node.nodes, function (result) {
          callback(tag('tr', result));
        });
      },
      ...
    }
    

    and then simply execute nodeTypeHandlers[node.type].call(this);

  • Other than that, the code is well commented, readable and JSHint cannot find anything.
share|improve this answer
    
Thanks! Nice feedback. –  AlexAtNet Mar 7 at 21:33

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.