Conversation
| @@ -0,0 +1,59 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| class LsLong < LsFormatter | |||
There was a problem hiding this comment.
- 特に同じインターフェースを共有したりするのでなければ、継承を使わなくてもよいのではと思います。
- インターフェースを共有する場合、
のようにエラーをraiseするだけのメソッドを親クラスに宣言しておいて、実装漏れを減らす方法があります。
def some_method raise "Not Implemented Yet!" end
- インターフェースを共有する場合、
- たとえば
LongFormatterとするなど、クラス名単体で役割がわかるほうがわかりやすいかなと思います。
There was a problem hiding this comment.
自分でも必要ないかな〜と迷いながら書いていました。インターフェースの共有を一つの指針にして継承の必要性を考えてみます。~Formatterにクラス名を変更しました。こちらのほうがわかりやすいですね。
07.ls_object/ls.rb
Outdated
|
|
||
| FILETYPE = { | ||
| '1' => 'p', | ||
| '2' => 'c', | ||
| '4' => 'd', | ||
| '6' => 'b', | ||
| '10' => '-', | ||
| '12' => 'l', | ||
| '14' => 's' | ||
| }.freeze |
There was a problem hiding this comment.
このあたり、long formatのときだけしか使わないと思うので、そちらのクラスに移動したほうがよいかなと思います。クラス内にも定数を宣言でき、場合によってはprivateとすることも可能です。
https://docs.ruby-lang.org/ja/latest/method/Module/i/private_constant.html
There was a problem hiding this comment.
承知しました、そうやって整理していくんですね。移動してprivateにしました。
07.ls_object/ls.rb
Outdated
| def run | ||
| paths = parse_paths(@pathname, @options) | ||
| entries = parse_entries(paths) | ||
| ls = select_formatter(entries, @options) |
There was a problem hiding this comment.
formatterという変数名のほうが直感的に思います。
https://bootcamp.fjord.jp/pages/readable-variable-name
There was a problem hiding this comment.
formatterに変更しました。以前読んだ記事ですが無意識にズレていました😵 改めてしっかり読んでまとめました、ありがとうございます。
| # frozen_string_literal: true | ||
|
|
||
| class LsLong < LsFormatter | ||
| def parse |
There was a problem hiding this comment.
parse というのはおおくの場合、何かしらの文字列を入力とし、何かの構造を出力とする場合が多いです。たとえばプログラミング言語のparserはプログラムのソースコードを入力として、ASTと呼ばれるツリー構造のプログラムの構造を出力とする場合が多いです。
今回はそのようなものではないと思うので、名前に違和感があります。
There was a problem hiding this comment.
そのような意味があるんですね、よく目にしていたので深く考えずに使っていました。理解できたと思います、ありがとうございます。format_outputに変更してみました。
07.ls_object/lib/entry.rb
Outdated
|
|
||
| require 'etc' | ||
|
|
||
| class Entry |
There was a problem hiding this comment.
ちょっと名前が抽象的すぎるかなと思います。 FileStat のラッパーのようになっていると思うので、 FileInfo とか FileMetadata とかファイルの情報をあらわしているよ、というのがわかるようにしましょう。
There was a problem hiding this comment.
承知しました。FileMetadataに変更しました。
07.ls_object/lib/paths.rb
Outdated
| @options = options | ||
| end | ||
|
|
||
| def parse |
There was a problem hiding this comment.
ここもparseではないように思います。
また、そもそも initialize 時点でここまでやってもよいのかなと思います。オブジェクト指向において、オブジェクトの構成要素として大事なのはオブジェクトの状態/データとロジックです。そのオブジェクトがあらわしているモノやコトが内部的にもっている状態やデータは何でしょうか？
Paths となっているので、何らかのパス(複数)を持つのが自然なように思います。
このままだとあまりこのクラスの存在意義がなく、ただメソッドを別ファイルにわけただけ、に近いように思います。
There was a problem hiding this comment.
クラスの存在理由はPathsという情報を持つことなのに、外部からparseメソッドを呼んでわざわざPathsを生成するのであれば、ただ処理を分けただけになりますね💡 理解が深まりました、ありがとうございます！
…tterに変更。親クラスLsFormatterを削除しそれぞれ独立したクラスに修正。それに合わせてls.rbのクラス呼び出し側も修正
…a_listメソッドに名称変更した他、変数名、他メソッドの引数名を変更
…hortFormatterクラスへ移動
07.ls_object/lib/paths.rb
Outdated
| class Paths | ||
| attr_reader :paths | ||
|
|
||
| def initialize(pathname, options) |
There was a problem hiding this comment.
修正する必要まではないと思いますが、ここでは dot_match? と reverse? のオプションしか使ってないので、明示的にその2つを渡してもよいと思います。
関係ないものも含んだ構造に依存してしまうことをスタンプ結合とよびます。興味あれば調べてみてください。
There was a problem hiding this comment.
オブジェクトをそのまま渡した方が効率的に思えたのですが、それも依存のうちなんですね。依存について感度を高めたいので修正してみました。スタンプ結合についても調べてみました。いろいろな結合の分類があることを初めて知りました。奥が深いです🤔
| if file_metadata.setuid? | ||
| [SUID_SGID[user], REGULAR_MODE[group], REGULAR_MODE[others]] | ||
| elsif file_metadata.setgid? | ||
| [REGULAR_MODE[user], SUID_SGID[group], REGULAR_MODE[others]] | ||
| elsif file_metadata.sticky? |
There was a problem hiding this comment.
オブジェクト指向関係ない指摘になってしまって申し訳ないのですが、SUIDとSGIDは両方ONになることがあるので、たとえば、
-rwsr-sr-x 1 yoshitsugu yoshitsugu 0 2月 9 2023 test8.txt
みたいな状況もあり得ます。
スティッキービットも同様です。それぞれ独立して計算できるようにしたほうがよいです。
There was a problem hiding this comment.
同時に設定はできないものだと思い込んでいました。仕様のリサーチをしっかりやらなければと思います。修正いたしました。
2 participants
よろしくお願いいたします。
テストとテスト用のfixturesも作成しています。