5
\$\begingroup\$

What this command line utility does is to take a text file and update the content a bit before turning it into Markdown. For example, the first for loop searches all the straight quotes and replace them for curly quotes. The second loop searches for hr tags and replace them for * * *, then adds the noind class to the p that follows that hr tag.

#!/usr/bin/env node

var fs = require('fs')
var program = require('commander')
var markdown = require('markdown').markdown

program
  .version('0.0.1')
  .usage('<keywords>')
  .parse(process.argv)

if (!program.args.length)
  program.help()

var filename = program.args[0]

fs.readFile(filename, 'utf8', function(err, data) {
  if (err) throw err

  var tree = markdown.parse(data)

  ;(function updateTree(jsonml) {

    // replace straight quotes for curly quotes 
    for (i = 1; i < jsonml.length; i++) {
      if (typeof jsonml[i][1] === "string" && jsonml[i][1].match(/"/g)) {
        var p = jsonml[i]

        for (j = 1; j < p.length; j++) {
          if (typeof p[j] === "string" && p[j].match(/"/g)) {
            p[j] = p[j].replace(/"(?=\b)/g, "“")
                       .replace(/"(?!\b)/g, "”")
          }
        }
      }

      if (typeof jsonml[i][1] === "string" && jsonml[i][1].match(/'/g)) {
        var p = jsonml[i]

        for (j = 1; j < p.length; j++) {
          if (typeof p[j] === "string" && p[j].match(/'/g)) {
            p[j] = p[j].replace(/'(?=\b)/g, "‘")
                       .replace(/'(?!\b)/g, "’")
          }
        }
      }
    }

    // replace hr for * * * and add noind class to the p tag that follows it
    for (i = 1; i < jsonml.length; i++) {
      if (jsonml[i][0] === 'hr') {
        var hr = jsonml[i]
        var p = jsonml[i + 1]

        hr.splice(0, 1, 'para', '* * *')
        p.splice(1, 0, {'class': 'noind'})
      }       
    }
  })(tree)

  var html = markdown.renderJsonML(markdown.toHTMLTree(tree))
  console.log(html)
})

So my question is, how can I reduce the indentation level?

\$\endgroup\$
  • \$\begingroup\$ I would suggest to edit your title to what your code does, rather than what aspect of it you would like reviewed. The text of your question should be used to point out specific aspects. \$\endgroup\$ – Phrancis Mar 4 '15 at 16:55
  • \$\begingroup\$ @Phrancis How about now? \$\endgroup\$ – janoChen Mar 4 '15 at 16:57
  • 1
    \$\begingroup\$ Much better, thank you. Hope you get some good reviews! \$\endgroup\$ – Phrancis Mar 4 '15 at 16:59
2
\$\begingroup\$

Have you looked at https://github.com/caolan/async? It can really help with organizing your code. As the name implies, async was created to help with writing asynchronous code. While you are only calling one asynchronous function, we can still use it to help us structure our entire program.

Async aside, the general idea is to use functions to help us break down our problem. Instead of a loop within a loop, we can think about what that inner loop is doing and create a function to replace it (for example, our replaceChar function below).

With this in mind and using https://github.com/caolan/async#waterfall, we could write something like:

#!/usr/bin/env node

var fs = require('fs')
var program = require('commander')
var markdown = require('markdown').markdown
var async = require('async');

function printResult(e, r) {
    if (e) {
        throw e;
    }
    else {
        console.log(r);
    }
}

function parseMarkdown(data, cb) {
    cb(null, markdown.parse(data));
}

function replaceChar(node, search, replace) {
    var i;
    var re0 = new RegExp(search + '(?=\b)', 'g');
    var re1 = new RegExp(search + '(?!\b)', 'g');
    for (i = 1; i < node.length; ++i) {
        node[i] = node[i].replace(re0, replace).replace(re1, replace);
    }
    return node;
}

function replaceQuotes(tree, cb) {
    var i;
    for (i = 1; i < tree.length; i++) {
        tree[i] = replaceChar(tree[i], '"', '“');
        tree[i] = replaceChar(tree[i], "'", "‘");
    }
    cb(null, tree);
}

function replaceHr(tree, cb) {
    var i;
    for (i = 1; i < tree.length; i++) {
        if (tree[i][0] === 'hr') {
            var hr = tree[i]
            var p = tree[i + 1]

            hr.splice(0, 1, 'para', '* * *')
            p.splice(1, 0, {'class': 'noind'})
        }
    }
    cb(null, tree);
}

function convertToHtml(tree, cb) {
    cb(null, markdown.renderJsonML(markdown.toHTMLTree(tree)));
}

program
  .version('0.0.1')
  .usage('<keywords>')
  .parse(process.argv)

if (!program.args.length)
  program.help()

var filename = program.args[0]

async.waterfall([
    fs.readFile.bind(fs, filename, 'utf8'),
    parseMarkdown,
    replaceQuotes,
    replaceHr,
    convertToHtml
], printResult);
\$\endgroup\$
  • \$\begingroup\$ Welcome to CodeReview! Your proposed code looks good, but code-only answers which only rewrite the code but do not explain why/what exactly is changed are a bit discouraged here. It would be great if you could add a small list of changes you made, and possibly explanations for it (eg you removed duplicated code, extracted code to functions for increased readability, used RegExp, etc). \$\endgroup\$ – tim Mar 4 '15 at 18:31
  • 2
    \$\begingroup\$ I see, thank you @tim, I've updated my answer. In a bit of a rush at the moment, will possibly edit further later today! \$\endgroup\$ – wakeda Mar 4 '15 at 18:49
2
\$\begingroup\$

I'm not really going to comment on the style of your code (e.g., iterative loops vs. async waterfall) but I will call into question the correctness of your solution.

Consider this:

The markdown processor you are using uses JsonML as the intermediate format that you are manipulating. JsonML models HTML/XML pretty directly by design. HTML isn't really a list of top level elements; it is a tree of nested elements. Your implementation only ever visits the top-most layer of the resulting tree.

It will work for a lot of your markup since markdown restricts a lot of what HTML is even able to be generated. But for example, anything nested in lists or tables (both are part of markdown) would never be touched by your code:

1. First

2. Second:
    * Fee
    * Fie
    * Foe

3. Third

Becomes:

  ["ol",
    ["li", ["p", "First"] ],
    ["li",
        ["p", "Second:"],
        ["ul",
            ["li", "Fee"],
            ["li", "Fie"],
            ["li", "Foe"]
        ]
    ],
    ["li", ["p", "Third"]]
  ]

Your code will only ever see the top level unordered list. This is also true of the async version suggested above.

You will need a recursive solution to be able to handle elements nested in a tree.

\$\endgroup\$
  • \$\begingroup\$ You're right. I already ran into this problem. What kind of fix would you suggest? \$\endgroup\$ – janoChen Mar 6 '15 at 15:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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