Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions pullrequests/move_zeroes/step1.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
//lint:file-ignore U1000 Ignore all unused code
package movezeroes

/*
レビュワーの方へ:
- このコードは既にGoの標準のフォーマッタで整形済みです。演算子の周りにスペースがあったりなかったりしますが、これはGoのフォーマッタによるもので、優先順位の高い演算子の周りにはスペースが入らず、低い演算子の周りには入るようになっています。https://qiita.com/tchssk/items/77030b4271cd192d0347
*/

/*
左側に非ゼロのエリア、右側にゼロのエリアを作れば良い。
イメージとしてはzeroIndexはゼロのエリアのはじまり(境界)のインデックスを持ち、
for文で探索していって非ゼロの要素を見つけたらzeroIndexの要素とスワップして、
zeroIndexをインクリメントすることで、zeroIndexの左側に非ゼロの要素を集め、
右側にゼロを集めるという感じ。

境界というのを明確にしたいのであればstartZeroIndexとかでもいいかも。またはzeroIndexだとインデックスの値自体が0なのかという誤解を引き起こす恐れがあるので、indexOfStartZeroIndexとかでもいいかもしれない。ただ全体の行数に対して変数名が冗長すぎる気がしたので今回はzeroIndexにした。

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loop内でzeroIndexが0以外を指していることもあるので、役割と名前があってなさそうです。

あと、zeroIndexは境界の役割というコメントがありましたが、zeroIndexの動きを言葉で表現しようとすると境界よりも複雑なことをしてて、fhiyoのレビューのように、コメントで補足してもらうとわかりやすいと思いました

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

最初の0が見つかるまでの間はzeroIndex == iなのでその間はスワップ自体はしていますが自分自身をスワップしているだけで、結果的には単にzeroIndexとiをそれぞれインクリメントしているだけなので何もしていないようなものかなと思っています。

最初の0が見つかったあとは0がだんだん後ろに流れていくという動きになります。
例(太字がzeroIndexの場所):[1, 0, 0, 2, 3, 4]→[1, 2, 0, 0, 3, 4]→[1, 2, 3, 0, 0, 4]→[1, 2, 3, 4, 0, 0]

そういう意味でzeroIndexがゼロ値を指していて、非ゼロとゼロの境界になると説明した感じです。なのでそこまでzeroIndex自体は複雑なことはやっていないかなと思っているのですが、最初のゼロが見つかるまでの動きについては確かにちょっと混乱を招くので、コメントをつけた方がいいかなと思いました。

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

私の気持ちとしては、これは配列とサイズの組み合わせで追記をしている(vector push_back の動作)ものの変形と感じるので。「いくつ非ゼロが見つかったか」というように見ています。

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

確かにそう考えると最初の0が見つかるまでの間の挙動もわかりやすくなりそうですね

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

そうすると、あとから0を埋めるというのも一つです。
C++ の remove_if と fill という組み合わせですね。

速度はどっちが速いか分からないです。複雑な書き込みのほうが遅いかも?

*/
func moveZeroes(nums []int) {
zeroIndex := 0
for i, n := range nums {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zeroIndexという名前で何を表す変数なのか(indexである以上のことが)分からない気はしました。
iとかjとかにしちゃって、コメントで補足するほうが読みやすいと思いました
(個人的にこの問題はループ不変条件が何かを意識するのが大事な気がしており、今回であればzeroIndexより左と、zeroIndexからiまでが各ループの終わりで何が入っているか書いてあると分かりやすいかなと思います)

if n == 0 {
continue
}
nums[zeroIndex], nums[i] = nums[i], nums[zeroIndex]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

これ C++ だと zeroIndex == i の場合は未定義動作にあたるかと思いますが、Go では大丈夫でしょうか。

この質問は、本当に不安に思っているというよりは、言語仕様を調べたことがありますか、それとも漫然と経験上書いていますか、という質問です。

私は練習の仕方は自由度があってよいと思っていますが、参考までに練習の目的については下のように思っています。
https://docs.google.com/document/d/1bjbOSs-Ac0G_cjVzJ2Qd8URoU_0BNirZ8utS3CUAeLE/edit?tab=t.0

Copy link
Owner Author

@rihib rihib Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ありがとうございます。多重代入については以前に調べたことがあり、この場合は左辺と右辺の値が評価されてから代入が行われると書かれているため(https://go.dev/ref/spec#Assignment_statements )、問題ないという認識を持っています。

ただしマニュアルを読みたくなるという反応がきちんと自分の中に備わっているかどうかは微妙な部分があるので、そういった点は気をつけながら取り組んでいかないとなと思います。

zeroIndex++
}
}