patternjavascriptMinor
MVC Design using backbone.js
Viewed 0 times
usingdesignbackbonemvc
Problem
I am a backbone.js newbie. I have created a color picker application using backbone.js. I am trying to figure out if I have nailed the MVC concept. Please review my code and let me know if I can do better.
```
MVC Design - Color Picker
$(function(){
//Model for RGB
var RGB = Backbone.Model.extend({
defaults: {R: 255, G: 255, B: 255}
});
//End of Model for RGB
//Model for HEX
var HEX = Backbone.Model.extend({
defaults: {HEX: "000000"}
});
//End of Model for HEX
//Model for HSV
var HSV = Backbone.Model.extend({
defaults: {H: 0, S: 0, V: 1}
});
//End of Model for HSV
//Model for CMYK
var CMYK = Backbone.Model.extend({defaults: {C: 0, M: 0, Y: 0, K:0}});
//End of Model for CMYK
//Begin RGB View
RView = Backbone.View.extend({
id: "RGB",
view: {},
initialize: function(){
this.render();
},
render: function(){
$("#R").val(rgb.get("R"));
$("#G").val(rgb.get("G"));
$("#B").val(rgb.get("B"));
},
events: {
"change" : "globalChange"
},
globalChange: function(){
//set all the textbox values
set();
var res_hsv = rgb2hsv(rgb.get("R"),rgb.get("G"),rgb.get("B"));
var res_cmyk = rgb2cmyk (rgb.get("R"),rgb.get("G"),rgb.get("B"));
$("#R").val(rgb.get("R"));
$("#G").val(rg
```
MVC Design - Color Picker
$(function(){
//Model for RGB
var RGB = Backbone.Model.extend({
defaults: {R: 255, G: 255, B: 255}
});
//End of Model for RGB
//Model for HEX
var HEX = Backbone.Model.extend({
defaults: {HEX: "000000"}
});
//End of Model for HEX
//Model for HSV
var HSV = Backbone.Model.extend({
defaults: {H: 0, S: 0, V: 1}
});
//End of Model for HSV
//Model for CMYK
var CMYK = Backbone.Model.extend({defaults: {C: 0, M: 0, Y: 0, K:0}});
//End of Model for CMYK
//Begin RGB View
RView = Backbone.View.extend({
id: "RGB",
view: {},
initialize: function(){
this.render();
},
render: function(){
$("#R").val(rgb.get("R"));
$("#G").val(rgb.get("G"));
$("#B").val(rgb.get("B"));
},
events: {
"change" : "globalChange"
},
globalChange: function(){
//set all the textbox values
set();
var res_hsv = rgb2hsv(rgb.get("R"),rgb.get("G"),rgb.get("B"));
var res_cmyk = rgb2cmyk (rgb.get("R"),rgb.get("G"),rgb.get("B"));
$("#R").val(rgb.get("R"));
$("#G").val(rg
Solution
Interesting question,
MVC
It seems, you did not nail the MVC concept perfectly.
In essence, you should have 1 single render routine for both RGB, HSV and CMYK since all these are just outputs of the same value from the model. It does not make sense to split that out. ( If you do this exercise you will notice the removal of a ton of copy pasted code).
Readability
Your indenting is terrible, please use a tool like jsbeautifier
Inconsistencies
-
You are using 2 ways to store RGB values:
sticking to one convention would be far more readable ( I would go for the first convention )
-
You should split out the check for valid values of
JsHint
Naming
-
This, is terrible as well:
Do not prefix your variables with
-
Similarly, this
has the prefix of
since you do nothing with the
MVC
It seems, you did not nail the MVC concept perfectly.
In essence, you should have 1 single render routine for both RGB, HSV and CMYK since all these are just outputs of the same value from the model. It does not make sense to split that out. ( If you do this exercise you will notice the removal of a ton of copy pasted code).
Readability
Your indenting is terrible, please use a tool like jsbeautifier
Inconsistencies
-
You are using 2 ways to store RGB values:
{R: 255, G: 255, B: 255} and[computedR,computedG,computedB]sticking to one convention would be far more readable ( I would go for the first convention )
-
You should split out the check for valid values of
r, g and b these validity checks do not belong in conversion routines (plus, they are copy pasted right now)JsHint
- You are missing a ton of semicolons
- In
function rgb2cmyk (r,g,b) {you do not need to usevar rsinceris already known ( as a parameter ).
Naming
-
This, is terrible as well:
var_h = h * 6;
var_i = Math.floor(var_h);
var_1 = v * (1 - s);
var_2 = v * (1 - s * (var_h - var_i));
var_3 = v * (1 - s * (1 - (var_h - var_i)));Do not prefix your variables with
var_, it adds no value at all and hinders readability. Also variables 1 through 3 are terrible variable names.-
Similarly, this
var computedR = 0;
var computedG = 0;
var computedB = 0;has the prefix of
computed which also adds very little (there is plenty of context to tell the reader that these values are computed.) I would have gone for var red, blue, green;since you do nothing with the
0 value anyway.Code Snippets
var_h = h * 6;
var_i = Math.floor(var_h);
var_1 = v * (1 - s);
var_2 = v * (1 - s * (var_h - var_i));
var_3 = v * (1 - s * (1 - (var_h - var_i)));var computedR = 0;
var computedG = 0;
var computedB = 0;var red, blue, green;Context
StackExchange Code Review Q#25953, answer score: 2
Revisions (0)
No revisions yet.