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',
frameWidth: 15,
frameStart: 0,
frameStop: 360
});
this.circle({
fill: true,
fillStyle: 'white',
stroke: true,
strokeStyle: 'lightgrey',
});
this.circle({
fill: true,
colourStart: 'white',
colourFinish: 'rgba(255,255,255,0)'
}),
});
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 < 15) {
segment.update('distanceFromCenter', segment.options.distanceFromCenter + 0.3);
}
if (segment.options.broke === true) {
if (segment.options.height < 105) {
segment.update('height', segment.options.height + 3);
} else if (segment.options.height >= 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 this.outerSegments) {
var segment = this.outerSegments[percentage];
if (segment.options.started === true) {
if (segment.options.outerAnimThickness >= 0) {
segment.update('outerAnimThickness', segment.options.outerAnimThickness - 0.2);
} else {
segment.update('outerAnimDistance', 200);
segment.update('finished', true);
}
if (segment.options.outerAnimDistance < 40) {
segment.update('outerAnimDistance', segment.options.outerAnimDistance + 0.5)
}
this.renderOuterAnim(segment);
}
}
requestAnimationFrame(function() {
self.render();
});
};

Canvas.prototype.circle = function(options) {
options.start = 0;
options.stop = 360;
this.arc(options);
};

Canvas.prototype.arcFrame = function(options) {
this.ctx.beginPath();
this.ctx.arc(
this.canvas.width / 2,
this.canvas.height / 2,
false
);
if (options.fillStyle) {
this.ctx.fillStyle = options.fillStyle;
}
this.ctx.arc(
this.canvas.width / 2,
this.canvas.height / 2,
true
);
this.ctx.fill();
};

Canvas.prototype.arc = function(options) {
this.ctx.beginPath();
if (options.lineWidth) {
this.ctx.lineWidth = options.lineWidth;
}
this.ctx.arc(
this.canvas.width / 2,
this.canvas.height / 2,
options.antiClockwise || false
);
if (options.stroke) {
this.ctx.strokeStyle = options.strokeStyle || 'white';
this.ctx.stroke();
}
if (options.fillStyle) {
this.ctx.fillStyle = options.fillStyle;
}
if (options.fill) {
this.ctx.fill();
}
};

Canvas.prototype.renderSegment = function(segment) {
this.arcFrame({
start: Math.floor(segment.lastPercentage * 3.6),
stop: Math.ceil(segment.percentage * 3.6),
fillStyle: 'green',
frameWidth: segment.options.distanceFromCenter,
frameStart: Math.ceil(segment.percentage * 3.6),
frameStop: Math.floor(segment.lastPercentage * 3.6)
});
};

Canvas.prototype.renderOuterAnim = function(segment) {
this.arcFrame({
start: Math.floor(segment.lastPercentage * 3.6),
stop: Math.ceil(segment.percentage * 3.6),
fillStyle: 'green',
radius: 105 + segment.options.outerAnimDistance,
frameWidth: segment.options.outerAnimThickness,
frameStart: Math.ceil(segment.percentage * 3.6),
frameStop: Math.floor(segment.lastPercentage * 3.6)
});
};

Canvas.prototype.renderPercentage = function(percentage, options) {
this.ctx.fillStyle = options.fillStyle;
this.ctx.font = options.font;
this.ctx.textAlign = options.textAlign;
this.ctx.fillText(percentage, options.x, options.y);
};

Canvas.prototype.clear = function() {
this.ctx.clearRect(0, 0, this.canvas.width, this.canvas.height);
};

Canvas.prototype.circleGradient = function(options) {
this.canvas.width / 2,
this.canvas.height / 2,
this.canvas.width / 2,
this.canvas.height / 2,
);
};

/*Finish Canvas*/

/*Start Segment*/
var Segment = function(percentage, lastPercentage, options) {
this.percentage = percentage;
this.lastPercentage = lastPercentage;
this.options = options || {
height: 20,
distanceFromCenter: 0,
broke: false,
finished: false
};
return this;
};

Segment.prototype.update = function(key, value) {
if (typeof this.options[key] !== "undefined") {
this.options[key] = value;
}
};

