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。その引数は、オブジェクト、比較する値、およびオブジェクトのキーです。スプラットのおかげで、これはコーヒーでは非常に簡単です。

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

「ロードされた」値を頻繁にチェックすることがわかっている場合は、別の関数を作成してください。

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

'econ_status', 'medium_status', 'thumb_status'キーが頻繁に一緒にチェックされる場合は、関数をもう 1 つ追加することをお勧めします。

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

リファクタリングに関する私のルールは、2 回以上繰り返すすべてのものに対して関数を作成する必要があるというものです。CoffeeScript を使用すると、関数を楽しくすばやく作成できます。

これがお役に立てば幸いです。

于 2013-02-07T23:37:53.603 に答える
3

もちろん！まず、 があちこちで繰り返されていることに注意して@mediaAddQueue[''+mid]ください。これを変数に置き換えることができます。something['prop']また、プロパティが名前として有効な識別子を持っている場合のように、プロパティにアクセスする必要はありません。できますsomething.prop。これら 2 つのことを変更すると、コードがかなりきれいになります。

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

if次に、と の両方の内部のコードが同じであることに注意してelse ifください。コードがより自己文書化され DRY になるように、条件に意味のある名前を付けることができれば素晴らしいと思います。このコードのコンテキストについてはよくわからないので、変数名を推測します。その意味をできるだけ明確に説明するものを使用するようにしてください。

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

流行の答えから、私はまだこれを少しリファクタリングします：

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

補足: その場合、常に null の targetEventRecord を渡すことに気付きました

于 2013-02-08T17:13:00.487 に答える