CODEWITHSUNDEEP
coffeescript
celwell
celwell

Reputation: 1662

Is there a more elegant way to write this CoffeeScript?

This works, but does any have a suggestion for a more simplified / elegant way to write this?:

if @mediaAddQueue[''+mid]['mediaType'] is 'photo' and
  @mediaAddQueue[''+mid]['econ_status'] is 'loaded' and
  @mediaAddQueue[''+mid]['medium_status'] is 'loaded' and
  @mediaAddQueue[''+mid]['thumb_status'] is 'loaded' and
  not @mediaAddQueue[''+mid]['targetEventRecord']?
    @addMedia @mediaAddQueue[''+mid]['targetEventRecord'], mid, @mediaAddQueue[''+mid]['mediaType']
else if @mediaAddQueue[''+mid]['mediaType'] is 'video' and
  @mediaAddQueue[''+mid]['video_status'] is 'loaded' and
  not @mediaAddQueue[''+mid]['targetEventRecord']?
    @addMedia @mediaAddQueue[''+mid]['targetEventRecord'], mid, @mediaAddQueue[''+mid]['mediaType']

Upvotes: 0

Views: 97

Answers (3)

Guillaume86
Guillaume86

Reputation: 14400

Going from the epidemian answer, I would still refactor it a bit to this:

media = @mediaAddQueue[mid]

typeValidationMap =
  'photo' : (m) ->
    m.econ_status is 'loaded' and
    m.medium_status is 'loaded' and
    m.thumb_status is 'loaded'
  'video' : (m) ->
    m.video_status is 'loaded'
  'default': () -> no

isValidMedia = (m) ->
  return no if m.targetEventRecord?
  validate = typeValidationMap[m.mediaType] or typeValidationMap.default
  validate m

if isValidMedia media
  @addMedia media.targetEventRecord, mid, media.mediaType

side note: I noticed you pass a always null targetEventRecord in that case

Upvotes: 0

epidemian
epidemian

Reputation: 19219

Sure! First, notice that @mediaAddQueue[''+mid] is repeated all over the place, you can replace that with a variable. Also, there's no need to access properties like something['prop'] if the property has a valid identifier as a name; you can do something.prop. Changing those two things already cleans the code quite a bit:

media = @mediaAddQueue[mid]

if media.mediaType is 'photo' and
  media.econ_status is 'loaded' and
  media.medium_status is 'loaded' and
  media.thumb_status is 'loaded' and
  not media.targetEventRecord?
    @addMedia media.targetEventRecord, mid, media.mediaType
else if media.mediaType is 'video' and
  media.video_status is 'loaded' and
  not media.targetEventRecord?
    @addMedia media.targetEventRecord, mid, media.mediaType

Then, notice that the code inside both the if and the else if is the same. I think it would be great if you could give some meaningful name to the conditions so that the code becomes more self-documented and DRY. I don't know much about the context of this code, so i'll be guessing the variable names; try to use something that explains their meaning as clearly as possible:

media = @mediaAddQueue[mid]

isValidPhoto = media.mediaType is 'photo' and
  media.econ_status is 'loaded' and
  media.medium_status is 'loaded' and
  media.thumb_status is 'loaded' and
  not media.targetEventRecord?

isValidVideo = media.mediaType is 'video' and
  media.video_status is 'loaded' and
  not media.targetEventRecord?

if isValidPhoto or isValidVideo
  @addMedia media.targetEventRecord, mid, media.mediaType

Upvotes: 3

pawroman
pawroman

Reputation: 1300

Sure is. There is always something that can be refactored.

Temporary variables

@mediaAddQueue[''+mid]

Is everywhere. Consider refactoring by introducing a temporary helper variable:

elem = @mediaAddQueue[''+mid]

The code will suddenly become more readable.

Functions, functions, functions!

I see you perform a specific type of check (I've assumed that we have the elem variable):

elem['econ_status'] is 'loaded' and
elem['medium_status'] is 'loaded' and
elem['thumb_status'] is 'loaded'

You could write a function that takes such elem (or whatever that object is) and performs this check, its arguments being the object, the value to compare and object's keys. This is very easy in Coffee thanks to splats.

##
# Check whether all obj's keys are set to value.
checkAllKeys = (obj, value, keys...) ->
    for k in keys
        if obj[k] != value
           return false

    return true

Now that previous code block would become:

checkAllKeys(elem, 'loaded', 'econ_status', 'medium_status', 'thumb_status')

If you know that you will check for 'loaded' value often, make yourself another function:

checkLoaded = (elem, keys...) ->
    checkAllKeys(elem, 'loaded', keys...)

If 'econ_status', 'medium_status', 'thumb_status' keys are often checked together, then even one more function might be a good idea:

checkPhotoLoaded = (photo) ->
    checkLoaded(photo, 'econ_status', 'medium_status', 'thumb_status')

My rule for refactorings is that one should write a function for all stuff that repeats more than twice. CoffeeScript makes writing functions fun, and fast.

I hope this has been helpful.

Upvotes: 4

Related Questions