Conversation
| for right in range(len(nums) - 1, left, -1): | ||
| if nums[left] < nums[right]: | ||
| return (left, right) | ||
| return (-1, -1) |
There was a problem hiding this comment.
(-1, -1) のような、説明のない値、特別な値を返している点が気になりました。このような値を使って関数の処理結果を処理しようとした場合、書き間違いによってバグを生む可能性が高くなります。関数の処理が正しく行われたかどうかを、別の値を用いて返すのが良いと思います。
例:
成功した場合: (True, left, right)
失敗した場合: (False)
成功した場合 (left, right)
失敗した場合 ()
There was a problem hiding this comment.
コメントありがとうございます。返り値の数が違うのもバグを生みそうな気がするのですがどうでしょうか。indexを返す関数なので（←これが分かりづらければ関数名の検討の余地あり）、説明がなくても-1が返ってくれば無効な値なんだなというのは分かるような気がします。
There was a problem hiding this comment.
他の人がこの関数を使うことを考えた場合、ソースコードコメントで -1 が返ることを記述しても、注意不足で -1 に関する処理を忘れることが考えられます。これを避けるため、関数の成功失敗を表す値を別途返すことをお勧めいたします。
There was a problem hiding this comment.
2ndの2つ目の関数についても同様ですか？仰ってることは分かるのですがちょっとやり過ぎ感もある気がするんですよね。
def find_first_not_ascending_from_right(nums):
for i in range(len(nums) - 2, -1, -1):
if nums[i] < nums[i + 1]:
return i
return -1There was a problem hiding this comment.
別の方法として、 INVALID_INDEX という定数を定義しておき、それを返すという方法もあると思います。これですと、 -1 といった説明のないマジックナンバーを返すより、ある程度ダメージを押さえられると思います。
There was a problem hiding this comment.
結局はチームやプロダクト内でのルールにもよるんですかね？プロダクトが成功、失敗を別に返すルールで運用されていればそれに従うのは構わないんですけど、LeetCodeで今回のような問題を解くとすると-1を返す方が好みです。
| def permute(self, nums: List[int]) -> List[List[int]]: | ||
| def find_first_not_acending_pair_from_right(nums): | ||
| right = len(nums) - 1 | ||
| index = len(nums) - 2 |
There was a problem hiding this comment.
ありがとうございます。色々修正を加えていった結果不要な変数が残ってしまっていました。
| def find_first_not_acending_pair_from_right(nums): | ||
| right = len(nums) - 1 | ||
| index = len(nums) - 2 | ||
| for left in range(len(nums) - 2, -1, -1): |
There was a problem hiding this comment.
この部分の処理が直感的に正しいと感じられませんでした。 2 重ループはいらないのではないでしょうか？
for index in range(len(nums - 2, -1, -1):
if nums[index] < nums[index + 1]:
return (index, index + 1)
さらにいえば、隣り合った要素のインデックスを返しているのであれば、左のインデックスだけ返せば十分だと思いました。
for index in range(len(nums - 2, -1, -1):
if nums[index] < nums[index + 1]:
return index
There was a problem hiding this comment.
コードが整理されているかはともかく処理としては正しいと思います。
この関数内では以下の2つのことを同時に行っています。
- 右側からみて昇順になっていない値を見つける。これをaとする
- aより右にあり、aより大きい最小の値を見つける。これをbとする
例）[2, 5, 4, 3, 1]があった場合に(0, 3)を返しています。
その後の処理で、0と3を交換して[3,5,4,2,1]となりindex 0の右から末尾までreverseして[3,1,2,4,5]になります。
There was a problem hiding this comment.
ありがとうございます。正しそうです。
他のソフトウェアエンジニアから見て、処理の内容をすぐに理解できなさそうなコードについては、ソースコードコメントで補足説明を加えるとよいと思います。
2 participants
https://leetcode.com/problems/permutations/
#5 で解いているが解き直し