Coffeescript Review

Question

class ShareModel
  constructor: ->
    questionaireView = new QuestionnaireView
    @previewImages =
      shareCurrent: -> ShareModel::getSingle(ModelView::getCurrentImage(),'current',questionaireView.weight(),questionaireView.weightUnits())
      shareGoal: -> ShareModel::getSingle(ModelView::getGoalImage(),'goal',questionaireView.goal(),questionaireView.weightUnits())
      shareSideBySide: -> ShareModel::getDualImage(questionaireView.weight(),questionaireView.goal(),questionaireView.weightUnits())

  getSingle:(image,caption,weight,units) ->
    url = "http://..."
    url += "?current=#{image}"
    url += "&template=single"
    url += "&text=#{caption}"
    url += "&weight=#{weight+units}"

  getDualImage:(current,goal,units) ->
    url = "http://..."
    url += "?current=#{ModelView::getCurrentImage()}&goal=#{ModelView::getGoalImage()}"
    url += "&template=side_by_side"
    url += "&current_weight=#{current+units}&goal_weight=#{goal+units}"

Answer 1

Next time, add a brief description of what the code actually does. Sure, we can read it, but it still leaves a lot of questions. For instance, the current parameter: Is that a path to an image, or a number or what?

Anyway, here are some observations, but I don't know enough of the context to fully refactor your code.

1. Build your parameters from an object; don't hand-code them. Use jQuery's $.param or something similar to build the query string. It makes everything more explicit and less error-prone.
   Here's how it'd work with jQuery

   url = "http://...?" + $.param
     { name: "current", value: someVar }
     { name: "text",    value: someVar }
     ...
   

2. Don't add instance methods to your "class" and then call them as prototype methods (you seem to be doing the same for ModelView). Either make local functions or IIFEs just to group the code and keep it private, or make them "static methods" (i.e. define a method with @foo -> on ShareModel, and call it as ShareModel.foo() from anywhere), or keep them as instance methods and just call with @getSingle(...). Calling a prototype's methods directly is something you can do in javascript, but that doesn't mean it's the right thing to do.
   Alternatively: Why not make the previewImages functionality a separate class? Or create global utility functions?

3. Keep your naming consistent. One method is called getDualImage while the other is called getSingle with no Image suffix. Also, I'd call the former one getSideBySideImage as that's what it's called elsewhere (and "dual" could imply getting 2 image URLs, not one)

4. Keep the signatures more consistent too. One method (getSingle) is sent all the variables it needs, while the other accesses some of them itself. The latter seems more appropriate, as most parameters can indeed be accessed from the methods themselves.
   The same can be said of the parameters; the two URLs end up being very different. Passing parameter arrays might be better, i.e. weight[]=...&weight[]=... for the side-by-side.
   And current seems an odd choice for a parameter name, when it might not actually be the current image, but the goal image instead. I'd prefer something more generic like image or whatever is appropriate for the kind of value you're sending.

5. Lastly, it might be easier to use 3 separate base URLs for side-by-side, current, and goal images. Right now, you're passing a hard-coded template parameter, and (sometimes) a hardcoded text parameter; why not have URLs like current.png?param..., goal.png?param... and /side-by-side.png?param... instead?

As mentioned, I'd provide more code, but there's a lot that could be done differently, and I don't know enough of the context to know what will work, and what won't.