15
\$\begingroup\$

I have developed a simple and easy to use JavaScript library called, Omega for practice. I would highly appreciate if you can point out all sorts of flaws in the code such as violations of best practices, conventions and even would highly prefer optimization tips or other better ways of achieving what I did.

Here's the source code:

Omega.js:

/**
 *  Library: Omega.js - Ω.js - A utility library for JavaScript programs. :)
 *
 *  Author: Hassan Althaf
 *  Website: http://hassanalthaf.com
 *  License: MIT
 */

(function (global) {

    var Omega = function () {
        return new Omega.init();
    };

    Omega.init = function () {

    };

    Omega.init.prototype = {
        map: function (originalArray, mappingFunction) {
            var newArray = [];

            if (arguments.length < 2) {
                throw 'This function expects two parameters. The original array and a mapping function.';   
            } else {
                for (var index in originalArray) {
                    newArray.push(mappingFunction(originalArray[index]));
                }
            }

            return newArray;
        },
        filter: function (originalArray, filteringFunction) {
            var filteredArray = [];

            if (arguments.length < 2) {
                throw 'This function expects two parameters. The original array and a filtering function.'; 
            } else {
                for (var index in originalArray) {
                    if (filteringFunction(originalArray[index]) === true) {
                        filteredArray.push(originalArray[index]);
                    }
                }

                return filteredArray;
            }
        },
        implode: function (array, divider) {
            var implodedString = null;

            if (arguments.length < 2) {
                throw 'This function expects two parameters. The first to be an array, and second a string divider.';   
            } else {
                for (index in array) {
                    implodedString = implodedString + array[index] + divider;
                }

                implodedString = implodedString.substring(0, implodedString.length - divider.length);

                return implodedString;
            }
        },
        explode: function (string, needle) {
            if (arguments.length < 2) {
                throw 'This function expects two parameters. A string and a needle.';   
            } else {
                return string.split(needle);
            }
        },
        mergeArrays: function (topArray, bottomArray) {
            if (arguments.length < 2) {
                throw 'This function expects two parameters. A top array and a bottom.';    
            } else {
                for (index in bottomArray) {
                    topArray.push(bottomArray[index]);  
                }

                return topArray;
            }
        },
        eliminateDuplicates: function (array) {
            if (arguments.length < 1) {
                throw 'This function expects one parameter. An array where duplicates need to be eliminated.';
            } else {
                array = this.recursivelyEliminateDuplicates(array);

                return array;
            }
        },
        recursivelyEliminateDuplicates: function(array) {
            for (mainLoopIndex in array) {
                console.log(array);
                var currentItem = array[mainLoopIndex];

                for (index in array) {
                    if ((index !== mainLoopIndex) && (currentItem === array[index])) {
                        array.splice(index, 1);
                        this.recursivelyEliminateDuplicates(array);
                    }
                }
            }

            return array;
        }
    };

    window.Ω = window.Omega = Omega;

}(window));

index.html:

<!DOCTYPE html>
<html lang="en">
    <head>
        <meta charset="UTF-8">
        <title>Omega Workspace</title>
    </head>
    <body>

        <script type="text/javascript" src="Omega.js"></script>
        <script type="text/javascript" src="app.js"></script>
    </body>
</html>

app.js:

var Ω = Ω(); // or Omega();

var numbers = [ 1, 2, 3 ];

var newNumbers = Ω.map( numbers, function(number) {
    return (number * 2);
});

var filteredNumbers = Ω.filter( numbers, function(number) {
    return (number % 2) === 0;
});

var implodeData = Ω.implode( numbers, '-' );

var explodeData = Ω.explode( '192.168.1.1', '.' );

var mergedArrays = Ω.mergeArrays( numbers, [ 4, 5, 6 ] );

var duplicatesEliminated = Ω.eliminateDuplicates( [1, 1, 1, 2, 2, 3, 4, 5, 9, 1, 3, 5, 9, 1, 5, 9, 8, 18, 20] );

