f:id:grooves:20160202205843p:plain:w400

大変長らくお待たせしました。Forkwell Jobs にて、2015年11月24日〜12月31日の期間で開催していた【リファクタリングコンテスト in Ruby 】の審査結果がようやく出揃いました。

今回、なんと最もよいコードに贈られる Ruby賞 を1人のユーザーが独占する結果となりました。

気になる審査結果の前に、あらためて審査員をご紹介します。

松田明 ( @a_matsuda )

f:id:grooves:20160202195645j:plain:w100

Ruby / Rails / Haml / CarrierWave等のコミッター。kaminari / action_args / active_decorator / motorhead 等の作者。
好きな寿司：アナゴ

和田卓人 ( @t_wada )

f:id:grooves:20160202195656p:plain:w100

タワーズ・クエスト株式会社取締役社長、プログラマ。日本におけるテスト駆動開発（TDD）のスペシャリスト。
好きな寿司：赤貝

藤村大介 ( @ffu_ )

f:id:grooves:20160202195707j:plain:w100

grooves、Aiming、Quipperなどでリーダー的な活動をしていたプログラマー。RubyとHaskellが得意。フロントエンド開発の実績も豊富。
好きな寿司：コハダ

それでは審査結果の発表です。

Ruby賞

f:id:grooves:20160202195726p:plain:w400

今回 Ruby賞に選ばれたのは、imaz さんの２つの投稿です。
それぞれを見ていきましょう！

和田氏、藤村氏に最も良かったと選ばれたコード

imaz さんの"【Refactor Me】複数の配列の要素を並び替えるコード" をリファクタリングしました

Before

def sort_lists(a, b)
  a.sort!
  b.sort!
  b.size.times do |i|
    a.size.times do |j|
      x, y = a[j], b[i]
      if y < x
        a[j] = y
        b[i] = x
      else
        next
      end
    end
  end
  b.sort!

  [a, b]
end

require 'minitest/autorun'

class TestSortLists < Minitest::Test
  def test_sort_lists
    data = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h'].shuffle
    assert_equal([['a', 'b', 'c', 'd'], ['e', 'f', 'g', 'h']], sort_lists(data[0..3], data[4..7]))
  end
end

After

def sort_lists(a, b)
  c = [a, b].flatten.sort!

  [c.shift(a.length), c.shift(b.length)]
end

require 'minitest/autorun'

class TestSortLists < Minitest::Test
  def test_sort_lists
    data = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h'].shuffle
    assert_equal([['a', 'b', 'c', 'd'], ['e', 'f', 'g', 'h']], sort_lists(data[0..3], data[4..7]))
  end 

  def test_sort_lists_for_different_size_lists
    data = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h'].shuffle
    assert_equal([['a', 'b', 'c'], ['d', 'e', 'f', 'g', 'h']], sort_lists(data[0..2], data[3..7]))
  end 
end

和田氏コメント

「複数の配列の要素を並び替えるコード」に挑戦された方は何人かいらっしゃいましたが、
この回答は「簡潔に書ける」という Ruby の特色が遺憾なく発揮されており、
リファクタリング前のコードと比べて著しく可読性が改善されている点が印象に残りました。

ひとつ気になるのは、リファクタリング前の実装には存在していた副作用がなくなっている点です。
リファクタリング前のコードでソートされるのは戻り値だけでなく、引数に渡した配列も破壊的にソートされます。

リファクタリングで大事なのは「外部から見た振る舞いを変えずに、内部を改善する」ことなのですが、
この副作用は、はたして外部から見た振る舞いでしょうか?

今回は、引数に及ぼす副作用は、意図していないものであろうと判断しました。
理由は、リファクタリング前のテストに引数の副作用に関するテストが無いためです。
テストに書いていないということは、副作用は意図したものでない、という判断です。
テストが書かれている場合は、意図の有無を調べる材料として既存のテストを使い、
機能互換性に関する判断を行うことができます。
(このあたりの設計判断も回答に書かれているとなお良かったと思います)

などなど、様々な意味で、この投稿は印象に残りました。

藤村氏コメント

Array#shift で副作用を起こしながら結果の配列を作るという、不変性とは何だったのか的なコードでテンションが上がりました。
トリッキーだと感じる人もいるかもしれませんが、僕は簡潔で読みやすいコードだと思います。ということであまり指摘するところが無いんですが、強いて言えばcという変数名はa、bの次にあるものを連想させてミスリーディングかもしれないので、sortedとかにするといいかなあと。

松田氏に最も良かったと選ばれたコード

imaz さんの"【Refactor Me】Controllerをスッキリさせたい" をリファクタリング

Before

