Canvas loading animation - follow-up

This is kind of a follow-on from here:

Canvas animation

I've been trying to copy the following GIF animation using a canvas.

I have re-factored the code following Stuart's pointers and wanted to know if I was doing what I should be.

jsbin

```
/Start Canvas/

var Canvas = function(canvas) {
this.canvas = canvas;
this.ctx = this.canvas.getContext('2d');
this.segments = {};
this.outerSegments = {};
this.percentage = 0;
};

Canvas.prototype.render = function() {
var self = this;
this.clear();
this.arcFrame({
start: 0,
stop: 360,
fillStyle: 'white',
radius: 105,
frameWidth: 15,
frameStart: 0,
frameStop: 360
});
this.circle({
fill: true,
fillStyle: 'white',
stroke: true,
strokeStyle: 'lightgrey',
radius: 20
});
this.circle({
fill: true,
fillStyle: this.circleGradient({
radiusStart: 60,
radiusFinish: 1,
colourStart: 'white',
colourFinish: 'rgba(255,255,255,0)'
}),
radius: 60
});
this.renderPercentage(this.percentage, {
fillStyle: 'green',
font: '16pt Arial',
textAlign: 'center',
x: this.canvas.width / 2,
y: this.canvas.height / 2 + 8
});
for (var percentage in this.segments) {
var segment = this.segments[percentage];

if (segment.options.distanceFromCenter = 105) {
segment.update('height', 105);
this.outerSegments[percentage].update('started', true);
} else {
segment.update('distanceFromCenter', segment.options.distanceFromCenter - 0.1);
}
}
this.renderSegment(segment);
}
if (typeof this.segments[100] !== "undefined") {
if (this.segments[100].options.distanceFromCenter >= 15) {
this.segments[100].update('broke', true);
}
}
for (var percentage in

Interesting question, I was sure @Flambino would pick this one up ;)

Hint/Lint

• Some missing semicolons, no big problem

• Some clear copy pasting between for (var percentage in this.outerSegments) and for (var percentage in this.segments) resulting in declaring var percentage and var segment twice

• You iterate 3 times over an object with for( key in object) without using object.hasOwnProperty(key). You should either

• Use object.hasOwnProperty(key)

• Use Object.keys() to get the keys

• Convert segments and outerSegments to arrays ?

You might be tempted to leave this alone since you are in full control of your script and you wont be modifying the prototype of Object

Copy Pastage

• Definitely the treatment of inner and outer segments could use some re-factoring ( rendering functions, iterating logic etc. )

-
This might also be outer/inner segment, but my CPD caught it:

this.ctx.arc(
       this.canvas.width / 2,
       this.canvas.height / 2,
       options.radius,
       Helper.toRadians(options.start),
       Helper.toRadians(options.stop),

Naming

• Excellent naming, your code is very easy to follow, and I am no canvas expert

• However, so many magical number constants could have used a nice constant name ( 2 , 8 , 60 , 20 etc. etc. )

• Same for the clockwise/counterclockwise parameter in arc, those could have been nicely named variables

Idiomatics

• I much prefer function Canvas(canvas) { over var Canvas = function(canvas) {, death to anonymous functions!!

-
This:

requestAnimationFrame(function() {
    self.render();
});

could be

requestAnimationFrame( this.render );

this would be shorter, and you don't need a reference to self as it's no longer a closure.

Commenting

• There are 0 useful comments in there, and a number of comments that only seem to help the reader in navigating the source

Paranoia

• You only call setPercentage from 1 place, you check for the upper bound of 100 both prior to calling and inside setPercentage, I would consider removing the upper bound check inside setPercentage

Consistency

• If functions like renderPercentage can take options, then so should Canvas.prototype.render

StackExchange Code Review Q#59024, answer score: 4