var OuterSegment = function(percentage, lastPercentage, options) {
this.percentage = percentage;
this.lastPercentage = lastPercentage;
this.options = options || {
started: false,
outerAnimThickness: 5,
outerAnimDistance: 15,
colour: 'green'
}
return this;
}

OuterSegment.prototype.update = function(key, value) {
if (typeof this.options[key] !== "undefined") {
this.options[key] = value;
}
}
/*Finish Segment*/

var Loader = function(options) {
this.options = options || {
colour: 'green'
};
this.start();
};

Loader.prototype.setPercentage = function(percentage) {
if (typeof percentage === "undefined") {
return percentage;
} else {
if (percentage > 100) {
return false;
}
this.lastPercentage = this.percentage;
this.percentage = percentage;
this.canvas.percentage = this.percentage;
this.canvas.segments[percentage] = new Segment(percentage, this.lastPercentage);
if (percentage === 100 && typeof this.canvas.segments[100] !== "undefined") {
this.canvas.outerSegments[100] = new OuterSegment(100, 0);
} else {
this.canvas.outerSegments[percentage] = new OuterSegment(percentage, this.lastPercentage);
}
if (typeof this.canvas.segments[this.lastPercentage] !== "undefined") {
this.canvas.segments[this.lastPercentage].update('broke', true);
}
}
};

percentage = this.percentage + percentage > 100 ? 100 : this.percentage + percentage;
this.setPercentage(percentage);
};

Loader.prototype.start = function(remove) {
if (typeof remove !== "undefined") {
document.getElementById('canvas').parentNode.removeChild(document.getElementById('canvas'));
}
var canvas = document.createElement('canvas');
canvas.setAttribute('height', 300);
canvas.setAttribute('width', 300);
canvas.setAttribute('id', 'canvas');
this.canvas = new Canvas(document.getElementById('canvas'));
this.canvas.render();
this.percentage = 0;
};

Loader.prototype.reset = function() {
for (var key in this.canvas.segments) {
delete this.canvas.segments[key];
this.canvas.segments = {};
}
this.canvas.percentage = 0;
this.percentage = 0;
this.lastPercentage = false;
}

/*Start Helper*/
var Helper = {
return degrees * Math.PI / 180;
}
};
/*Finish Helper*/

• There are a lot of if-elseif-elses in the code. Maybe you can use more sub-functions to get better readable code? – maja Aug 5 '14 at 7:03
• Everything can be modularised later, I just wanted to know if I was using everything correctly and if certain parts can be simplified. – Ben Fortune Aug 5 '14 at 7:24

There are several valid ways of doing this, but the code still seems a bit convoluted to me.

1. a lot of the work of controlling the size and shape of the segments is within the Canvas prototype, which I would expect (from its name) to be exclusively concerned with drawing.

2. Segment.update simply sets a property and so does not add much value. It would be better if the update methods actually did all the work of changing the shape and size of the segment in each frame.

3. As it stands, Segment and OuterSegment are nearly identical so there's not much point in having both. This would be different if they had their own update methods that defined how they move. Also, they could both inherit from a single Animation prototype which would contain generic logic for creating animated objects and updating them.

4. Having segments and outerSegments as dictionaries with the percentages as keys is not helping to produce straightforward code. Consider storing both in a single array animations.

5. Consider having a separate function or object that defines segment behaviour when broke == true.

6. A different, and ultimately simpler, approach would be to define each stage of the animation -- the first bit where the segments move slowly, the second where they move faster, and the third where they move outside the circle -- as a function that takes the time since the stage begun, and uses that to determine the position and width of the segment. This jsfiddle shows how that could work.

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,


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
• Thanks very much for your input. Further re-factoring will change the hard-coded numbers into configurable variables. You also mention that requestAnimationFrame can be removed of it's closure but also mention that Canvas.prototype.render should have option arguments (if I follow correctly). The only way to do this would be to keep the closure. Comments aren't my strong point but I'll be sure to include some on public release. I agree about the 100% checks but I've had to hack it a bit so it correctly identifies it, without the checks the animation stops at the percentage before 100% – Ben Fortune Aug 6 '14 at 21:46
• So right about the parameter thing! If I were pedantic I would mention bind ;) – konijn Aug 7 '14 at 13:12