Skip to content

Conversation

@SuperHotDogCat
Copy link
Owner

@SuperHotDogCat
Copy link
Owner Author

無駄なファイルはmerge先のbranchをremoteに反映してなかったことから生じていそうなので次からは無くなるはずです...

Copy link

@nodchip nodchip left a comment

Choose a reason for hiding this comment

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

よいと思います。

"""

# ループ不変式, nums[0], nums[1], nums[2], ..., nums[non_zero_index-1]までが0ではない
class Solution:
Copy link

Choose a reason for hiding this comment

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

自分がこの問題を読んだとき、 C++ の標準ライブラリの std::remove() を連想しました。おそらく std::remove() を自力で書けるかどうかが主題なのではないかと思います。

Copy link
Owner Author

Choose a reason for hiding this comment

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

確かに似ています。ありがとうございます。

"""
Do not return anything, modify nums in-place instead.
"""
non_zero_index = 0
Copy link

Choose a reason for hiding this comment

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

これちょっと名前が、nums[non_zero_index] != 0 に見えません?
実際に言いたいことは、non_zeros_length か count くらいでは。

Copy link
Owner Author

Choose a reason for hiding this comment

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

確かにこの名前だとnon_zero_indexが指しているindexの数は0でないときもあれば0の時もあるのでおかしいですね, 修正します

class Solution {
public:
void moveZeroes(vector<int>& nums) {
auto result = nums.begin();

Choose a reason for hiding this comment

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

resultというより、次にnon-zeroの値を入れるiterぐらいの意味ですかね。

public:
void moveZeroes(vector<int>& nums) {
auto result = nums.begin();
auto last = nums.end();

Choose a reason for hiding this comment

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

変数を作らない方が分かりやすいと思います。

void moveZeroes(vector<int>& nums) {
auto result = nums.begin();
auto last = nums.end();
int non_zero_length = 0;

Choose a reason for hiding this comment

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

distance(nums.begin(), result) == non_zero_lengthなので、不要な気がします。

auto result = nums.begin();
auto last = nums.end();
int non_zero_length = 0;
for (auto first = nums.begin();first != last; first++){

Choose a reason for hiding this comment

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

一つずつ要素を見ているだけなので、firstよりitの方が分かりやすい気がします。ここでは値を順番に見ているだけなので、イテレータを使わなくて良さそうです。

for (int num : nums) { ... }

auto last = nums.end();
int non_zero_length = 0;
for (auto first = nums.begin();first != last; first++){
if (!(*first == 0)){

Choose a reason for hiding this comment

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

普通に*first != 0で良さそうです。

int non_zero_length = 0;
for (auto first = nums.begin();first != last; first++){
if (!(*first == 0)){
*result++ = std::move(*first);

Choose a reason for hiding this comment

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

intでstd::move()をしても意味は無さそうです。

non_zero_length++;
}
}
for (; non_zero_length < nums.size(); non_zero_length++){

Choose a reason for hiding this comment

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

fill(result, nums.end(), 0);

Copy link
Owner Author

Choose a reason for hiding this comment

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

C++に詳しくないのでここまでのレビューとても参考になりました。ありがとうございます

@rihib rihib mentioned this pull request Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants