Skip to main content
Code Review

Return to Question

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link
Rollback to Revision 12
Source Link
Ethan Bierlein
  • 15.9k
  • 4
  • 60
  • 146
  1. How should this simple universe be organized into classes? One "Wave" class with a constructor and draw method, and another "Waves" class with an animate method? Or is it better to use just one class, Waves, and have a createWave method which the constructor will call \$n\$ times to create all waves? I don't imagine ever needing to create a Wave in isolation from its river / Waves.
  2. Should the velocity (same for all waves) be stored at the Wave, or Waves level?
  3. Is the static CONFIGconfig() gettermethod a good idea, or should each property be separate? Let's assume for simplicity we won't build right now an interface to modify these properties at runtime, but they are to be grouped at the top of the class for easy editing in the code.
  4. Is it better to use just one class, Waves, and have a createWave method which the constructor will call \$n\$ times to create all waves? I don't imagine ever needing to create a Wave in isolation from its river / Waves.

Here's the first stab at this in ES6. Any other tips towards maintainability, ease of reuse, and other good OOP practices are welcome.:

'use strict';
class Wave {
 static get CONFIGconfig() {
 const baseLengthwaveBaseLength = 5;
 return {
 color: '#4F5E5B',
 waveLengths: [baseLength[waveBaseLength, baseLengthwaveBaseLength * 7, baseLengthwaveBaseLength * 15],
 velocity: 5
 }
 }
 constructor(x, y, velocity) {
 this.x = x;
 this.y = y;
 let waveLengths = Wave.CONFIGconfig().waveLengths;
 this.lengthl = waveLengths[Math.floor(Math.random() * waveLengths.length)];
 this.velocity = velocity || 0.5; // Chrome currently fails at `velocity=0.5` in the constructor declaration
 }
 draw(context) {
 context.beginPath();
 context.moveTo(this.x, this.y);
 context.lineTo(this.x + this.lengthl, this.y);
 context.strokeStyle = Wave.CONFIGconfig().color;
 context.stroke();
 }
}
class Waves {
 constructor(n, context) {
 this.waves = [];
 this.context = context;
 for (var i = 1; i <= n; i++) {
 var x = Math.random() * context.canvas.width,
 y = context.canvas.height / n * i;
 this.waves.push(new Wave(x, y));
 }
 }
 step() {
 for (var i = 0; i < this.waves.length; i++) {
 var wave = this.waves[i];
 if (wave.x + wave.lengthl > 0) {
 // if still partially visible in the canvas, draw it
 wave.draw(this.context);
 wave.x -= wave.velocity;
 } else {
 // otherwise, recreate the wave off-screen to the right, at a different Y location to keep it interesting
 // TODO this does create a "gap" as waves come on-screen, but fixing it is not a priority
 wave.x = this.context.canvas.width;
 wave.y = Math.random() * this.context.canvas.height;
 }
 }
 }
}
let canvas = document.getElementById('river');
let context = canvas.getContext('2d');
let waves = new Waves(150, context);
window.requestAnimationFrame = window.requestAnimationFrame || window.msRequestAnimationFrame;
(function loop(timestamp) {
 context.clearRect(0, 0, canvas.width, canvas.height);
 waves.step();
 // keep animating
 window.requestAnimationFrame(loop);
})();
<canvas id="river"></canvas>
  1. How should this simple universe be organized into classes? One "Wave" class with a constructor and draw method, and another "Waves" class with an animate method? Or is it better to use just one class, Waves, and have a createWave method which the constructor will call \$n\$ times to create all waves? I don't imagine ever needing to create a Wave in isolation from its river / Waves.
  2. Should the velocity (same for all waves) be stored at the Wave, or Waves level?
  3. Is the static CONFIG() getter a good idea, or should each property be separate? Let's assume for simplicity we won't build right now an interface to modify these properties at runtime, but they are to be grouped at the top of the class for easy editing in the code.

Here's the first stab at this in ES6. Any other tips towards maintainability, ease of reuse, and other good OOP practices are welcome.

'use strict';
class Wave {
 static get CONFIG() {
 const baseLength = 5;
 return {
 color: '#4F5E5B',
 waveLengths: [baseLength, baseLength * 7, baseLength * 15],
 velocity: 5
 }
 }
 constructor(x, y, velocity) {
 this.x = x;
 this.y = y;
 let waveLengths = Wave.CONFIG.waveLengths;
 this.length = waveLengths[Math.floor(Math.random() * waveLengths.length)];
 this.velocity = velocity || 0.5; // Chrome currently fails at `velocity=0.5` in the constructor declaration
 }
 draw(context) {
 context.beginPath();
 context.moveTo(this.x, this.y);
 context.lineTo(this.x + this.length, this.y);
 context.strokeStyle = Wave.CONFIG.color;
 context.stroke();
 }
}
class Waves {
 constructor(n, context) {
 this.waves = [];
 this.context = context;
 for (var i = 1; i <= n; i++) {
 var x = Math.random() * context.canvas.width,
 y = context.canvas.height / n * i;
 this.waves.push(new Wave(x, y));
 }
 }
 step() {
 for (var i = 0; i < this.waves.length; i++) {
 var wave = this.waves[i];
 if (wave.x + wave.length > 0) {
 // if still partially visible in the canvas, draw it
 wave.draw(this.context);
 wave.x -= wave.velocity;
 } else {
 // otherwise, recreate the wave off-screen to the right, at a different Y location to keep it interesting
 // TODO this does create a "gap" as waves come on-screen, but fixing it is not a priority
 wave.x = this.context.canvas.width;
 wave.y = Math.random() * this.context.canvas.height;
 }
 }
 }
}
let canvas = document.getElementById('river');
let context = canvas.getContext('2d');
let waves = new Waves(150, context);
window.requestAnimationFrame = window.requestAnimationFrame || window.msRequestAnimationFrame;
(function loop(timestamp) {
 context.clearRect(0, 0, canvas.width, canvas.height);
 waves.step();
 // keep animating
 window.requestAnimationFrame(loop);
})();
<canvas id="river"></canvas>
  1. How should this simple universe be organized into classes? One "Wave" class with a constructor and draw method, and another "Waves" class with an animate method?
  2. Should the velocity (same for all waves) be stored at the Wave, or Waves level?
  3. Is the static config() method a good idea, or should each property be separate? Let's assume for simplicity we won't build an interface to modify these properties at runtime, but they are to be grouped at the top of the class for easy editing in the code.
  4. Is it better to use just one class, Waves, and have a createWave method which the constructor will call \$n\$ times to create all waves? I don't imagine ever needing to create a Wave in isolation from its river / Waves.

Here's the first stab at this in ES6:

'use strict';
class Wave {
 static config() {
 const waveBaseLength = 5;
 return {
 color: '#4F5E5B',
 waveLengths: [waveBaseLength, waveBaseLength * 7, waveBaseLength * 15],
 velocity: 5
 }
 }
 constructor(x, y, velocity) {
 this.x = x;
 this.y = y;
 let waveLengths = Wave.config().waveLengths;
 this.l = waveLengths[Math.floor(Math.random() * waveLengths.length)];
 this.velocity = velocity || 0.5; // Chrome currently fails at `velocity=0.5` in the constructor declaration
 }
 draw(context) {
 context.beginPath();
 context.moveTo(this.x, this.y);
 context.lineTo(this.x + this.l, this.y);
 context.strokeStyle = Wave.config().color;
 context.stroke();
 }
}
class Waves {
 constructor(n, context) {
 this.waves = [];
 this.context = context;
 for (var i = 1; i <= n; i++) {
 var x = Math.random() * context.canvas.width,
 y = context.canvas.height / n * i;
 this.waves.push(new Wave(x, y));
 }
 }
 step() {
 for (var i = 0; i < this.waves.length; i++) {
 var wave = this.waves[i];
 if (wave.x + wave.l > 0) {
 // if still partially visible in the canvas, draw it
 wave.draw(this.context);
 wave.x -= wave.velocity;
 } else {
 // otherwise, recreate the wave off-screen to the right, at a different Y location to keep it interesting
 // TODO this does create a "gap" as waves come on-screen, but fixing it is not a priority
 wave.x = this.context.canvas.width;
 wave.y = Math.random() * this.context.canvas.height;
 }
 }
 }
}
let canvas = document.getElementById('river');
let context = canvas.getContext('2d');
let waves = new Waves(150, context);
window.requestAnimationFrame = window.requestAnimationFrame || window.msRequestAnimationFrame;
(function loop(timestamp) {
 context.clearRect(0, 0, canvas.width, canvas.height);
 waves.step();
 // keep animating
 window.requestAnimationFrame(loop);
})();
<canvas id="river"></canvas>
Incorporated Ethan's suggestions. Thanks!
Source Link
  1. How should this simple universe be organized into classes? One "Wave" class with a constructor and draw method, and another "Waves" class with an animate method? Or is it better to use just one class, Waves, and have a createWave method which the constructor will call \$n\$ times to create all waves? I don't imagine ever needing to create a Wave in isolation from its river / Waves.
  2. Should the velocity (same for all waves) be stored at the Wave, or Waves level?
  3. Is the static configCONFIG() methodgetter a good idea, or should each property be separate? Let's assume for simplicity we won't build right now an interface to modify these properties at runtime, but they are to be grouped at the top of the class for easy editing in the code.
  4. Is it better to use just one class, Waves, and have a createWave method which the constructor will call \$n\$ times to create all waves? I don't imagine ever needing to create a Wave in isolation from its river / Waves.

Here's the first stab at this in ES6:. Any other tips towards maintainability, ease of reuse, and other good OOP practices are welcome.

'use strict';
class Wave {
 static configget CONFIG() {
 const waveBaseLengthbaseLength = 5;
 return {
 color: '#4F5E5B',
 waveLengths: [waveBaseLength[baseLength, waveBaseLengthbaseLength * 7, waveBaseLengthbaseLength * 15],
 velocity: 5
 }
 }
 constructor(x, y, velocity) {
 this.x = x;
 this.y = y;
 let waveLengths = Wave.config()CONFIG.waveLengths;
 this.llength = waveLengths[Math.floor(Math.random() * waveLengths.length)];
 this.velocity = velocity || 0.5; // Chrome currently fails at `velocity=0.5` in the constructor declaration
 }
 draw(context) {
 context.beginPath();
 context.moveTo(this.x, this.y);
 context.lineTo(this.x + this.llength, this.y);
 context.strokeStyle = Wave.config()CONFIG.color;
 context.stroke();
 }
}
class Waves {
 constructor(n, context) {
 this.waves = [];
 this.context = context;
 for (var i = 1; i <= n; i++) {
 var x = Math.random() * context.canvas.width,
 y = context.canvas.height / n * i;
 this.waves.push(new Wave(x, y));
 }
 }
 step() {
 for (var i = 0; i < this.waves.length; i++) {
 var wave = this.waves[i];
 if (wave.x + wave.llength > 0) {
 // if still partially visible in the canvas, draw it
 wave.draw(this.context);
 wave.x -= wave.velocity;
 } else {
 // otherwise, recreate the wave off-screen to the right, at a different Y location to keep it interesting
 // TODO this does create a "gap" as waves come on-screen, but fixing it is not a priority
 wave.x = this.context.canvas.width;
 wave.y = Math.random() * this.context.canvas.height;
 }
 }
 }
}
let canvas = document.getElementById('river');
let context = canvas.getContext('2d');
let waves = new Waves(150, context);
window.requestAnimationFrame = window.requestAnimationFrame || window.msRequestAnimationFrame;
(function loop(timestamp) {
 context.clearRect(0, 0, canvas.width, canvas.height);
 waves.step();
 // keep animating
 window.requestAnimationFrame(loop);
})();
<canvas id="river"></canvas>
  1. How should this simple universe be organized into classes? One "Wave" class with a constructor and draw method, and another "Waves" class with an animate method?
  2. Should the velocity (same for all waves) be stored at the Wave, or Waves level?
  3. Is the static config() method a good idea, or should each property be separate? Let's assume for simplicity we won't build an interface to modify these properties at runtime, but they are to be grouped at the top of the class for easy editing in the code.
  4. Is it better to use just one class, Waves, and have a createWave method which the constructor will call \$n\$ times to create all waves? I don't imagine ever needing to create a Wave in isolation from its river / Waves.

Here's the first stab at this in ES6:

'use strict';
class Wave {
 static config() {
 const waveBaseLength = 5;
 return {
 color: '#4F5E5B',
 waveLengths: [waveBaseLength, waveBaseLength * 7, waveBaseLength * 15],
 velocity: 5
 }
 }
 constructor(x, y, velocity) {
 this.x = x;
 this.y = y;
 let waveLengths = Wave.config().waveLengths;
 this.l = waveLengths[Math.floor(Math.random() * waveLengths.length)];
 this.velocity = velocity || 0.5; // Chrome currently fails at `velocity=0.5` in the constructor declaration
 }
 draw(context) {
 context.beginPath();
 context.moveTo(this.x, this.y);
 context.lineTo(this.x + this.l, this.y);
 context.strokeStyle = Wave.config().color;
 context.stroke();
 }
}
class Waves {
 constructor(n, context) {
 this.waves = [];
 this.context = context;
 for (var i = 1; i <= n; i++) {
 var x = Math.random() * context.canvas.width,
 y = context.canvas.height / n * i;
 this.waves.push(new Wave(x, y));
 }
 }
 step() {
 for (var i = 0; i < this.waves.length; i++) {
 var wave = this.waves[i];
 if (wave.x + wave.l > 0) {
 // if still partially visible in the canvas, draw it
 wave.draw(this.context);
 wave.x -= wave.velocity;
 } else {
 // otherwise, recreate the wave off-screen to the right, at a different Y location to keep it interesting
 // TODO this does create a "gap" as waves come on-screen, but fixing it is not a priority
 wave.x = this.context.canvas.width;
 wave.y = Math.random() * this.context.canvas.height;
 }
 }
 }
}
let canvas = document.getElementById('river');
let context = canvas.getContext('2d');
let waves = new Waves(150, context);
window.requestAnimationFrame = window.requestAnimationFrame || window.msRequestAnimationFrame;
(function loop(timestamp) {
 context.clearRect(0, 0, canvas.width, canvas.height);
 waves.step();
 // keep animating
 window.requestAnimationFrame(loop);
})();
<canvas id="river"></canvas>
  1. How should this simple universe be organized into classes? One "Wave" class with a constructor and draw method, and another "Waves" class with an animate method? Or is it better to use just one class, Waves, and have a createWave method which the constructor will call \$n\$ times to create all waves? I don't imagine ever needing to create a Wave in isolation from its river / Waves.
  2. Should the velocity (same for all waves) be stored at the Wave, or Waves level?
  3. Is the static CONFIG() getter a good idea, or should each property be separate? Let's assume for simplicity we won't build right now an interface to modify these properties at runtime, but they are to be grouped at the top of the class for easy editing in the code.

Here's the first stab at this in ES6. Any other tips towards maintainability, ease of reuse, and other good OOP practices are welcome.

'use strict';
class Wave {
 static get CONFIG() {
 const baseLength = 5;
 return {
 color: '#4F5E5B',
 waveLengths: [baseLength, baseLength * 7, baseLength * 15],
 velocity: 5
 }
 }
 constructor(x, y, velocity) {
 this.x = x;
 this.y = y;
 let waveLengths = Wave.CONFIG.waveLengths;
 this.length = waveLengths[Math.floor(Math.random() * waveLengths.length)];
 this.velocity = velocity || 0.5; // Chrome currently fails at `velocity=0.5` in the constructor declaration
 }
 draw(context) {
 context.beginPath();
 context.moveTo(this.x, this.y);
 context.lineTo(this.x + this.length, this.y);
 context.strokeStyle = Wave.CONFIG.color;
 context.stroke();
 }
}
class Waves {
 constructor(n, context) {
 this.waves = [];
 this.context = context;
 for (var i = 1; i <= n; i++) {
 var x = Math.random() * context.canvas.width,
 y = context.canvas.height / n * i;
 this.waves.push(new Wave(x, y));
 }
 }
 step() {
 for (var i = 0; i < this.waves.length; i++) {
 var wave = this.waves[i];
 if (wave.x + wave.length > 0) {
 // if still partially visible in the canvas, draw it
 wave.draw(this.context);
 wave.x -= wave.velocity;
 } else {
 // otherwise, recreate the wave off-screen to the right, at a different Y location to keep it interesting
 // TODO this does create a "gap" as waves come on-screen, but fixing it is not a priority
 wave.x = this.context.canvas.width;
 wave.y = Math.random() * this.context.canvas.height;
 }
 }
 }
}
let canvas = document.getElementById('river');
let context = canvas.getContext('2d');
let waves = new Waves(150, context);
window.requestAnimationFrame = window.requestAnimationFrame || window.msRequestAnimationFrame;
(function loop(timestamp) {
 context.clearRect(0, 0, canvas.width, canvas.height);
 waves.step();
 // keep animating
 window.requestAnimationFrame(loop);
})();
<canvas id="river"></canvas>
it's -> its
Source Link
Loading
improved formatting
Source Link
Quill
  • 12k
  • 5
  • 41
  • 93
Loading
No isolated Wave
Source Link
Loading
improved grammar
Source Link
Quill
  • 12k
  • 5
  • 41
  • 93
Loading
Clarified the static config() method
Source Link
Loading
edited tags
Link
Jamal
  • 35.2k
  • 13
  • 134
  • 238
Loading
added ecmascript-6 tag
Link
Quill
  • 12k
  • 5
  • 41
  • 93
Loading
improved formatting; improved hyperlink structure; removed superfluous tag from title
Source Link
Quill
  • 12k
  • 5
  • 41
  • 93
Loading
deleted 37 characters in body
Source Link
Jamal
  • 35.2k
  • 13
  • 134
  • 238
Loading
Working example in current Chromium. Re-added ES6/ES2015 because Quill didn't add the tags. Use \$n\$ in both places.
Source Link
Loading
fixed usage of n; removed tag from title
Source Link
Quill
  • 12k
  • 5
  • 41
  • 93
Loading
Source Link
Loading
lang-js

AltStyle によって変換されたページ (->オリジナル) /