「CoffeeScriptのリファクタリング」の実践とレビュー

CoffeeScriptのリファクタリングと聞いたので、いてもたってもいられなくなった。まず、お題の結果を見ずにやってみる。

これが元のコード

$(document).ready ->
  photoHTML = (photo) =>
    "<li>
       <a id='photo_#{photo.id}' href='#{photo.url}'>
         <img src='#{photo.url}' alt='#{photo.alt}' />
       </a>
    </li>"

  $.ajax
    url: '/photos'
    type: 'GET'
    contentType: 'application/json'
    onSuccess: (response) =>
      for photo in response.photos
        node = $(photoHTML(photo)).appendTo($("#photos-list"))

        node.on('click', (e) =>
          e.preventDefault()
          node.find('img').prop('src', 
            photo.url + '.grayscaled.jpg')
        )
    onFailure: =>
      $("#photo-list").append("<li>
                                 Failed to fetch photos.
                              </li>")

なにかと問題があるように見える。

リファクタ

とりあえずCoffeeScriptとjQueryレベルで綺麗にする

$ ->
  photoHTML = (photo) =>
    "<li>
       <a id='photo_#{photo.id}' href='#{photo.url}'>
         <img src='#{photo.url}' alt='#{photo.alt}' />
       </a>
    </li>"

  $.get('/photos')
    .done ({photos}) =>
      for photo in photos
        node = $(photoHTML(photo)).appendTo($("#photos-list"))
        node.on 'click', (e) =>
          e.preventDefault()
          node.find('img').prop('src', 
            photo.url + '.grayscaled.jpg')

    .fail =>
      $("#photo-list").append("<li>Failed to fetch photos.</li>")

ここで二つ気づいたことがある

multiline textじゃないので文字列が壊れている(バグ)
on 'click' のコールバックが、forスコープがスコープを作らないので一番最後のものに固定されている(バグ)

つまりバグって動いてないコード。

バグ修正

$ ->
  photoTemplate = (photo) => """
    <li>
       <a id='photo_#{photo.id}' href='#{photo.url}'>
         <img src='#{photo.url}' alt='#{photo.alt}' />
       </a>
    </li>
    """

  $photoList = $("#photos-list")
  $.get('/photos')
    .done ({photos}) =>
      photos.forEach (photo) =>
        $photo = $( photoTemplate(photo) )
        $photoList.append $photo

        $photoImg = $photo.find('img')
        $photo.on 'click', (e) =>
          e.preventDefault()
          $photoImg.prop
            src: photo.url + '.grayscaled.jpg'

    .fail =>
      $photoList.append("<li>Failed to fetch photos.</li>")

forEachの関数スコープで閉じ込めたのでphotoの参照は守られる

クラス化

class PhotoView
  constructor: (@photo) ->
    @$el = $('<li>')
    @render(@photo)

    $img = @$el.find('img')
    @$el.on 'click', ->
      e.preventDefault()
      $img.prop src: @photo.url + '.grayscaled.jpg'

  appendTo: ($container) ->
    @$el.appendTo($container)

  render: (photo) -> @$el.html """
    <a id='photo_#{photo.id}' href='#{photo.url}'>
      <img src='#{photo.url}' alt='#{photo.alt}' />
    </a>
  """

$ ->
  $photoList = $("#photos-list")
  $.get('/photos')
    .done ({photos}) =>
      photos.forEach (photo) =>
        photoView = new PhotoView(photo)
        photoView.appendTo $photoList

    .fail =>
      $photoList.append("<li>Failed to fetch photos.</li>")

ここでのポイントは

$elでコンテナ化し、クラス内ではそこに操作を集中させる
- グローバルクエリを投げない
クラス自身がどこにappendされるか知らなくていいようにし、モジュール性を高める
- 自分自身で副作用を起こせないようにする

もちろん、最近のモダンな環境ならビューフレームワークを使ってると思うので、普通はそれを使う。今回はわかりやすく自作。

ListView化

これはやや過剰な気がするけど、一応やってみる。

class View
  appendTo: ($container) ->
    @$el.appendTo($container)

class PhotoListView extends View
  constructor: (@$el) ->
    @$el ?= $('<ul>')

  update: (@photoList) ->
  render: ->
    @$el.empty()
    @photoList.forEach (photo) =>
      photoView = new PhotoView(photo)
      photoView.appendTo @$el

  renderAsFailed: ->
    @$el.empty()
    @$el.append("<li>Failed to fetch photos.</li>")

class PhotoView extends View
  constructor: (@photo) ->
    @$el = $('<li>')
    @render(@photo)

    $img = @$el.find('img')
    @$el.on 'click', ->
      e.preventDefault()
      $img.prop src: @photo.url + '.grayscaled.jpg'

  render: (photo) -> @$el.html """
    <a id='photo_#{photo.id}' href='#{photo.url}'>
      <img src='#{photo.url}' alt='#{photo.alt}' />
    </a>
  """

$ ->
  photoListView = new PhotoListView $("#photos-list")
  $.get('/photos')
    .done ({photos}) =>
      photoListView.update(photos)
      photoListView.render()
    .fail =>
      photoListView.renderAsFailed()

PhotoViewの操作をPhotoListViewに閉じ込める
- 外部からはデータの受け渡しだけ見えるようにする
JavaScriptのピュアなデータ構造と、Viewを切り離す($.getは仕方なく使うけど)
モジュール化しようと思うとだいたい長くなる。プロジェクトの方針がそれを要求しているか、要相談
テンプレートエンジンは必要に応じて使えばよいし、View基底クラスで指定してしまうことが多い
DOMの差分レンダリングは鬼門なので、おとなしくフレームワークを使った方が良い(元記事でもReact使ってる)
ルートビュー以外で大域クエリを投げるな(今回はお題の特製が不明な部分があったので、$('photos-list') は仕方なく使った)

お題の解答

これが筆者のコードらしい

Example of possible refactoring of badly written code - loading photos and making it grayed after click

Photos = {}

class Photos.App
  constructor: ->
    @gui = new Photos.Gui($("#photos-list"))
    @backend = new Photos.Backend()

  start: =>
    @backend.fetchPhotos()
      .done(
        (photos) =>
          for photo in photos
            @gui.addPhoto(photo)
      )
      .fail(@gui.fetchPhotosFailed)

class Photos.Gui
  constructor: (@dom) ->

  photoRow: (photo) =>
    $("<li><a id='photo_#{photo.id}' href='#{photo.url}'><img src='#{photo.url}' alt='#{photo.alt}' /></a></li>")

  addPhoto: (photo) =>
    photoNode = @photoRow(photo).appendTo(@dom)
    @linkClickHandlerToPhoto(photoNode, photo)

  linkClickHandlerToPhoto: (photoNode, photo) =>
    photoNode.on('click', (e) =>
      e.preventDefault()
      @switchPhotoToGrayscaled(photoNode, photo)
      
  switchPhotoToGrayscaled: (photoNode, photo) =>
    photoNode.find('img').prop('src', photo.grayscaledURL())

  fetchPhotosFailed: =>
    $("<li>Failed to fetch photos.</li>").appendTo(@dom)

class Photos.Photo
  constructor: (@id, @url, @alt) ->

  grayscaledURL: =>
    @url + ".grayscaled.jpg"

  @fromJSON: (json) ->
    new Photos.Photo(json.id, json.url, json.alt)

class Photos.Backend
  fetchPhotos: =>
    request = $.ajax(
                url: '/photos'
                type: 'GET'
                contentType: 'application/json'
               )
               .then (response) =>
                 photos = []
                 for photo in response.photos
                   photos.append(Photos.Photo.fromJSON(photo))
                 photos
                 
$(document).ready =>
  # put logic about starting your app here.
  app = new Photos.App()
  app.start()

これがアプリ全体という話は知らなかったので、そもそものエントリポイントの設計方針が違うが… まあ似たようなものになったとは思う。

GuiとBackendという命名はあまり一般的ではないと思うが、やりたいことはわかる。自分はBackend相当の部分にあまり興味がなかったので、こういう感じになってるが、Backendが膨らむかどうかでこの層を作るかどうか決まると思う。

Guiのメソッド分割はやや過剰で、正直自分には追いにくく見える。というか一部シンタックスエラー混じってるし…(.on の部分)

まとめ

適切な粒度で
ライブラリ使え
大域クエリを投げるな(何度でも言う)