全文検索エンジンGroongaの開発に参加している堀本です。 Groongaは、Redmineのプラグインとして組み込むことができます。名前を redmine_full_text_searchといいます。 このプラグインによって、Redmineの全文検索をGroongaを使って実行できます。
今回はこのプラグインへの機能追加した時の添削コミットを紹介します。
redmine_full_text_searchを使うと、検索結果からチケット等に遷移した際に、search_id, search_n というURLパラメータが付与された状態でチケット画面が表示されます。
これは、検索精度のチューニングのために付与している情報で、デフォルトで付与されるようになっています。
ただ、ユースケースによっては、これらのパラメーターが付与されたURLでは不都合なことがあります。
そのため、これらのパラメーターを無効にしたいという要望があり、それを実装しました。
実装が完了しPull Requestした当初のソースは以下のようになっていました。
<% @result_set.each_with_index do |e, i| %>
<dt class="<%= e.event_type %> icon icon-<%= e.event_type %>">
<%= content_tag("span", e.project, :class => "project") unless @project == e.project %>
+ <% if fts_add_search_related_parameters_in_url? %>
<%= link_to(e.event_highlighted_title,
e.event_url.merge("search_id" => @search_request.search_id,
"search_n" => i + @search_request.offset),
:data => {:rank => e.rank}) %>
+ <% else %>
+ <%= link_to(e.event_highlighted_title, :data => {:rank => e.rank}) %>
+ <% end %>
</dt>
<dd>
<ol class="search-snippets">
無効にするかどうかをオプションで選択できるようにしています。
<% if fts_add_search_related_parameters_in_url? %>の部分でパラメーターを付与するオプションが有効かどうかを判定し、有効だったらsearch_id, search_n をURLに付与するようにしています。
無効の場合は、<%= link_to(e.event_highlighted_title, :data => {:rank => e.rank}) %>が実行されてパラメーターが付与されないURLになります。
このコードは、プロジェクトのメンテナーによって以下のように添削されました。
上記のコードは、以下のようにURLパラメーターを付与するかどうかの判定を削除し、 search_result_entry_url(e, i)というメソッドを呼び出しその結果を表示するだけのコードになっています。
<% @result_set.each_with_index do |e, i| %>
<dt class="<%= e.event_type %> icon icon-<%= e.event_type %>">
<%= content_tag("span", e.project, :class => "project") unless @project == e.project %>
- <% if fts_add_search_related_parameters_in_url? %>
<%= link_to(e.event_highlighted_title,
- e.event_url.merge("search_id" => @search_request.search_id,
- "search_n" => i + @search_request.offset),
+ search_result_entry_url(e, i),
:data => {:rank => e.rank}) %>
- <% else %>
- <%= link_to(e.event_highlighted_title, :data => {:rank => e.rank}) %>
- <% end %>
</dt>
<dd>
<ol class="search-snippets">
URLパラメーターを付与するかどうかの判定は別のファイルに以下のように定義されました。
module Hooks
module SearchHelper
include FullTextSearch::Hooks::SettingsHelper
+
+ def search_result_entry_url(e, i)
+ if fts_enable_tracking?
+ search_parameters = {
+ "search_id" => @search_request.search_id,
+ "search_n" => i + @search_request.offset,
+ }
+ else
+ search_parameters = {}
+ end
+ e.event_url.merge(search_parameters)
+ end
end
end
添削前のコードでは、HTMLにオプションの有無を判定し、生成するURLを切り替える処理を埋め込んでいましたがこの処理を埋め込んだファイルは ERBテンプレートでした。
ERBテンプレートは、HTMLにRubyスクリプトを埋め込めるため、添削前のコミットのように条件を判定して表示を切り替えるというようなこともできます。
ただ、このようにすると、このファイルの見通しは悪くなります。 ERBテンプレートは、データをどうやって表示させるかを書く場所であるため、表示させるデータを選択する処理を埋め込むと表示を担当する処理と、表示するデータを選択する処理が混在してしまい、複雑なコードになりやすくなります。
Ruby on Rails には、こんな時のためにヘルパーという仕組みが用意されています。
添削後のコードでは、オプションの有無の判定はヘルパーに移動され、ERBテンプレートでは、ヘルパーの処理結果を表示するだけになりました。 これにより、ERBテンプレート内の判定処理がなくなり見通しがよくなりました。
見通しの良いコードはバグを埋め込みにくくなりますし、読みやすく理解もしやすくなります。理解しやすいコードはメンテナンスもしやすいです。 ファイルの役割分担を考えて実装することで、より良いコードになるということに改めて気がつく出来事でした。