class SessionsController < ApplicationController
  def signin
    @user = User.find_by(email: params[:email])

    if BCrypt::Password.new(@user.password_digest) == params[:password]
      self.current_user = @user
      redirect_to root_path
    else
      flash[:alert] = "Login failed."
      render :new
    end
  end
end

After

# app/controllers/sessions_controller.rb
class SessionsController < ApplicationController
  def signin(email, password)
    @user = User.find_by(email: email)

    if @user.authenticate(password)
      self.current_user = @user
      redirect_to root_path
    else
      flash.now[:alert] = "Login failed."
      render :new
    end
  end
end

# app/models/user.rb
class User < ActiveRecord::Base
  has_secure_password validations: false
end

松田氏コメント

バグを出さないおそらく唯一の方法は、プログラムを自分で書かないことです。
特にチームでのアプリケーション開発では、フレームワークやライブラリの機能を理解してうまく使いこなして自前で実装するコードを減らすスキルはとても重要です。
この投稿のAfterのコードは誰がどう読んでもなんの面白みもヒネリもない平凡すぎるコードなんですが、チームメンバーから見るとそんなコードが一番尊いのではないでしょうか。
なお、敢えてツッコミどころを探すなら、User.find_by(email: email)はfind_by! にしておかないとヌルポの可能性がありそうですね。

imaz さんには、回らないお寿司券として使えるギフト券をプレゼントいたします。

審査員特別賞

f:id:grooves:20160202195746p:plain:w500

松田賞

joker1007 さんのパラメーター引数のクラス化とActiveSupport::Concernによるモジュール化

Before

# app/models/pasokara.rb
class Pasokara < ActiveRecord::Base
  validates_presence_of :title, :fullpath, :md5_hash
  validates_uniqueness_of :md5_hash

  include SimpleTaggable

  searchable do
    text :title, stored: true
    string :title_sort do
      title
    end
    string :tags, multiple: true, stored: true do
      tags.map(&:name)
    end
    string :nico_vid, stored: true
    integer :nico_view_count, trie: true
    integer :nico_mylist_count, trie: true
    text :nico_description, stored: true
    time :nico_posted_at, trie: true
    integer :duration, trie: true
  end

  class << self
    def all_with_facet_tags(page: 1, per_page: 100)
      search(include: [:tags]) do
        facet :tags
        paginate page: page, per_page: per_page
      end
    end
  end

  paginates_per 100

  include CreateMethods
  
  # 省略...
end

After

# app/models/pasokara.rb
class Pasokara < ActiveRecord::Base
  validates_presence_of :title, :fullpath, :md5_hash
  validates_uniqueness_of :md5_hash

  include SimpleTaggable
  include Searching

  paginates_per 100

  include CreateMethods

  # 省略...
end

# app/models/pasokara/searching.rb
module Pasokara::Searching
  extend ActiveSupport::Concern

  included do
    searchable do
      text :title, stored: true
      string :title_sort do
        title
      end
      string :tags, multiple: true, stored: true do
        tags.map(&:name)
      end
      string :nico_vid, stored: true
      integer :nico_view_count, trie: true
      integer :nico_mylist_count, trie: true
      text :nico_description, stored: true
      time :nico_posted_at, trie: true
      integer :duration, trie: true
    end
  end

  class SearchParameter
    attr_reader :keyword, :tags, :page, :per_page
    def initialize(keyword: nil, tags: nil, page: 1, per_page: 100)
      @keyword = keyword
      @tags = tags || []
      @page = page || 1
      @per_page = per_page.to_i
      freeze
    end
  end

  module ClassMethods
    def search_with_facet_tags(search_parameter)
      search(include: [:tags]) do
        fulltext search_parameter.keyword if search_parameter.keyword.present?
        search_parameter.tags.each do |tag_name|
          with(:tags, tag_name)
        end
        facet :tags
        paginate page: search_parameter.page, per_page: search_parameter.per_page
      end
    end
  end
end

松田氏コメント

何か特別すごいテクニックが使われているわけでもないんですが、生々しくて実践的な感じがよいですね。
Searchingというモジュール名は、「検索」という意味だろうとは思いますが、現在進行形だと「探してる」みたいなニュアンスになってしまって違和感があります。
Search(検索)、Searcher(検索するやつ)、Sarchable(検索できるやつ)ぐらいでしょうか。

和田賞、藤村賞

kenchan さんの【Refactor Me】if else のテストを書いてリファクタリング

before

0 <= age or raise

if 0 <= age && age <= 2
  'baby'
elsif 3 <= age && age <= 6
  'little child'
elsif 7 <= age && age <= 12
  'child'
elsif 13 <= age && age <= 18
  'youth'
