Skip to content

1. Two Sum#14

Open
fhiyo wants to merge 1 commit intomainfrom
1_two-sum
Open

1. Two Sum#14
fhiyo wants to merge 1 commit intomainfrom
1_two-sum

Conversation

@fhiyo
Copy link
Copy Markdown
Owner

@fhiyo fhiyo commented Jun 1, 2024

Copy link
Copy Markdown

@torotoki torotoki left a comment

Choose a reason for hiding this comment

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

コメントしてみました。全体的に良かったと思います。(なのでコメントは細かい点かもしれません)


問題設定上 `return []` には到達しない。例外を返すか異常終了させるかも迷ったがこうしてみた。使われ方によって選択を変えるだろう。

remainingじゃなくてrestの方がより合ってる?そもそも形容詞だから微妙か。discord上の議論を見てcomplementもいいかもと思った。
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

restは名詞でも使うので変じゃないと思います: https://eow.alc.co.jp/search?q=rest

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ちなみに私はexceptionを返すのが好みですが、ここは本当にユースケースによりますよね

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.

remainingじゃなくてrestの方がより合ってる?そもそも形容詞だから微妙か

これremainingについての言及でした (分かりづらい)

def twoSum(self, nums: List[int], target: int) -> List[int]:
for i in range(len(nums)):
for j in range(i+1, len(nums)):
if nums[i]+nums[j] == target:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

pep8を把握しているなら全然問題ないと思いますが、syntax checkerを回すとnums[i] + nums[j]に直されそうなコードかもしれません

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.

https://peps.python.org/pep-0008/#other-recommendations

If operators with different priorities are used, consider adding whitespace around the operators with the lowest priority(ies). Use your own judgment; however, never use more than one space, and always have the same amount of whitespace on both sides of a binary operator:

# Correct:
i = i + 1
submitted += 1
x = x*2 - 1
hypot2 = x*x + y*y
c = (a+b) * (a-b)
# Wrong:
i=i+1
submitted +=1
x = x * 2 - 1
hypot2 = x * x + y * y
c = (a + b) * (a - b)

いまいちこのルールよく分かってないんですが、多分どちらでも良いんだろうという解釈をしています (実際の開発ではformatterに従うだけなので良いですが)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Google Style Guideの方はこう書いてありますね。
https://google.github.io/styleguide/pyguide.html#36-whitespace

Surround binary operators with a single space on either side for assignment (=), comparisons (==, <, >, !=, <>, <=, >=, in, not in, is, is not), and Booleans (and, or, not). Use your better judgment for the insertion of spaces around arithmetic operators (+, -, *, /, //, %, **, @).

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.

ありがとうございます。Google Style Guideも算術演算子周りのスペースは決められてないんですね

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

C++ が全部開けるので私はそっちの影響を受けています。

if remaining in num_to_index:
return [num_to_index[remaining], i]
num_to_index[num] = i
return [] # never reached
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, -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.

これ返り値の型と合わなくなるんですか?空のリストもList[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.

すみません、変なことを言っているかもしれません。
List[Optional[int]]なら良いのかなと思っていたのですが、[] ∈ List[int]でしたっけ。

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.

Optional[int] は int | None (https://docs.python.org/3/library/typing.html#typing.Optional) なので、たとえば [1, None] みたいなやつだと思います。

[] ∈ List[int] でしたっけ。

mypyとかでチェックされてみるといいかもしれないですね

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

これで問題なさそうですね。失礼しました。
mypyでも試してみました。

@rihib rihib mentioned this pull request Aug 6, 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