はじめに
みなさん、こんにちは torihaziです。
今日は何かのtipsというわけではなく振り返り回になります。
数回のコードレビューを通じて感じた自身の課題や習得したことを
つらつら書いていこうと思います。
それではltg
レビューを受けて
同じことを何度も書いている
こういうことをDRYともいうらしいですね。
Don't Repeat Yourself.
同じものを何度も書くな、という考え方です。
私はコードが書けたら嬉しさのあまりそのままPR投げてしまうので
終了したら少し時間をおいて見直してみることが大事だと感じました。
elseを書きがち
現職でシェルスクリプトを書いているためか、
去年を通じて学んだPHPでそう書くことが多かったためか
ことあるごとに if ~~ else ~~ endとelseを書いてしまいがちでした。
しかし、rubyでは raise ~~ unless ~~と書くことが可能です。
rubyに限らず、if のネストは最小限にして早期リターンすることを肝に銘じておこうと思いました。
if 条件式 trueの処理 else raise 'falseです' end
raise 'falseです' unless 条件式 trueの処理
1つの処理にまとめがちで何をする処理なのかわかりにくい
これはrubocopに怒られたことになります。
def ~ endの間の行数があまりに多いとダメなようです。
例えば 何かを買う処理を書こうとした時、私は次のようにして書いていました。
# arrayには買いたいものが入っている配列です。
def buy(name, num)
if array.include?(name) && num >= 1
arrayから1つ値を削除する
所持金が 買いたいものの値段分減った
売上が値段分増えた
else
raise '買えなかったよ'
end
end
この時最初のifで買えるかの判定をしていますが、ぱっと見何をしているかわかりません。
そのため新しくbuy?メソッドを作りその後unlessを使用して次のように書き換えました。
これが正解かどうかはわかりませんが、わかりやすくなったと思います。
def buy?(name, num) <= true or falseを返す関数
array.include?(name) && num >= 1
end
def buy (name, num)
raise '買えなかったよ' unless buy?(name, num)
arrayから1つ値を削除する
所持金が 買いたいものの値段分減った
売上が値段分増えた
end
今後は
- 行数をなるべく少なく
- 他人が見てもわかりやすく
- 1つの大きなものを作らず
- 複数の小さなものを組み合わせて
コーディングを進めていくことが必要だと感じました。
習得したこと
配列やハッシュを扱う課題であったため、少し前に比べたらレベルが上がったと思います。
ジムバッジ1つか2つはもらえたのではないでしょうか。
Array#each
配列の各要素を1つずつ取り出し、ブロック内の処理を評価することができるメソッドです。
評価した後は配列を返しません。ただ処理をするだけです。
[1, 2, 3].each do |i| puts i end #=> 1 # 2 # 3
Array#map
配列の各要素を1つずつ取り出し、ブロック内の処理を実行します。
そしてそのそれぞれの処理結果を要素とする新しい配列を返します。ここがeachと違うポイントです。
そのためここからさらに Array#firstやArrayselectを使用することもできます。
new_array = [1, 2, 3].map {|n| n * 3 }
p new_array
# => [3, 6, 9]
Array#select
配列の各要素を1つずつ取り出し、ブロック内の処理を実行します。
この時ブロック内の処理は 条件式を記載します。
selectはこの条件式がtrueとなった要素のみを各要素とする新しい配列を返します。
even_array = [1,2,3,4,5].select { |num| num.even? }
p even_array
# => [2, 4]
Array#uniq
与えた配列の中から重複している要素を削除し、その削除後の結果を新しい配列としてを返します。
duplicated_array = [1, 3, 2, 2, 3].uniq p duplicated_array # => [1, 3, 2]
Array#delete_if
配列の各要素を1つずつ取り出し、ブロック内の処理を実行します。
この時、ブロック内の処理は条件式を記載します。
delete_ifはこの条件式がtrueとなった要素のみ削除し、残った要素から構成される新しい配列を返します。
a = [0, 1, 2, 3, 4, 5].delete_if{|x| x % 2 == 0}
p a
#=> [1, 3, 5]
Array#any?
配列の各要素を1つずつ取り出し、ブロック内の処理を実行します。
この時、ブロック内の処理は条件式を記載します。
any?は条件式が要素全てでfalseとなった場合、falseを返します。
一方で途中でtrueが出た場合はそこでtrueを返し、処理を終了します。
p [1, 2, 3].any? {|v| v > 3 } # => false
Array#first
今回は first(n)を使用しました。
firstは作用する配列の最初の要素(indexが0)を返します。
first(n)とすると最初からn個分取り出して、取得結果から構成される配列を返します。
ary = [0, 1, 2] p ary.first(0) p ary.first(1) p ary.first(2) # => [] # [0] # [0, 1]
Array#size
配列の要素数を返します。
p [1, nil, 3, nil].size #=> 4
Object#eql?
作用させるオブジェクトの値と判別すべき値を同値であるか判断し、ture かfalseを返します。
p("foo".equal?("bar")) #=> false
p("foo".equal?("foo")) #=> false
p(4.equal?(4)) #=> true
p(4.equal?(4.0)) #=> false
Object#object_id
作用させたオブジェクトが持つ一意な整数を返します。 ただこちらについてはいつか痛い目を見るような気がします。 便利そうだと思って使っては見ましたが、どうなることやらです。
p "ruby".object_id #=> 60 p "ruby".object_id #=> 80 p [].object_id #=> 100 p [].object_id #=> 120 p :ruby.object_id #=> 710428 p :ruby.object_id #=> 710428
Numeric#positive?
作用させた値が正の数であるか判別し、trueかfalseで返します。
1.positive? # => true 0.positive? # => false -1.positive? # => false
Object#dup
作用させたオブジェクトの複製を返します。
これについてはあまり理解できていません。
同じインスタンスを作りたいが、別物として作りたかったので使用しました。
浅いコピーと深いコピーという概念があるようです。
いつか痛い目見ると思います。
その時はまた戻って来たいと思います。
array = ["red","blue","yellow"] p array.object_id d = array.dup c = array.clone p d.object_id p c.object_id # =>60 80 100
終わりに
初めて数回にわたるコードレビューというものをgithubを通じてしていただきました。
前スクールでは先生の方に恥ずかしくて見せることができず終わってしまったので
今回できてよかったと思います。
ただ動くだけではダメなようです。
難しいです。
ベタな表現ですが、最適化されたような気がします。
今後も受けっぱなしで終わりにせず、どこがダメでどこがよかったのか
毎度言語化してうやむやにしないようにしていきたいと思います。
その際、メソッドの説明(コードは一旦別)はマニュアルの丸パクリではなく
自分の言葉で説明することを絶対としていきます。
みなさんも頑張ってみてください。
僕も頑張ります。
