オブジェクト指向版lsコマンドの実装 by kutimiti1234 · Pull Request #14 · kutimiti1234/ruby-practices

kutimiti1234 · 2024-08-25T09:18:20Z

lsコマンド実装したもの

larオプション

実装しなかったもの

ファイルの列ごとの幅調整(すべてのファイルの名前の中で最も文字数の大きなものを幅として設定した)

…rクラスとFileEntriesListクラスの機能がほぼ同じのため、EntriesListモジュールを作成し、共通化

u1tnk · 2024-08-31T15:24:00Z

07.ls_object/main.rb

+require 'io/console'
+require_relative 'ls'
+
+LS_SHORT_WIDTH = IO.console.winsize[1]


これは何の値を取って LS_SHORT_WIDTHとはどういう意味でしょうか？

こちらについてですが、画面のサイズを取得してshort_formatで出力する際の最大幅を取得しています

なるほど、shortフォーマット用ということですね。
どちらかというと、「何に使うか？」よりも「何が入っているか？」の方がより気になるところなので画面のサイズという意味の変数の方が良いでしょう。

また、この値は環境によって違うので定数ではなく変数の方が良いですね。

u1tnk · 2024-08-31T15:25:32Z

07.ls_object/ls.rb

+
+  private
+
+  def parse_paths_and_options


メソッドはシンプルにするのが原則なので、必要が無ければ分けましょう。

こちらについて1点質問です。
parse_optionsとparse_pathsに分ける場合、parse_options⇒parse_pathsの順番で実行されない場合挙動がおかしくなってしまう認識です(オプションあり、引数無しの場合に何も表示されないなど)。
この場合でもメソッドを分割したほうがよろしかったでしょうか

確かにそうですね。ここはメソッドにしてない人が多かったのでいつも気にしておらず、長過ぎるメソッド名を見て指摘した感じですね。

理想としては parse_pathsでオプションにあたるものを無視するようにするあたりですが、今の状況で順番が大事である旨をコメントしておくぐらいで良いと思います。

あとオススメするわけじゃないですが parse_input ぐらいの名前だったらスルーしてしまったかもしれません 😓
長い名前で and を付けてると「このメソッド分けられそう！」って思っちゃうので。

u1tnk · 2024-08-31T15:27:46Z

07.ls_object/ls.rb

+    paths, options = parse_paths_and_options
+
+    @command = if options[:long_format]
+                 LsLong.new(paths, options)


LongとShortは出力の形式が違うだけなのでLsCommand自体を継承した二つに分けるのはちょっとおかしいですね。

u1tnk · 2024-08-31T15:43:06Z

07.ls_object/file_entries_list.rb

+class FileEntriesList
+  include EntriesList
+
+  attr_accessor :entries


このaccessorは使って無いように見えますがどうでしょうか？

u1tnk · 2024-08-31T15:44:48Z

07.ls_object/entries_list.rb

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+module EntriesList


名前が Entriesの時点で rubyの慣習上、複数系は単数名のクラスの配列もしくは配列を保持するクラスになるので、Listは不要です。

u1tnk · 2024-08-31T15:46:52Z

07.ls_object/dir_entry.rb

+require_relative 'entries_list'
+
+class DirEntry
+  include EntriesList


DirEntryと FileEntriesList は両方とも file_entriesを持っていますが、どのように使い分けるのでしょうか？

こちらに関してですが、lsコマンドで引数にファイルを直接指定した場合と、ディレクトリを指定した場合でそれぞれ異なる挙動を取っていたため、ファイルを直接指定した場合のものをまとめて扱うクラスをFilleEntriesListとして、ディレクトリを扱うものをDirEntryとしました。

例：

なるほど！
「任意のパスを指定できる」仕様は現状必須では無いはず
( https://bootcamp.fjord.jp/pages/ls-command で確認しました)
なのでいったん外してしまいましょう。

そのまま無しでもプラクティス的には良いですし、やるにしてもいったん外してオブジェクト指向的に整理してから追加した方が早そうですし。

u1tnk · 2024-09-04T16:04:45Z

07.ls_object/file_entry.rb

+  'directory' => 'd',
+  'file' => '-',
+  'link' => 'l'
+}.tap { |h| h.default = '-' }.freeze


