0
\$\begingroup\$

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?

\$\endgroup\$
1
\$\begingroup\$

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 {}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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