0

这行得通，但是有没有人建议以更简化/优雅的方式来写这个？：

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']
4

3 回答 3

4

当然是。总有一些东西可以重构。

临时变量

@mediaAddQueue[''+mid]

无处不在。考虑通过引入一个临时辅助变量来重构：

elem = @mediaAddQueue[''+mid]

代码会突然变得更具可读性。

功能，功能，功能！

我看到您执行了特定类型的检查（我假设我们有elem变量）：

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

您可以编写一个函数来接受此类elem（或任何该对象）并执行此检查，其参数是对象、要比较的值和对象的键。由于 splats，这在 Coffee 中非常容易。

##
# 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

现在之前的代码块将变为：

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.

于 2013-02-07T23:37:53.603 回答
3

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
于 2013-02-07T23:40:53.980 回答
0

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

于 2013-02-08T17:13:00.487 回答