この defaultのところは何をしてるのでしょうか？

こちらに指摘いただいたのですが、3通り以外のファイルタイプに対応しておらず、3通り以外のものを指定されたときに'-'を表示するように設定いたしました。
#11 (comment)

こちらせっかくなので下記の記事を参考に他のファイルタイプについても対応いたしました。
https://zenn.dev/universato/articles/20201202-z-mode

なるほど。了解です。

u1tnk · 2024-09-04T16:07:25Z

07.ls_object/file_entry.rb

+    # File::statのブロックサイズの単位は512bytesであるから変換する
+    block_size = stats.blocks * (512 / BLOCK_SIZE.to_f)
+
+    { type_and_mode: mode, nlink:, user:, group:, size:, mtime:, name:,


こうやって計算結果をhashに詰めて外部から参照できるんであればこのhashを作るメソッドと変わりません。

このFileEntryクラスに modeは？ userは？と聞いて返ってくるようにしましょう。

u1tnk · 2024-09-17T16:28:09Z

07.ls_object/file_entry.rb

+
+require 'etc'
+
+FILE_TYPE_LOOKUP = {


解決するための変数名ですが、LOOKUPだとちょっと違和感があります、意味的にはFOR_LOOKUPならわかるけど、ちょっと冗長だし、 FILE_TYPESとかSHORT_FILE_TYPESとかかなと思います。

u1tnk · 2024-09-17T16:28:55Z

07.ls_object/file_entry.rb

+
+  def initialize(path)
+    @path = path
+    @stats = File.lstat(@path)


細かいけど、メソッドもstatだし statsだと配列かな？って思っちゃうので @stat の方が良いかと思います。

u1tnk · 2024-09-17T16:30:01Z

07.ls_object/file_entry.rb

@@ -0,0 +1,62 @@
+# frozen_string_literal: true


オブジェクトっぽくなりましたね！ 👍

u1tnk · 2024-09-17T16:39:24Z

07.ls_object/ls.rb

+require 'optparse'
+require_relative 'ls_command'
+
+class Ls


このクラスはちょっと冗長かと思います。LsCommandかmainでやれば良いと思います。

u1tnk · 2024-09-17T16:40:46Z

07.ls_object/ls_command.rb

+  def initialize(paths, options)
+    @paths = paths
+    @options = options
+    @format = @options[:long_format] ? LsLong.new : LsShort.new


formatだとフォーマットの名前が入っていそうです。 formatするオブジェクトを入れるなら formatter の方が良いですね。
LsLong/LsShort も Formatterを最後に付けた方が良いですね。またついですが、 Lsは冗長なのでLongFormatter/ShortFormatterで良いと思います。

u1tnk · 2024-09-17T16:42:16Z

07.ls_object/ls_short.rb

+
+require 'io/console'
+class LsShort
+  def initialize(window_width = IO.console.winsize[1])


デフォルト値を外から渡すケースは無いと思うので、引数を無くして、initialize内で設定すれば良いと思います。

u1tnk · 2024-09-17T16:43:46Z

07.ls_object/ls_command.rb

+  def run
+    output = @directories.map do |dir_entry|
+      header = "#{dir_entry.path}:" if @directories.count > 1
+      body = @format.run(dir_entry).to_s


formatするオブジェクトが runして戻ってきたものを to_s する必要は無いはずです。

kutimiti1234 added 17 commits August 12, 2024 23:22

add:ls -lの実装

ace2983

clean:不要な条件を削除

c900f9f

clean:LsLongとLsShortの共通の振る舞いを持つ親クラスLsCommandを設定

5436be0

clean:冗長なパス指定を削除

50fed16

clean:適切なメソッド名へ変更

dbf3857

clean:render機能をentry側に移管

27a620f

add:dirクラスにデフォルトのlsコマンド機能を実装

b9b39c4

add:filelistクラスにデフォルトのlsコマンド機能を実装

c543bc7

add:lsコマンドの制御用クラスを作成。main.rbも整備

71d9b2e

add:Dirクラスの実態がEntryクラスよりもFileEntriesListの機能に似ているため、Entryクラスを廃した。また、Di…

fa552f4

…rクラスとFileEntriesListクラスの機能がほぼ同じのため、EntriesListモジュールを作成し、共通化

clean:誤ったパスを設定した際に気づきやすいように

4627718

add:reverseオプションを実装

56e63a2

clean:mainメソッドの表示の幅サイズを画面に

e24e0d4

fix:引数なしの場合に何も表示されない事象を修正

4206ffe

clean:インスタンス変数にする必要のないものを修正

6412c37

clean:わかりやすい変数名に変更

88144b9

clean:不要な再代入を削除

36864e4

u1tnk suggested changes Aug 31, 2024

View reviewed changes

kutimiti1234 added 4 commits September 3, 2024 01:24

clean:メソッドを機能ごとに分割

801e75b

fix:メソッド分割によって、parse_optionsを先に実行しないと、引数なしかつオプションありの場合に対応できないため

0385b41

clean:formatが異なるだけのls_shortとls_longのクラスを統合

f15930a

clean:不要なアクセサメソッドを削除

9c213ff

kutimiti1234 force-pushed the my-ls_object branch from cdf1a29 to 9c213ff Compare September 2, 2024 17:43

clean:entriesのみでクラスの集合であることを表すことができるため

be4239e

u1tnk reviewed Sep 4, 2024

View reviewed changes

delete:一旦、fileを直接指定した場合の挙動を削除

3fa75b5

kutimiti1234 force-pushed the my-ls_object branch from cd1e3c0 to 3fa75b5 Compare September 10, 2024 13:53

kutimiti1234 added 2 commits September 11, 2024 00:49

clean:file_entryクラスをstatsを直接扱わないように設定

f3a0dc4

clean:Rubocop

80e990c

kutimiti1234 added 21 commits September 16, 2024 16:21

clean:Rubocop

43bcfb5

clean:重複した処理を簡潔に

aa76923

clean:LsShortの定数の定義場所を移動したため

e311f64

clean:それぞれのファイルタイプに対応

ba17355

add:複数のディレクトリを指定した場合に対応

bdedfb3

fix:LsCommandクラスの修正に伴ってテストケースも変更

29ee691

fix:LsShortクラスの追加に伴ってテストケースも変更

3d76d92

clean:不要な条件節を削除

13f08d5

clean:クラス名の省略を避けた

99e5208

fix:ファイルの日時のフォーマットが適用されていなかったため

dd95986

clean:クラス名をDirectoryEntryに統一したため

001a21b

fix:tapは実行した際のレシーバを返すため、yield_selfで対応

e34c448

clean:不要な処理を削除

04b78bf

clean:無駄な行を削除

8e3869a

clean:クラス外で呼び出されないメソッドのため

ee0a1de

clean:Rubocop

3a434d6

clean:評判のよくないメソッド名なので、別のエイリアスを使う

9f901b8

clean:不要な変数への代入を省略

0cec18e

clean;端末の環境によって画面サイズは異なるため定数から変更

2d88508

clean:Rubocop

03a5d77

clean:より分かりやすい変数名に変更

6ac6c30

u1tnk suggested changes Sep 17, 2024

View reviewed changes

kutimiti1234 added 7 commits September 18, 2024 18:34

clean:変数名が冗長だったため変更

ffaa759

clean:変数名をメソッド側に統一

24b7f08

clean:クラス内部で閉じている変数のため、initializeメソッドの引数から除外

b4bd436

clean:クラス名を実態に合った名前に

8afe3b4

clean:冗長なクラス設計を排除

1c70bb5

clean:クラス設計の変更を受けてテスト内容を変更

669dd28

clean:テストファイルの名前を間違えていたたため

4d1c8a2

u1tnk approved these changes Sep 18, 2024

View reviewed changes

		@@ -0,0 +1,27 @@
		# frozen_string_literal: true

		module EntriesList


		require 'etc'

		FILE_TYPE_LOOKUP = {

Conversation

kutimiti1234 commented Aug 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

u1tnk Sep 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

u1tnk Sep 4, 2024 •

edited

Loading