console.log(newNumbers);
console.log(filteredNumbers);
console.log(implodeData);
console.log(explodeData);
console.log(mergedArrays);
console.log(duplicatesEliminated);
\$\endgroup\$
5
  • 3
    \$\begingroup\$ new Omega.init() is horrifying and also why doing a new if you never use this, it could be done way simpler, (let alone all those util functions can be done with native functions in one line) \$\endgroup\$
    – caub
    Commented Nov 28, 2015 at 13:24
  • 1
    \$\begingroup\$ @kwak I do use this and I did .init() getting inspired by jQuery. \$\endgroup\$
    – codez
    Commented Nov 28, 2015 at 13:26
  • 2
    \$\begingroup\$ Oh well I just think this is quite more justified/useful when you maintain a state (with variables attributes), well sorry for my opinion, and I also think there are much better things than jquery for inspirations \$\endgroup\$
    – caub
    Commented Nov 28, 2015 at 13:32
  • 1
    \$\begingroup\$ @kwak Yeah sure, I understand that everyone has opinions. And, by the way, this is not the end of this library. The moment I get some sort of inspiration, I will add on more functionality to it. I just suck at thinking of things to code. :) \$\endgroup\$
    – codez
    Commented Nov 28, 2015 at 13:37
  • \$\begingroup\$ Being "inspired by jQuery" is almost always a bad sign, FYI. \$\endgroup\$
    – Dan
    Commented Nov 29, 2015 at 1:05

3 Answers 3

Reset to default
14
\$\begingroup\$

Using Ω as a key:

You shouldn't be using Ω as a keyword to access your library, I did see that you made both Omega and Ω able to access it. I would suggest removing Ω entirely, having both is confusing, and it's not a key on the keyboard (although could be a key-combo, I'm not sure on that), so people can't really type it.

Description:

Library: Omega.js - Ω.js - A utility library for JavaScript programs. :)
  • A utility library for JavaScript programs is a bit... confusing, consider alternatives like: A utility library for JavaScript scripts/JavaScript functions or something similar.
  • Additionally with Ω
  • I hate to be the fun police, but :) is really not necessary :)

throw:

The insides of your functions look like the following:

if (!case){
    throw '';
} else {
   doStuff();
}

This structure is redundant; after a throw has been executed, the function ends, meaning with or without the else, the contents would be executed identically.

Your thrown errors are written with the following format:

This function expects N parameters. A W of X and a Y of Z.

The sentences shouldn't be separated by full stop, instead by colon.

This function expects N parameters: a W of X and a Y of Z.

Miscellanous:

implodedString = implodedString + array[index] + divider;

Instead of string = string + secondString, use the += operator instead, meaning it would become:

implodedString += array[index] + divider;

Empty strings shouldn't be set to null. Set them as empty strings.

var implodedString = null;

into: var implodedString = "";

Omega.init = function () {

};

should really just be Omega.init = function (){};

throw '';

use throw new Error('') instead as it reports with a stack trace.

implodedString = implodedString.substring(0, implodedString.length - divider.length);

return implodedString;

Instead of assigning implodedString that value, just return it directly.

return implodedString.substring(0, implodedString.length - divider.length);

Same for the following:

array = this.recursivelyEliminateDuplicates(array);

return array;

Instead of leaving a blank line before your returns, remove it.

        }

        return newArray;

into:

}
return newArray;

Your explode function is just a reuse of split, and implode is just a sibling of Array.prototype.join()

You have a pointless console.log call in your recursivelyEliminateDuplicates function.

In a few of your functions, you initialise variables before the parameter checks, it's better to initialise those variables afterward as then they aren't added to the compiler until afterward.

for looping over arrays:

Instead of using for loop to iterate over your arrays, use forEach instead.

The following:

for (var index in originalArray) {
    newArray.push(mappingFunction(originalArray[index]));
}

