lsコマンドをクラス化 #11
lsコマンドをクラス化 #11
Conversation
shibainurou
force-pushed
the
my-ls_object
branch
from
d61d544 to
b9c4e93
Compare
08.ls_object/ls_command.rb
Outdated
|
|
||
| view = | ||
| if options.list_mode | ||
| ListView.new(file_names) |
There was a problem hiding this comment.
ここは .new(file_names) が重複しているので、クラス名の部分のみ分岐させるとコードがシンプルになりますね。
There was a problem hiding this comment.
なるほど。クラス名を変数に入れる書き方はRebuilding Railsという本でも書かれていました。
klass = options.list_mode ? ListView : ColumnViewに書き直しました。
08.ls_object/list_view.rb
Outdated
|
|
||
| def display_file_info | ||
| @file_names.map do |f| | ||
| d = Struct::Display.new(f) |
There was a problem hiding this comment.
ブロック引数などでスコープが1〜数行に限られている場合は1文字の変数でも意図がつかめるのですが、この規模のブロックとなると少しコードが読みづらいです。 f は file_name 、 d は display` に置き換えてはどうでしょうか？
There was a problem hiding this comment.
確かにそうですね。特に"f"はすごく読みづらいです。
おすすめの修正方法 「f は file_name」 、 「d は display」に修正しました。
名称を合わせるためにStructもfilename -> file_nameに修正しました。
08.ls_object/list_view.rb
Outdated
| def row_max_lenght(display_file_infos) | ||
| max_len = Struct::MaxLenght.new(0, 0, 0, 0, 0, 0, 0, 0) | ||
| display_file_infos.each do |v| | ||
| max_len.filename = [max_len.filename, v.filename.length].max |
There was a problem hiding this comment.
ここで行っていることは一定の規則性を持った繰り返し処理なので、イテレータを用いて重複をなくしたいですね。
Structを使っていることで少しイテレータを用いづらくなっているかもしれません。場合によってはStructではなくHashを用いることを検討しても良さそうです。
There was a problem hiding this comment.
順番が常に同じ、という意味では配列でもいいかもしれません。これは Struct::Display についても同様です。
There was a problem hiding this comment.
Structから配列に変更しました。
配列のindexをenumっぽいmoduleで実装しましたがenumが存在しないRubyでは少し強引な修正方法でしたでしょうか？
08.ls_object/column_view.rb
Outdated
| require_relative 'base_view' | ||
|
|
||
| class ColumnView < BaseView | ||
| COLUMNS = 3 |
There was a problem hiding this comment.
カラム数を ColumnView.new の引数で渡せるようにすると、より使いやすいクラスになりますね。
There was a problem hiding this comment.
カラム数を ColumnView.new の引数で渡せるように修正しました。
文字列の最大長の取得をイテレータを使って取得できるようにするため
08.ls_object/list_view.rb
Outdated
| print "#{file.timestamp.rjust(row_max_len.timestamp)} " | ||
| print file.filename | ||
| print " -> #{File.readlink(file.filename)}" if file.type == 'l' | ||
| print file[DisplayIndex::TYPE].rjust(row_max_len[DisplayIndex::TYPE]) |
There was a problem hiding this comment.
この使い方をするのであれば file が Hash になるようにして
file[:type].rjust(row_max_len[:type])のように書いた方がコードもスッキリすると思いました。
これであれば定数の定義も不要になりますね。いかがでしょう？
There was a problem hiding this comment.
hashに修正しました！
そうですね。。。私もこの書き方をするならばhashの方が良さそうだなぁと思いながら書いてました。
先のコメントにあった配列での実装にする場合はどのような実装を想定していたのか教えていただけると嬉しいです。
私では配列にした場合の実装方法しか思い浮かばず。
There was a problem hiding this comment.
こちらのコメントの件ですよね？
#11 (comment)
これはこちらのコメントで提案したような実装をイメージしていました 👍 今回はHashで進めるとのことでしたのでこのままでもOKです。
#11 (comment)
08.ls_object/list_view.rb
Outdated
| @file_names.map do |file_name| | ||
| display = [] | ||
| stat = File.lstat(file_name) | ||
| display[DisplayIndex::FILE_NAME] = file_name |
There was a problem hiding this comment.
ここは次のように書けますね。
@file_names.map do |file_name|
[
file_name,
FILE_TYPE_STR[stat.ftype],
print_permission(stat.mode, PERMISSION_STR),
# 以降省略
]
endThere was a problem hiding this comment.
たしかに配列なのでそうすれば順番に詰め込まれるので左辺が省略できてシンプルです。インデックスの書き間違えが無いのもメリットに感じます。
今回はhashに変えたので以下のように書き換えました。
@file_names.map do |file_name|
stat = File.lstat(file_name)
display = {
file_name: file_name,
type: FILE_TYPE_STR[stat.ftype],
# 以降省略
}
display
end
08.ls_object/object_ls.rb
Outdated
| @@ -0,0 +1,155 @@ | |||
| #!/usr/bin/env ruby | |||
There was a problem hiding this comment.
不要なファイルです 💦 削除しました。
lsテスト用ファイルは.gitignoreに書いて対象外としました。
lsコメントをクラス化しました。