else
  'adult'
end

after

require 'test/unit'

class TimeOfLife
  attr_reader :label, :min_age, :max_age

  def initialize(label, min_age, max_age)
    @label, @min_age, @max_age = label, min_age, max_age
  end

  def include?(age)
   (@min_age..@max_age) === age
  end
end

TIME_OF_LIFES = [
  TimeOfLife.new('baby', 0, 2),
  TimeOfLife.new('little child', 3, 6),
  TimeOfLife.new('child', 7, 12),
  TimeOfLife.new('youth', 13, 18),
  TimeOfLife.new('adult', 19, Float::INFINITY)
].freeze

def age_to_label(age)
  tol = TIME_OF_LIFES.find {|t| t.include?(age) }

  tol&.label || raise
end

class MyTest < Test::Unit::TestCase
  def test_negative_age
    assert_raise do
      age_to_label(-1)
    end
  end

  def test_baby_min
    assert_equal('baby', age_to_label(0))
  end

  def test_baby_max
    assert_equal('baby', age_to_label(2))
  end

  def test_little_child_min
    assert_equal('little child', age_to_label(3))
  end

  def test_little_child_max
    assert_equal('little child', age_to_label(6))
  end

  def test_child_min
    assert_equal('child', age_to_label(7))
  end

  def test_child_max
    assert_equal('child', age_to_label(12))
  end

  def test_youth_min
    assert_equal('youth', age_to_label(13))
  end

  def test_youth_max
    assert_equal('youth', age_to_label(18))
  end

  def test_adult
    assert_equal('adult', age_to_label(19))
  end

  def test_immortality
    assert_equal('adult', age_to_label(200))
  end

  def test_float
    assert_equal('baby', age_to_label(0.1))
  end
end

和田氏コメント

この投稿が良かったのは、レガシーコードのリファクタリングの過程を公開している点です。
ここで言う「レガシーコード」とは、テストコードが書かれていないコードのことです。
https://github.com/kenchan/forkwell-jobs-sushi-contest-2015

リファクタリングは、小さく安全なステップで対象のコードを段階的に綺麗にしていくプロセスです。
安全なステップの「安全」を支えるのはテストです。
そして、レガシーコードはテストが書かれてこなかった故に、テストが書きにくい、
あるいはそのままではテストが書けない構造になっていることが多々あります。

つまりリファクタリングにはテストが必要だが、テストを書くためにはリファクタリング(構造変更)が必要、という一種のデッドロックに陥ります。このデッドロックに直面したとき、私たちは

テストを書けるようになるまでテスト無しで構造変更するか

構造変更を伴わず強引に大外からテストする

という二つの道のどちらかを選択します。 kenchan さんの選んだ道は前者で、
それは対象コードが十分に小さいから、という判断も働いたのでしょう。
github に公開されている履歴を辿っていくと

テストが書けない構造をテスト可能にするために最小限の変更を行う

小さなテスト基盤を整えてテストを必要なだけ記述

そのテストを通しながら内部を改善する

安全圏に達したら、必要に応じて段階的に対象コードの(外部も含め)構造変更を行う

というレガシーコード改善の王道とも言える進め方をしている点が印象に残りました。

藤村氏コメント

ベタ書きだった処理が、適切な範囲の責務を持つクラスに切り分けられていく。
小さな例ですがオブジェクト指向リファクタリングのあるべき姿を示していると感じました！
それでいて冗長になっていないのが個人的に非常に好みです。

僕だったら{ Range: Label }のHashをlookupして終わりにしちゃうところですが、このくらい簡潔に書けるならクラスにしてもいいなあと。
指摘するとしたら、TimeOfLifeはLifeStageのほうが通りがよさそう。

松田氏コメント

※同氏はこちらのコードを次点としておりましたが、その選出時のコメントを掲載しています。

1つぐらいはRubyの新機能を活用したものを採りたかったのでこれを選びました。
この内容ならクラスを作るまでもないだろうという意見もありそうですが、そのあたりは敢えてそういう設計を選んだようですし、そういった方向性も含めて、全体的な仕事の丁寧さに好感が持てます。
コミットログをしたためてくれたのは素晴らしいんですが、日本語コミットはちょっと読む気しないのでプラマイゼロ。
lifeの複数形はlivesなような気がするのでそのあたりが減点対象で、そもそもそういう紛れがありそうな名前を避けるのもスキルのうちかもしれません。

特別賞

今回、Ruby賞、審査員賞それぞれで審査員の意見が一致しましたので、特別にもう一つ賞をご用意しました。
特別賞に選ばれたのは、teruki.shinohara さんのブロックを使って、繰り返し使うコードをメソッドに切り出すです。