would become:

The library format:

You're doing it wrong. Instead of having init, just append each function as a prototype to Omega.

You don't need to use the new parameter, as the Additionally, the following line makes use of the window variable, but you pass it in as a parameter in the IIFE, which makes the usage redundant.

window.Ω = window.Omega = Omega;

should be:

global.Omega = Omega;

Instead of the object approach you are currently using, consider the following approach instead:

var Omega = function(){};
Omega.prototype.map = function(){
    ...
};
Omega.prototype.filter = function(){
    ...
};
...

All together:

With all the changes I discussed, your code would look like this instead:

(function (global) {

    var Omega = function(){};

    Omega.prototype.map = function (originalArray, mappingFunction) {
            if (arguments.length < 2) {
                throw new Error('This function expects two parameters: the original array and a mapping function.');   
            }
            var newArray = [];
            originalArray.forEach(function(elem){
                newArray.push(mappingFunction(elem));
            });
            return newArray;
    };
    Omega.prototype.filter = function (originalArray, filteringFunction) {
            if (arguments.length < 2) {
                throw new Error('This function expects two parameters: the original array and a filtering function.'); 
            }
            var filteredArray = [];
            originalArray,forEach(function(elem){
                if (filteringFunction(elem) === true) {
                    filteredArray.push(elem);
                }
            });
            return filteredArray;
        };
        Omega.prototype.implode = function (array, divider) {
            if (arguments.length < 2) {
                throw new Error('This function expects two parameters: an array, and a string divider.'); 
            }
            var implodedString = "";
            array.forEach(function(elem){
                implodedString += elem + divider;
            });
            return implodedString.substring(0, implodedString.length - divider.length);
        };
        Omega.prototype.explode = function (string, needle) {
            if (arguments.length < 2) {
                throw new Error('This function expects two parameters. A string and a needle.');   
            }
            return string.split(needle);
        };
        Omega.prototype.mergeArrays = function (topArray, bottomArray) {
            if (arguments.length < 2) {
                throw new Error('This function expects two parameters. A top array and a bottom.');    
            }
            bottomArray.forEach(function(elem){
                topArray.push(elem);  
            });
            return topArray;
        };
        Omega.prototype.eliminateDuplicates = function (array) {
            if (arguments.length < 1) {
                throw new Error('This function expects one parameter. An array where duplicates need to be eliminated.');
            }
             return this.recursivelyEliminateDuplicates(array);
        };
        Omega.prototype.recursivelyEliminateDuplicates: function(array) {
            array.forEach(function(topLevelElem, topLevelIndex){
                array.forEach(function(elem, index){
                    if ((index !== topLevelIndex) && (elem === topLevelElem)) {
                        array.splice(index, 1);
                        this.recursivelyEliminateDuplicates(array);
                    }
                });
            });
            return array;
        };

    global.Omega = Omega;

}(window));

And you would use it like normal, but you wouldn't need to initialise it, as the functions (part of the Omega scope) are already bound to the window.

\$\endgroup\$
11
  • \$\begingroup\$ Thank you! Beautiful and an interesting answer to read. Also, would you be able to recommend me something I could code next? I really suck at thinking of what to code. \$\endgroup\$
    – codez
    Commented Nov 28, 2015 at 16:28
  • \$\begingroup\$ I would recommend looking at UserScripts as something to look at, they let you apply cool scripts to pages you visit in your browser, and are good for helpful little things, we write them all the time on Code Review, take a look at the tag to see some of the ones we've done. \$\endgroup\$
    – Quill
    Commented Nov 28, 2015 at 16:31
  • \$\begingroup\$ Thank you for your response. I'll take a look at it. @Quill \$\endgroup\$
    – codez
    Commented Nov 28, 2015 at 16:36
  • \$\begingroup\$ One more thing. Now you have a variable called Omega that points to an anonymous function. Remove the var and use function Omega(){} instead. imho \$\endgroup\$
    – Pinoniq
    Commented Nov 29, 2015 at 11:07
  • 2
    \$\begingroup\$ In general, the review feels opinionated. E.g. "Instead of leaving a blank line before your returns, remove it." – this is just a matter of code style. \$\endgroup\$
    – Pavlo
    Commented Nov 29, 2015 at 12:33
