Rakeタスクをレビューしてもらった
前回の記事でまとめた情報を元にRakeタスクを作りました。
起動すると、はてなブックマークのランキングをスクレイピングし、タイトルを出力し、記事を選択してブラウザで開くという物です
これに対し
というissueをもらったので
それを踏まえて書き直したのが、こちら
これをビデオチャットでレビューしてもらいました。 個人的にはそこそこできたと思っていたのですが、かなり深刻な問題点を指摘・修正してもらいました。
レビューまとめ
実際はもっと細かく指摘されましたが、自分なりにまとめてみました。振り返られるのがpull requestのコードと記憶のみなので無意識に素通りしている所はあるかも知れません。
クラスの区分を考える
私が作ったタスクはclassがRankingScraping
しかなく、スクレイピング・タイトルとURL格納・タイトル出力・記事の選択とブラウザ移行が一つのクラスに記述されていました。
このクラス名はScraper
の様な名詞に、行う処理はスクレイピングと出力のみにし、要素(タイトル・url)の格納とコレクション等は別のクラスを作った方が良いです。
紐付いた値はまとめて管理する
タイトルはtitle_ary
、URLはurl_ary
という配列に格納していましたが、これらは本来セットとなる要素です。取り回し辛い上に、そもそもRubyは配列の順番を保証していません。
これらには新しいクラスを作り、そのインスタンスのインスタンス変数にタイトルとURLとランキングを格納という方法が良いです。
これらのコレクションクラスも必要です。
固定の値はクラス内部に隠す
今回はスクレイピングしたいURLやxpathやcssセレクタを外部からもらう様なことはせず常に固定です。にも関わらず、固定のURLやxpathを変数としてRakefike内に記述していました。これらは値を扱うクラスに定数として置くのが好ましいです。
メソッド名は簡潔に
get_elementやchoice_articleなど冗長なメソッド名が多いです。elementやselectなど可能なら一語で書くのがrubyの慣習らしいです。rubyの慣習にそったコーディングか判断してくれるRubocop
というgemもあるので今度使ってみようと思います。
例外処理は外側に書く
クラスのメソッド内で例外処理を書いていました。結果的に握り潰してしまうことになるので、実行ファイルであるRakefileに記述しました。また、例外処理でプログラムの実行を終了する場合のexit
の戻り値はエラーを意味する1
にします。エラーでない場合は0
ですが、それがデフォルトなので引数なしでも正常終了扱いとなります。
既存の機能を活用する
先ほど言及したコレクションクラスを定義する時にはEnumerable
を使います。
Enumerable
はmix-inしeachを定義するだけでコレクションクラスが簡単に作れる、Arrayクラスもincludeしている繰り返しの汎用モジュールです。rubyはこういった便利な汎用モジュールがあるのでクラス作成にそれを使わない手はないです。
感想
基本的なプログラミングと設計の力がないこと、クラスを使ってオブジェクト指向的にプログラミングする難しさを再認識しました。クラスに関しては数をこなさないと身につかないと言っていたので、これからはもっとコードを書いて少しづつでも身に付けられたらと思っています。