Better way to write this particular function in JavaScript

Question

I'm beginning my journey with JavaScript and programming in general. Not having many developers around I could ask how to do this or do that i'm learning by doing however I'd like some pro's to look around my code and tell me how bad it is.

Any comments for a Noob will be appreciated. Thank you in advance! I know there is probably zillion such classes but I want to learn from scratch and not reuse someones code at this level of my knowledge.

this function returns colors in rgba(r,g,b,a) or rgb(r,g,b) format taking a value for each channel or a random value if "random" is a parameter for an color channel:

someColor = nutils.getNewColor(100,200,"random","random");

will return a string in format:

"rgba(100,232,0.12)" or "rgb(100,200,234)" if no alpha is passed

Here is the code I wrote:



window.utils.getNewColor = function (rRange, gRange, bRange, alpha) {
    function rValue() {
                    return Math.round(Math.random() * 255);
                }

                if (!rRange == undefined || rRange == null || typeof rRange === 'string' && rRange == "random") {
                    rRange = rValue();
                }
                if (!gRange == undefined || gRange == null || typeof gRange === 'string' && gRange == "random") {
                    gRange = rValue();
                }
                if (!bRange == undefined || bRange == null || typeof bRange === 'string' && bRange == "random") {
                    bRange = rValue()
                }
                if (typeof alpha === 'string' && alpha == "random") {
                    alpha = Math.round(Math.random() * 100) / 100
                    return "rgba(" + rRange + "," + gRange + "," + bRange + "," + alpha + ")";
                } else if (typeof alpha === 'number' && alpha < 1) {
                    return "rgba(" + rRange + "," + gRange + "," + bRange + "," + alpha + ")";
                }
                else {
                    return "rgb(" + rRange + "," + gRange + "," + bRange + ")";
                }
            };

UPDATE

After reading your comments I come up with a solution based on @KooilNic advise, however slightly modified to comprehend my lack of understanding ternary operations evaluation.

Here is the modified/updated code:



utils.getNewColor = function (rRange, gRange, bRange, alpha) {
    rRange = rRange || Math.round(Math.random() * 255);
    gRange = gRange || Math.round(Math.random() * 255);
    bRange = bRange || Math.round(Math.random() * 255);
    if (alpha === undefined) {
        return "rgb(" + [rRange, gRange, bRange].join(',') + ")";
    } else {
        alpha = alpha && alpha < 1 ? alpha : (Math.round(Math.random() * 100) / 100);
        return "rgba(" + [rRange, gRange, bRange, alpha].join(',') + ")";
    }
};

KooiInc · Accepted Answer · 2012-03-12 07:01:03Z

1

You can make this method very short using short circuit booleans or in the case of alpha a ternary operator. There's no need to reference window in the method call. All the check for undefined or null are unnecessary. A declared value can be referenced. Within if statements, use === as equality comparison operator, because that compares both compares value and type.

The shorter version of your method:

utils.getNewColor = function (rRange, gRange, bRange, alpha) {
    rRange = rRange || Math.round(Math.random() * 255;
    gRange = gRange || Math.round(Math.random() * 255;
    bRange = bRange || Math.round(Math.random() * 255;
    alpha  = alpha && alpha < 1 ? alpha : (Math.round(Math.random() * 100) / 100;
    return "rgba(" + [rRange,gRange,bRange,alpha].join(',') + ")";
};

edited Mar 12, 2012 at 7:01

answered Mar 12, 2012 at 5:55

KooiInc

124k32 gold badges145 silver badges181 bronze badges

Sign up to request clarification or add additional context in comments.

10 Comments

Dogbert Over a year ago

Did you miss a ? in the alpha line?

Inoperable Over a year ago

If I only knew what circuit booleans are ;-) I probably will. Gonna search for the definition right away. Thanks!

Inoperable Over a year ago

Koolinc, could you tell me in simple vocabulary how those assigment works? I don't get it. alpha = alpha & alpha < 1 alpha : (Math.round(Math.random() * 100) / 100;

Inoperable Over a year ago

@KooiInc great answer, clears a lot. Thank you for the links and for your expertise!

KooiInc Over a year ago

Hi PJJ, glad I could help. The alpha = part uses a ternary operator, kind of a short if. Maybe this link is more clear: javascript.about.com/od/byexample/a/ternary-example.htm

|

metatheorem · Accepted Answer · 2012-03-12 06:21:41Z

0

Consider replacing rValue with a function to include those conditional checks, like so:

function genValue(v) {

    if(v == undefined || v == null || v == "random") {
        return Math.round(Math.random() * 255);
    }

    return v;

}

// and later:
rRange = genValue(rRange);
gRange = genValue(gRange);

That should reduce some code duplication.

Also consider reducing the places where you construct your result ("rgb(" + ... + ")" to one or two places.

EDIT:

Some people are recommending code like !rValue || rValue == "random". Take care with this, as if you pass 0 in for rValue, you will get random colors.

edited Mar 12, 2012 at 6:21

answered Mar 12, 2012 at 5:54

metatheorem

5981 gold badge3 silver badges14 bronze badges

2 Comments

Inoperable Over a year ago

Thank you! Will do and repost the result's.

ZER0 Over a year ago

notice that v == undefined || v == null is redundant. You can simply have one of them, if you don't use strict equal, because undefined == null is true.

ndp · Accepted Answer · 2012-03-12 06:28:18Z

The comparisons like this:

typeof rRange === 'string' && rRange == "random"

can simply be rRange === "random", as === avoids the type conversion.

Creating a fn that encapsulates your test and random generation will shorten the code. The name/concept of this function isn't ideal, but it will work:

function randIfNull(val, maxRandVal) {
    if (val == undefined || val == null || val === "random") {
        return Math.round(Math.random() * (maxRandVal || 255));
    } else {
        return val;
    }
}

(NOTE: I think you have an rRange == undefined || which equals rRange != undefined-- which will almost always evaluate to true. I changed this.)

Then the body of your function can be more simple:

rRange = randIfNull(rRange);
gRange = randIfNull(gRange);
bRange = randIfNull(bRange);

And then you can combine some of the code in your final statements:

if (alpha == "random" || (typeof alpha === 'number' && alpha < 1)) {
    alpha = randIfNull(alpha, 100);
    return "rgba(" + rRange + "," + gRange + "," + bRange + "," + alpha + ")";
}
else {
    return "rgb(" + rRange + "," + gRange + "," + bRange + ")";
}

Collectives™ on Stack Overflow

Better way to write this particular function in JavaScript

UPDATE

3 Answers 3

10 Comments

EDIT:

2 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

UPDATE

3 Answers 3

10 Comments

EDIT:

2 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related