※商品は審査員特別賞と同じものを贈呈いたします。

before

# fbのfeedを全て取得する
def get_fb_feeds(fb_api_client)
  option = {iam: 'fb'}
  response = fb_api_client.get_feeds(option)
  fb_feeds = response

  until response.empty?
    option[:max_id] = response.last.id
    response = fb_api_client.get_feeds(option)
    fb_feeds += response
  end

  fb_feeds.flatten
end

# twのtweetを全て取得する
def get_tw_tweets(tw_api_client)
  option = {iam: 'tw'}
  response = tw_api_client.get_tweets(option)
  tw_tweets = response

  until response.empty?
    option[:from_id] = response.last.id
    response = tw_api_client.get_tweets(option)
    tw_tweets += response
  end

  tw_tweets.flatten
end

after

def paginate(&block)
  response = yield(nil)
  sns_posts = response

  until response.empty?
    response = yield(response)
    sns_posts += response
  end

  sns_posts.flatten
end

# fbのfeedを全て取得する
def get_fb_feeds(fb_api_client)
  option = {iam: 'fb'}
  paginate do |response|
    option[:max_id] = response.last.id unless response.nil?
    fb_api_client.get_feeds(option)
  end
end

# twのtweetを全て取得する
def get_tw_tweets(tw_api_client)
  option = {iam: 'tw'}
  paginate do |response|
    option[:from_id] = response.last.id unless response.nil?
    tw_api_client.get_tweets(option)
  end
end

藤村氏コメント

僕も同じようなコードを半年前に書いたので懐かしい思いで見させて頂きました。繰り返し処理の抽象化により、コードがあるべき方向に進んでいると感じました。
paginate の実態は繰り返し処理をして値を返すメソッドなので、paginate（ページを付ける）だとミスマッチかも。
APIリクエストを発行する Client をデータソースとする Enumerator を定義して、それによるストリーム処理にするとよさそうです。

例えば SnsPostsEnumerator.new(FacebookClient.new).each.all という風に書けると、馴染みがよくなりそうです。
オーバーデザインだ！と言われる気もしますが。

プログラムはチームで開発している事が多いので、このように議論のスタートになるリファクタリングはとても価値が高いと個人的には思っています。コードレビューを通して完成形に近づけばよいので。

joker1007 さん、kenchan さん、teruki.shinohara さんには、ネスカフェコーヒーメーカーをプレゼントします。

参加賞

f:id:grooves:20160202195828p:plain:w500

コードをきれいにするアレを手に入れたのはこちらの3名です

hanachin_
yancya
5t111111

すべての受賞者には、Forkwellより個別にご連絡させていただきます。

多くのご参加、誠にありがとうございました！

Forkwell Jobs では、IT 開発者が主役となって活躍できる会社を増やし、そんな会社と開発者が出会うきっかけを創造したいと考えています。
そのために、開発環境をしっかりと整備し、公開できる会社の求人だけを厳選して掲載しています。

Ruby が（きちんと）使える環境に転職したいとお考えの方は、ぜひ Forkwell Jobs をご活用ください！

jobs.forkwell.com

Grooves開発ブログ

リファクタリングコンテスト in Ruby 審査結果発表

松田明 ( @a_matsuda )

和田卓人 ( @t_wada )

藤村大介 ( @ffu_ )

Ruby賞

和田氏、藤村氏に最も良かったと選ばれたコード

Before

After

和田氏コメント

藤村氏コメント

松田氏に最も良かったと選ばれたコード

Before

After

松田氏コメント

審査員特別賞

松田賞

Before

After

松田氏コメント

和田賞、藤村賞

before

after

和田氏コメント

藤村氏コメント

松田氏コメント

特別賞

before

after

藤村氏コメント

参加賞

松田 明 ( @a_matsuda )

和田 卓人 ( @t_wada )

藤村 大介 ( @ffu_ )

Ruby賞

和田氏、藤村氏に最も良かったと選ばれたコード

Before

After

和田氏 コメント

藤村氏 コメント

松田氏に最も良かったと選ばれたコード

Before

After

松田氏 コメント

審査員特別賞

松田賞

Before

After

松田氏 コメント

和田賞、藤村賞

before

after

和田氏 コメント

藤村氏 コメント

松田氏 コメント

特別賞

before

after

藤村氏 コメント

参加賞

松田明 ( @a_matsuda )

和田卓人 ( @t_wada )

藤村大介 ( @ffu_ )

和田氏コメント

藤村氏コメント

松田氏コメント

松田氏コメント

和田氏コメント

藤村氏コメント

松田氏コメント

藤村氏コメント