Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

How to make this code in constructor more DRY? Is there some more concise way to assign variables passed through object?

class Ball
  constructor: (args) ->
    @name  = args.name
    @x     = args.x
    @y     = args.y
    @color = args.color

ball = new Ball(name: "ball1", x: 20, y: 20, color: "red")
share|improve this question
add comment

1 Answer 1

up vote 5 down vote accepted

TL;DR
Your code is fine, it doesn't need any improvement. What is there is all explicit code about what your code does/expects. It does not violate the DRY principle.


Every line in your code is very explicit about what it does. Beware the danger of trying so hard to avoid each slightest bit of apparent duplication that your codes loses the ability to explicitly convey its intent.

I've seen this mistake manifest in two ways:

1) A belief that all objects should have behaviour X
So buried in a base class somewhere, all objects get behaviour X. Which is fine and well until things start behaving unexpectedly for a handful of classes that really don't want behaviour X! An explicit call for the objects or classes that support behaviour X avoids this problem, and is not a violation of DRY - even if you make that simple call on every one of your current classes.

2) Generalisation to the Nth degree
You see yourself regularly assigning a number of identically named parameters/attributes from A to B. So you try to find a way to make this more generic. Pseudo code example:

for each attribute i in A
  B[i.Name] = i.Value

There are a number of problems with this approach:

  • If we really need to make an exact copy of A, isn't there a way for us to just use A instead? (Not always possible, but when it is - it's obviously the better option.)
  • What happens with attributes of A not in B and vice versa?
  • What will be the impact of adding an attribute to A which will now also be implicitly copied?
  • What happens with attributes that shouldn't be copied as-is?
  • The above concerns make the generic code error-prone. So typically there's no error checking, or it's very relaxed. The result being that if you now remove an attribute from A, suddenly B doesn't work correctly - but there aren't any errors to warn you.

In my experience, usually this excessive generalisation amounts to no more than a poor re-invention of the wheel by placing your own type system on top of the one provided by your language.


Let's consider some conceptual alternatives to your code. (Disclaimer: I'm not familiar with coffeescript, so it's unlikely any of this code would work - but that's not the point of my code samples.)

class Ball
  constructor: (args) ->
    @ballAttributes = args //or any other generic mechanism that avoids "duplicating" attribute names.

We've eliminated the duplication, but at the cost of losing the explicit indication of what attributes define a ball. I might try create your ball as follows (which is obviously incorrect - but I have no idea what attributes are allowed):

ball = new Ball(belongsTo: "George", shape: "sphere", y: 20, temperature: 97)

The only other place I could imagine removing the "duplicate" attribute names would be from the constructor call. Most languages support "parameters by position", which is fine for a very small number of parameters, but gets confusing and error prone as the number of parameters increases. E.g. I've added radius to your example, and introduced a not so obvious bug trying to place the ball at coordinates (0, 0)

class Ball
  constructor: (name, radius, x, y, colour) ->
    @name   = args.name
    @radius = args.radius
    @x      = args.x
    @y      = args.y
    @color  = args.color

ball = new Ball("ball1", 0, 0, 15, "red")

If any improvement is to be made (if it's even feasible in this language), you could consider abstracting the concept of position by defining a point class. In which case you could create your ball as follows:

ball = new Ball(name: "ball1", radius: 15, position: new Point(x: 0, y: 0), color: "red")
share|improve this answer
2  
Agree. As an addendum. DRY - while good - increases coupling - which is bad. It's all about finding appropriate fault lines and striking a balance infoq.com/news/2012/05/DRY-code-duplication-coupling –  George Mauer Jul 7 '12 at 14:55
    
Thanks for so deep explanation! It helps me to look at this problem from different angles. –  evfwcqcg Jul 7 '12 at 15:57
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.