This is kind of a follow-on from here:
I've been trying to copy the following GIF animation using a canvas.
enter image description here
I have re-factored the code following Stuart's pointers and wanted to know if I was doing what I should be.
/*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 < 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,
options.radius,
Helper.toRadians(options.start),
Helper.toRadians(options.stop),
false
);
if (options.fillStyle) {
this.ctx.fillStyle = options.fillStyle;
}
this.ctx.arc(
this.canvas.width / 2,
this.canvas.height / 2,
options.radius + options.frameWidth,
Helper.toRadians(options.frameStart),
Helper.toRadians(options.frameStop),
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.radius,
Helper.toRadians(options.start),
Helper.toRadians(options.stop),
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',
radius: segment.options.height,
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) {
var gradient = this.ctx.createRadialGradient(
this.canvas.width / 2,
this.canvas.height / 2,
options.radiusStart,
this.canvas.width / 2,
this.canvas.height / 2,
options.radiusFinish
);
gradient.addColorStop(0, options.colourFinish);
gradient.addColorStop(1, options.colourStart);
return gradient;
};
/*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*/
/*Start Loader*/
var Loader = function(options) {
this.options = options || {
loader: 'loader',
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);
}
}
};
Loader.prototype.addPercentage = function(percentage) {
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.options.loader.appendChild(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;
}
/*Finish Loader*/
/*Start Helper*/
var Helper = {
toRadians: function(degrees) {
return degrees * Math.PI / 180;
}
};
/*Finish Helper*/
-
\$\begingroup\$ There are a lot of if-elseif-elses in the code. Maybe you can use more sub-functions to get better readable code? \$\endgroup\$maja– maja2014年08月05日 07:03:35 +00:00Commented Aug 5, 2014 at 7:03
-
\$\begingroup\$ Everything can be modularised later, I just wanted to know if I was using everything correctly and if certain parts can be simplified. \$\endgroup\$Ben Fortune– Ben Fortune2014年08月05日 07:24:55 +00:00Commented Aug 5, 2014 at 7:24
2 Answers 2
There are several valid ways of doing this, but the code still seems a bit convoluted to me.
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.Segment.update
simply sets a property and so does not add much value. It would be better if theupdate
methods actually did all the work of changing the shape and size of the segment in each frame.As it stands,
Segment
andOuterSegment
are nearly identical so there's not much point in having both. This would be different if they had their ownupdate
methods that defined how they move. Also, they could both inherit from a singleAnimation
prototype which would contain generic logic for creating animated objects and updating them.Having
segments
andouterSegments
as dictionaries with the percentages as keys is not helping to produce straightforward code. Consider storing both in a single arrayanimations
.Consider having a separate function or object that defines segment behaviour when
broke == true
.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)
andfor (var percentage in this.segments)
resulting in declaringvar percentage
andvar segment
twice - You iterate 3 times over an object with
for( key in object)
without usingobject.hasOwnProperty(key)
. You should either- Use
object.hasOwnProperty(key)
- Use
Object.keys()
to get the keys - Convert
segments
andouterSegments
to arrays ? You might be tempted to leave this alone since you are in full control of your script and you wont be modifying theprototype
ofObject
- Use
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) {
overvar 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 of100
both prior to calling and insidesetPercentage
, I would consider removing the upper bound check inside setPercentage
Consistency
- If functions like
renderPercentage
can take options, then so shouldCanvas.prototype.render
-
1\$\begingroup\$ 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 thatCanvas.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 the100%
checks but I've had to hack it a bit so it correctly identifies it, without the checks the animation stops at the percentage before100%
\$\endgroup\$Ben Fortune– Ben Fortune2014年08月06日 21:46:53 +00:00Commented Aug 6, 2014 at 21:46 -
1\$\begingroup\$ So right about the parameter thing! If I were pedantic I would mention
bind
;) \$\endgroup\$konijn– konijn2014年08月07日 13:12:13 +00:00Commented Aug 7, 2014 at 13:12