たとえばこのようなコードがある.Twitter でいいねした画像付きツイートを保存して,その URL を Slack に投稿して,サムネイル画像を生成して,必要な情報を DB に書き込むという処理をやっている.
client.on_event(:favorite) do |event|
source = event[:source]
target_object = event[:target_object]
if source[:screen_name] == YOUR_SCREEN_NAME && !target_object[:user][:protected] && target_object[:extended_entities]
id_str = target_object[:id_str]
screen_name = target_object[:user][:screen_name]
url = create_twitter_url(id_str, screen_name)
download_urls = []
target_object[:extended_entities][:media].size.times do |i|
media = target_object[:extended_entities][:media][i]
download_url = media[:media_url_https]
download_urls << download_url
end
text = url
puts text
options[:text] = text
Slack.chat_postMessage(options) if filtering(target_object)
download_urls.each do |download_url|
download_filepath = File.join(IMAGE_DOWNLOAD_DIR, File::basename(download_url))
thumbnail_filepath = File.join(THUMBNAIL_DIR, File::basename(download_url))
begin
body = open(download_url + ':orig', &:read)
download_url = download_url + ':orig'
rescue OpenURI::HTTPError
begin
body = open(download_url, &:read)
rescue OpenURI::HTTPError
raise '画像のダウンロードに失敗しました'
end
end
puts download_url
File.binwrite(download_filepath, body)
Magick::Image.from_blob(body).shift.resize_to_fit(128).write thumbnail_filepath
db.transaction
begin
db.execute INSERT_SQL, download_filepath, screen_name, id_str, Time.now.to_f, target_object[:text]
db.commit
rescue
db.rollback
end
end
end
end
私はこれを見て,困惑しているところである.いろいろ思うところはあるが,とりあえず抜粋すると,
- コメントがない
- 行が空いていない
- 処理ごとに変数が依存しあっている
コメントがない
悪い癖だと思う.自分だけのコードであっても,少し時間が経つと意図が不明になるというのはよくあることで,この条件式はこういう意味とかをとりあえず書いておけば,あとでそこを変更することになっても,現状に合うようにするだけで,タイムリープして条件式を解釈しなおす必要がなくなると思う.
行が空いていない
これもあまり良くないと思っている.先のコードは,もう少し噛み砕くと,次のような処理をやっている.
- いいねイベントを受け取る
- 自分のいいねイベントであり,いいねしたツイートにメディアが付いていて,かつツイートした人が限定公開アカウントでないなら次へ
- Slack に投稿する用にツイートの URL を生成する
- ダウンロードする画像の URL のリスト取得する
- URL を出力し,同時に Slack にも投稿する
- ダウンロードする画像の URL に対して,次のことを行う
- ダウンロード先のパスと,サムネイルを保存するパスを生成する
- 画像をダウンロードする
- サムネイルを生成して保存する
- DB に書き込む
これらの処理がぜんぶ,行を空けずに書いてあるので,初めて見た人にはどこまでがどの処理かが分かりづらくなっている.
処理ごとに変数が依存しあっている
私は,たとえば,ダウンロードとサムネイルの生成は別個で行われるべきだと考える.
そうして download
というメソッドと generate_thumbnail
というメソッドに分けようと思うが,このコードを素直に切り貼りしただけではうまく動かない.
サムネイルの生成が,ダウンロードに用いる body
という変数(ダウンロードしたファイルのデータを表す)に依存しているのである!
この場合は,サムネイルの生成はダウンロードより後に行われるとしているので,切り分ける際には次のようになるだろうか.
def download(url, save_path)
begin
body = open(url + ':orig', &:read)
rescue OpenURI::HTTPError
begin
body = open(url, &:read)
rescue OpenURI::HTTPError
raise '画像のダウンロードに失敗しました'
end
end
end
def generate_thumbnail(from_path, to_path)
File.open(from_path, 'rb') do |f|
Magick::Image.from_blob(f.read).shift.resize_to_fit(128).write to_path
end
end
basename = File::basename url
download_path = File::join DOWNLOAD_DIR, basename
download url, download_path
thumbnail_path = File::join THUMBNAIL_DIR, basename
generate_thumbnail download_path, thumbnail_path
まとめ
勢いよく書いて運用しているコードにあとから機能が必要になったと,これまた深夜テンションで継ぎ足していくと,あとで見直したときに行方不明になってしまう.
普段から疎結合なコードを書く癖がないとすぐこうなる,という典型例っぽい気がしてきた.みなさんは疎結合なコードを書くようにしましょう.あとテストも書きましょう.
常に正常な判断がしたい,正常とは……?
utgwkk.hateblo.jp
utgwkk.hateblo.jp
utgwkk.hateblo.jp