JavaScript JavaScript Loops, Arrays and Objects Simplify Repetitive Tasks with Loops The Refactor Challenge, Part 2

Seth Roope
Seth Roope
6,471 Points

DRY as I could get...

Anyone see anywhere you could lean this out even further?

var html = '';

function randomRGB() {
  return Math.floor(Math.random() * 256 );
}

function rgbColor(red, green, blue) {
  return 'rgb(' + randomRGB(red) + ',' + randomRGB(green) + ',' + randomRGB(blue) + ')';  
}

for (var i = 0; i < 160; i += 1) {
  html += '<div style="background-color:' + rgbColor() + '"></div>';
}

document.write(html);

3 Answers

Steven Parker
Steven Parker
174,163 Points

I don't think you can get any more "DRY".

But you can condense rgbColor a bit because randomRGB doesn't take any arguments, so you don't need to pass them to it. And that means rgbColor itself won't need to take any arguments either.

I also had to add an explicit height to the DIV styling to get them to display in the browser (perhaps you also had some CSS with this?).

Here's a version using denser syntax and eliminating local variables other than the loop index:

var randomRGB = () => Math.floor(Math.random() * 256);
var rgbColor = () => 'rgb(' + randomRGB() + ',' + randomRGB() + ',' + randomRGB() + ')';
for (var i = 0; i < 10; i++) {
  document.write('<div style="background-color:' + rgbColor() + '"></div>');
}
Seth Roope
Seth Roope
6,471 Points

Yeah, I added the colors as parameters for readability, but probably making it more confusing...

I did not have to modify the CSS at all to run in firefox.

Do you know if calling one function within another is generally frowned upon since rgbColor is dependent on randomRGB? Could see that causing issues if you wanted to reuse functions in another program.

i.e. Should it be nested?

var html = '';

function rgbColor() {
  function randomRGB() {
    return Math.floor(Math.random() * 256 );
  }
  return 'rgb(' + randomRGB() + ',' + randomRGB() + ',' + randomRGB() + ')';  
}

for (var i = 0; i < 160; i += 1) {
  html += '<div style="background-color:' + rgbColor() + '"></div>';
}

document.write(html);
Steven Parker
Steven Parker
174,163 Points

I don't think they need to be nested, but locating them next to each other in the source is a good idea. A reason to not nest them is if there's any possibility that randomRGB might be useful somewhere else in the program.

You could have the "best of both" by creating an object that has both of these as object methods.

Seth Roope
Seth Roope
6,471 Points

Thanks, that's an awesome idea! Hopefully I will start thinking more that way once we start getting more into the OOP portion of the lessons.

Cheers

Marcin Jerwan
Marcin Jerwan
4,628 Points

will this code only make randomRGB once and if number generated for example was x, would rgbColor not be 'rgb('x,x,x')'? that would make 10 divs same shade of grey?

Steven Parker
Steven Parker
174,163 Points

Marcin, every call to "randomRGB" gets a new random number, and each call to "rgbColor" calls it 3 separate times, so you will always get a different color, and it will be extremely rare to be a shade of grey. Give it a try and see!

Timothy Rackham
Timothy Rackham
7,870 Points

I reduced my solution down to

function color() {
  return Math.floor(Math.random() * 256 );
}
for (var i = 0; i < 10; i += 1) {
  document.write( '<div style="background-color:rgb('+ color() + ',' + color() + ',' + color() + ')"></div>');
}
Seth Roope
Seth Roope
6,471 Points

That's what I'm talking about, nice Timothy! Very simple.

ian izaguirre
ian izaguirre
3,220 Points

I am not 100% sure as I just started learning, but I would think that you would want to avoid puting the document object inside the "for loop", as that is likely bad practice. Any variable that does not change within the loop should be defined outside of the loop to avoid having it be recalculated every time the loop ran (needlessly using resources). You want to keep DOM access to the bare minimum, and by having it inside the "for loop", the javascript interpreter is making it calculate 10 times (every time the loop runs) which will eat up memory and make your script slow.

Steven Parker
Steven Parker
174,163 Points

While 10 interations might not be enough to be concerned about, you're quite right, and this would be more efficient:

var color = () => Math.floor(Math.random() * 256);
var html = "";
for (var i = 0; i < 10; i++)
  html += `<div style="background-color:rgb(${color()},${color()},${color()})"></div>`;
document.write(html);
ian izaguirre
ian izaguirre
3,220 Points

@Steven Parker

Thank you for confirming. As a side note: in the code block you added, you forgot the opening & closing curly braces in the For loop.

Steven Parker
Steven Parker
174,163 Points

I intentionally omitted the braces for compactness. The braces are optional when the body of the loop has only one statement.

ian izaguirre
ian izaguirre
3,220 Points

@Steven Parker

Thanks for making me look that up! I had no idea that was possible :-) . While reading on it, I came across this link: (https://stackoverflow.com/questions/4797286/are-curly-braces-necessary-in-one-line-statements-in-javascript)

Very interesting read. I did find an interesting comment on that page, which was on the answer by "William Malo"... he wrote an example which also showed that its possible to separate statements with commas instead of always using curly braces. A user comment on that said: " I am venturing a guess that the majority of developers are not aware of this (myself included prior to reading this), which I suspect would then quickly become a maintainability issue among developers. Sometimes, the most clever way is not the best."

Hmm, now I am confused. I wonder if omitting the curly braces for one line statements is common knowledge? What do you think, do think it would be a maintainability issue? Or is this something that is commonly done? Thank You for the help.

Steven Parker
Steven Parker
174,163 Points

In actual practice I would almost never omit braces for a loop, though I do occasionaly do it for "if" statements. However "best practice" would be to always use them to make it simpler if and when the need arises to add additional code at a later time.

Now commas are entirely a separate matter. Commas are expression separators where semicolons are statement delimiters. I would consider it a misuse of commas to avoid braces to enclose the body of a loop or conditional. For clarity, always use braces and semicolons for multiple statements.

ian izaguirre
ian izaguirre
3,220 Points

Awesome! thank you for clearing everything up.