Abstracting out two lines of code in Coffeescript

Question

I am currently working on refactoring two methods that exist in directives into a service.

This is method one:

scope.computeStyle = (component) ->
  elementHash = {}
  if component.element.type == 'table'
    elementHash.height = 600
    if component.height?
      elementHash.height = component.height * 50
  else if component.element.type == 'single_value'
    elementHash.height = 200
    elementHash.width = 800
  return elementHash

This is method two:

scope.computeStyle = (element, rowComponent) ->
  elementHash = {}
  if element.type == 'table'
    elementHash.height = 600
    if rowComponent?.height?
      elementHash.height = rowComponent.height
  else if element.type == 'single_value'
    elementHash.height = 200
    elementHash.width = 800
  return elementHash

They both do the same thing (a little differently). They take an element/component (which are objects containing data about an object in my application), checks what type of object they are (in this case, a table or a single value), and applies a height and/or width transformation.

Within my service, this is what I have done:

angular = require "angular"

angular.module("myapp.dashboards.layouts").service("DashboardLayoutComputeStyle" ->

  @computeStyle = (componentElement, height) ->
    elementHash = {}
    if componentElement.type == 'table'
      elementHash.height = 600
      if height?
        elementHash.height = height
    else if element.type == 'single_value'
      elementHash.height = 200
      elementHash.width = 800
    return elementHash

  @
)

Here's where my question comes in:

I am not sure what to do at this line in my abstracted method:

  if height?
    elementHash.height = height

In method one, this is what it looks like:

    if component.height?
      elementHash.height = component.height * 50

In method two, this is what it looks like:

    if rowComponent?.height?
      elementHash.height = rowComponent.height

I have three constraints to the refactoring of these two lines:

1. I must check whether both rowComponent and height exist in method two

2. I don't need to check whether component exists, but I must check whether height exists in method one

3. In method one, I must set the elementHash,height to component.height * 50, while in method two, I can simply assign rowComponent.height to elementHash.height

How would I abstract these two lines out?

Answer 1

I'd say your abstracted function looks OK - it's up to the caller to supply the right height. That's the abstraction. The function doesn't have to know exactly what it's looking at, it just needs an element and (optionally) a height.

So you either call:

@computeStyle componentA.element, componentA.height * 50

or

@computeStyle someElement, componentB?.height

if I'm not mistaken. Something like that anyway.

I'd be more worried about the magic width/height values in the function, to be honest. And I'd encourage you to use is rather than == - both compile into === so there's zero difference, but I'd use the more coffeescript'y is.

Oh, and you have an error in your else if conditional: You're referencing element, but in your abstracted function, it's called componentElement

Incidentally, since returns are implicit at the exit point of a function, and object literals can be written quite tersely, you can reduce your function to this:

computeStyle = (componentElement, height) ->
  switch componentElement.type
    when 'table'
      height: height or 600
    when 'single_value'
      height: 200
      width: 800
    else
      {}

or even more tersely:

computeStyle = (componentElement, height) ->
  switch componentElement.type
    when 'table' then height: height or 600
    when 'single_value' then height: 200, width: 800
    else {}