lsコマンド2の提出 #7
lsコマンド2の提出 #7
Conversation
05.ls/myls.rb
Outdated
| def height | ||
| (current_element.size / DISPLAY_NUMBER) + 1 | ||
| def main | ||
| acquired_elements = current_element |
There was a problem hiding this comment.
この代入は不要そうですね〜すぐ下、modified_elementsの引数としてcurrent＿elementを呼べばよいと思います。
| end | ||
|
|
||
| def string_count | ||
| current_element.map(&:length).max | ||
| def current_element |
There was a problem hiding this comment.
この処理、やろうと思えばmainの中にベタ書きすることもできるわけですが、そうせずに処理をちゃんとメソッド化することで名前がついて、コードの見通しがよくなってますね。これはgoodです！
|
@kfukai23 さん コードを修正しました！アドバイスをもとに修正してみました！お手隙の際に確認をお願いいたします🙇♂️ mainメソッド内での不必要な代入についてたしかに無駄なコードでした😅 作ったメソッドを実際に表示するためのmainメソッドをわざわざ作らないで、普通に表示させるほうがよかったかもしれませんが、伊藤さんのこのような記述をする記事をみつけて使ってみました！ Rubyスクリプトにもmainメソッドを定義するといいかも、という話 今回のような、短いコードではあまりのメリットはなかったかもしれませんね💦このような記述は、現場では使われるのでしょうか？ メソッドの作り方について
ありがとうございます！メソッドを使って可視性を高める用途でわけてみました😄 |
05.ls/myls.rb
Outdated
| height = acquired_elements.size / DISPLAY_NUMBER + 1 | ||
| string_count = acquired_elements.map(&:length).max | ||
| sliced = acquired_elements.map { |d| d.ljust(string_count) }.each_slice(height).to_a | ||
| def modified_element(element) |
There was a problem hiding this comment.
お、メソッド側の引数名も変更されたのですね:eye:
それ自体は良いと思いますが、配列が来るので単数形ではなく複数形のほうが良さそうに思います（実際中身が0~1つなこともありますが、単数形だと中身が配列であるとはイメージしづらいので）。
There was a problem hiding this comment.
@kfukai23 さん
複数の場合、0〜1の場合もあるので少し迷ったところでした！複数入る場合があるので、それを理解できる複数形のほうがよいですね🧐早速修正しました🔥
現場では単体のRubyスクリプトを書くことがそこまで多くないので、mainというメソッドを作っているのはあまり見ませんね。 |
|
@kfukai23 さん
ありがとうございます！！
メソッド内の引数名を複数形に修正しました。お忙しい中ですが、お手隙の際に確認お願いします🙇♂️ |
お世話になっております！
表題のプラクティスの課題を提出させていただきます🙇♂️ご確認のほど、よろしくお願いいたします！