10
\$\begingroup\$ 
for (var index in originalArray) {

In JavaScript, the for in loop is almost always used only with objects. And yes: while an array in JavaScript is still technically an object, this is bad practice.

It would be better practice to use a normal for loop to iterate through an array.

You did not specify that you are purposely reinventing the wheel here, so I'd like to point out that you are over-complicating some of these methods. Also, in your explode method, you are using a built in method, anyway.

Here are some built in JavaScript methods that already accomplish the same task:

var Omega = function () {
    return new Omega.init();
};

That middle line is very, very weird. Commonly, an init method initializes an object with some data that it will need. However, in this case, you are treating init as a class that you are instantiating. It looks to me that Omega should be the instantiated object here.

It looks even weirder when you do type checking. Here is an example console window:

var foo = new Omega();
==>

Okay, I'm creating a new Omega object.

foo instanceof Omega
==> false

Wait, what? I just created a new Omega!

 foo instanceof Omega.init
 ==> true

Huh? It's an instance of the Omega's init method?

Instead of using this pointless .init object, you should just use Omega and Omega.prototype.

Right now, you are throwing errors like this:

throw 'This function expects two parameters. A top array and a bottom.';

In the console, that looks like this:

Uncaught This function expects two parameters. A top array and a bottom.

What you are doing is just throwing an error message, but "JavaScript" is expecting an actual error, not just a message.

What you are doing right now is throwing a string. Instead, you should be throwing a new Error with the message included. Or, if possible, you can find a more specific error for the method (you could even create your own error).

\$\endgroup\$
4
  • \$\begingroup\$ Thank you! Beautiful and an interesting answer to read. Also, would you be able to recommend me something I could code next? I really suck at thinking of what to code. \$\endgroup\$
    – codez
    Commented Nov 28, 2015 at 16:29
  • \$\begingroup\$ Sorry, but I don't enough information about you to know what you would like to do next. \$\endgroup\$
    – SirPython
    Commented Nov 28, 2015 at 16:31
  • \$\begingroup\$ @HassanAlthaf Learn by doing: get the questions tagged javascript and try to answer it. If you got an answer (or none if you got impatient) check it against the answers already given. Or try the lmits of JavaScript and write (in that order) a bigint library, a rational library, a bigfloat library, a complex library, a language to use it, a library with a lot of mathematical functions, ???, make profit! Well, no , you won't make profit, GP/Pari has been ported to Android. \$\endgroup\$
    – deamentiaemundi
    Commented Nov 28, 2015 at 19:03
  • \$\begingroup\$ One could replace Omega.init with Omega.constructor. It would automatically call that function upon new Omega(). I think it keeps the same prototype. \$\endgroup\$
    – Ismael Miguel
    Commented Nov 29, 2015 at 13:13
8
\$\begingroup\$

new Omega.init()

Your library doesn't maintain state, so you don't need to make it instantiable. Instead you can create an object and assign properties to it:

(function (global) {
    var omega = {};
    omega.map = function () { ... };
    ...
    global.omega = omega;

I've chosen the name omega, with a lowercase "O", because conventionally, names that start with a capital letter designate a function that should / can be called with new, and that would no longer be the case for this object.

for (index in array) { ... }

Since you've omitted the var before index, you'll likely create or override a global variable index as a side-effect of running this loop.

If you want to continue using for in, declare the variable: for (var index in array) { ... }.

Also, add the 'use strict'; directive to the top of your IIFE; then the engine is more likely to throw an error (like "ReferenceError: assignment to undeclared variable index") if you forget the var:

(function () {
    'use strict';
    for (index in [1, 2, 3]) {} // Probably throws a ReferenceError
}());

However, I recommend against using for in. If someone is unlucky enough to run your code in an environment where index is already a global variable, then for in will still override it if you forget the var, and an error won't be thrown. Instead, iterate over the enumerable keys of an object:

Object.keys(object).forEach(function (key) {
    var value = object[key];
    ...
});

Although, the above advice only pertains to enumerating over non-array object properties. It is better to iterate over the indices of an Array object with methods on Array.prototype:

array.forEach(function (value, index) { ... });
array.map(function (value, index) { ... });
array.filter(function (value, index) { ... });
array.reduce(function (soFar, value, index) { ... }, initialValue);

(function (global) { ... }(window));

Do you intend for your library to be used in more environments than just a browser? e.g., Node.js? Or some JavaScript environment that does not exist yet?

If so, you have a few options. The first is to assume nothing about the environment and only provide enough code to run your library in a "pure" (somewhat theoretical) context. i.e., only export a variable:

var omega = (function () {
    var omega = {};
    ...
    return omega;
}());

If users include your script in a <script> element in a browser, coincidentally, all top-level variables become global variables, so they can access the library via the global omega variable if they are so inclined. However, they can also edit the script (or create a build script) that delivers the variable through their favorite module system, e.g. CommonJS (Node.js and Browserify), AMD, and the built-in module system in ES6. Here are some one-line Unix commands that could build the library for said module systems:

cat library.js <(printf 'module.exports = library;\n') > library.commonjs.js
cat <(printf 'define(function () {\n') library.js <(printf 'return library;\n});\n') > library.amd.js
cat library.js <(printf 'export default library;\n') > library.es6.js

By taking this approach, you can maintain your library independently from the rest of the world. You don't need to "add support for X, Y and Z" as X, Y and Z fade in (and out) of popularity. You shift that burden onto the people whom the lack of support concerns.

Although, JavaScript developers are accustomed to getting their libraries served on a silver platter. So, at the expense of agelessness, you can provide support for particular environments using a UMD. This one is my favorite:

(function (root, factory) {
    if (typeof define === 'function' && define.amd) {
        define(function () {
            return factory();
        });
    } else if (typeof module === 'object' && module.exports) {
        module.exports = factory();
    } else {
        root.omega = factory();
    }
}(this, function () {
    var omega = {};
    return omega;
}));

Although, note that the above UMD can't be imported via import in a (real) ES6 environment, because import-imported code is implicitly a module, which runs in strict mode, where this won't be coerced to the global object. So, for forwards-compatibility with modules, and without respect to the name of the global object in whatever environment the code runs in, you have to evaluate code at runtime (which allows you to run non-strict code in a module):

(function (root, factory) {
    ...
}(Function('return this')(), function () {
    ...
}));

Or, if you don't like eval, you could make a laundry list of popular global variable names (which may or may not be the global object):

(function (root, factory) {
    ...
}(typeof this !== 'undefined' ? this :
  typeof window !== 'undefined' ? window :
  ..., function () {
    ...
}));

Although, ES6 module users would probably prefer you export the module via export, which is a SyntaxError in ES5. So if you also want to provide support for ES6, you might consider writing your library in strict mode exclusively and creating a separate build for ES6 with the export concatenated to the bottom.

Hopefully you're starting to see how packaging your code for an arbitrary environment can get messy, which is why I recommend exporting the object via var, and calling it a day.

\$\endgroup\$
2
  • \$\begingroup\$ Welcome to Code Review! Great job on your first answer. \$\endgroup\$
    – SirPython
    Commented Nov 28, 2015 at 23:23
  • \$\begingroup\$ I really wonder why nobody else pointed out that the factory/constructor is totally superfluous (and var Ω = Ω(); clearly being weird). \$\endgroup\$
    – Bergi
    Commented Nov 28, 2015 at 23:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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