majodev / metalsmith-data-markdown

A Metalsmith plugin to use markdown content within html tags via data-markdown attribute
http://ranf.tl/2014/10/01/extracting-libs-from-a-node-js-project/
14 stars 4 forks source link

Unessissary use of _.isUndefined() #1

Closed unstoppablecarl closed 10 years ago

unstoppablecarl commented 10 years ago

It is a minor thing but I do not think it is necessary to use _.isUndefined() in this context.

https://github.com/majodev/metalsmith-data-markdown/blob/master/index.js#L29

_.isUndefined in lodash source

https://github.com/lodash/lodash/blob/2.4.1/dist/lodash.compat.js#L2917

In all versions of nodejs undefined is not mutable.

It would be much simpler to read and write if(options === undefined){ ....

unstoppablecarl commented 10 years ago

I just looked at your other new metalsmith plugins. I will be putting them to good use :smiley:

metalsmith-headings-identifier has the same problem with its options code.

The way you wrote your options code in metalsmith-word-count is much easier to read and just as effective.

https://github.com/majodev/metalsmith-word-count/blob/master/index.js#L19

majodev commented 10 years ago

ah finally, you might be the first one ever, who reviews the code of one of my lil' projects. Thanks for your interest!

I will definitely change the default options setup in my code this evening, as you said, it looks horrible. ^^

majodev commented 10 years ago

Defaults setup code of metalsmith-data-markdown changed (also finally added a unit test). I will close this issue here and update my other plugins as well...

Thanks for your feedback!