Skip to content

Next permutation#8

Open
SuperHotDogCat wants to merge 7 commits intoarai60from
next_permutation
Open

Next permutation#8
SuperHotDogCat wants to merge 7 commits intoarai60from
next_permutation

Conversation

@SuperHotDogCat
Copy link
Copy Markdown
Owner

@SuperHotDogCat
Copy link
Copy Markdown
Owner Author

あ、こちらを参考にしました https://cpprefjp.github.io/reference/algorithm/next_permutation.html

index -= 1
return index

if len(nums) == 1:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

長さ1は特別扱いが必要ですか?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

index = len(nums) - 2でindex = -1となるので不要でした

def _find_first_not_sorted_index(nums: List[int])->int:
# 後ろから走査し, nums[index] >= nums[index+1]ではなくなる初めてのindexを取り出す
index = len(nums) - 2
while index > -1 and nums[index] >= nums[index+1]:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

index > -1 よりindex >= 0の方が分かりやすい気がします。

index -= 1
return index

def _find_second_largest_index(nums: List[int], compare_index: int)->int:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

second largestは2番目に大きい数なので適切な表現ではないと思います。
rfind_bigger_than(num)なんかでどうでしょう。

numsは引数に含める必要はありません。


nums[right], nums[left] = nums[left], nums[right]

nums[left+1:] = sorted(nums[left+1:])
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

降順にソートされているので、reverseすればいいです。コピーするより、in-placeの方の関数を使った方がいいでしょう。

@@ -0,0 +1,35 @@
class Solution:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

pivot_indexswap_indexをコメントで定義して、使うのもありだと思います。


Reference:
https://github.com/shining-ai/leetcode/pull/58/files 解くことに夢中になりすぎて関数での分割を忘れていた。
nums[pivot_index+1:] = reversed(nums[pivot_index+1:])よりもnums[left + 1 :] = sorted(nums[left + 1 :])
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[]内の演算子の周りにスペースを入れるか入れないかはPEPではどうなっていますか?少なくとも一貫性のある書き方にはしてほしいです。

Copy link
Copy Markdown

@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.

phase5 良いと思います。

"""
Do not return anything, modify nums in-place instead.
"""
def _find_first_not_sorted_index() -> int:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

_rfind_bigger_thanに合わせるならこちら_rfind_first_not_sorted_indexじゃないですか?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

not_sortedよりnot_decreasingの方がより関数内の処理が具体化されると思います。


right = _rfind_bigger_than(left)
nums[left], nums[right] = nums[right], nums[left]
nums[left+1:].reverse()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

たぶんこの部分意図した挙動になっていないと思います。

Pythonのsliceは新しいリストを作ります。
https://www.reddit.com/r/Python/comments/rfm6ph/does_array_slicing_use_extra_memory/
https://github.com/python/cpython/blob/a62be77266b1beadd42d4952186332bc0847b7d6/Objects/listobject.c#L465

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

知らなかったです。あと多分leetcodeでテストしたつもりになっていましたが多分し忘れていました。今in-place処理にしたphase6を追加してみました。

right = _rfind_bigger_than(left)
nums[left], nums[right] = nums[right], nums[left]

swap_right = len(nums) - 1
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

関数に切り出しても良いですね。このままでも良いとは思います。

nums[swap_right], nums[swap_left] = nums[swap_left], nums[swap_right]
swap_left += 1
swap_right -= 1
return No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

最後のreturnはなくても挙動は変わらないですね。

index -= 1
return index

left = _rfind_first_not_decreasing_index()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

全体的にちょっと空行多めかしら。

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

19行目の空行だけ余分な気がします。23と26はあってもなくても良さそうな感じですかね。

@SuperHotDogCat
Copy link
Copy Markdown
Owner Author

空行に関して, トップレベルの関数やクラスは、2行ずつ空けて、クラス内部では、1行ずつ空けてメソッドを定義する。とあったのでそれに従ってやってみます

@SuperHotDogCat
Copy link
Copy Markdown
Owner Author

なんだかまだ空行の有無がよくわからない感じですね。
left = _rfind_first_not_decreasing_index()
if left == -1:
nums.reverse()
return

right = _rfind_bigger_than(left)
とphase7でも書きましたが, returnとrightのところで少しスペースを開けたくなりました。でも判断がとても主観的すぎる気がしています

@liquo-rice
Copy link
Copy Markdown

意味の塊ごとに適当に区切ればいいかと思いますが、例えばphase3.pyとかは多すぎるような気がします。

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