HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

MVC Design using backbone.js

Submitted by: @import:stackexchange-codereview··
0
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

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:

{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 use var r since r is 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.