-
Notifications
You must be signed in to change notification settings - Fork 0
Solved: 78. Subsets #53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| subset.append(nums[index]) | ||
| bitmask >>= 1 | ||
| index += 1 | ||
| return subset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bitmask を切り詰めるのと index を増やすのを同時にやるより、以下のように index のみを動かす書き方もシンプルでありかなと思いました。
あとこの関数の返り値の型は List[int] ではないでしょうか。
def create_subset_from_bitmask(bitmask: int) -> List[int]:
subset = []
for index, num in enumerate(nums):
if bitmask & (1 << index) != 0:
subset.append(num)
return subsetThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
レビューありがとうございます。
確かに if bitmask & (1 << index) != 0: という形はよく見かける気がしますし、シンプルでありですね。
あとこの関数の返り値の型は List[int] ではないでしょうか。
ご指摘の通りです。見逃してました。
| return subset | ||
|
|
||
| all_subsets = [] | ||
| for i in range(2**len(nums)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
いいと思います。一応**の両隣はスペースを開ける方が一般的でしょうか。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
レビューありがとうございます。
おっしゃる通りですね。
なんとなく累乗に空白を空けていない例をみた記憶があったので調べてみたのですが、
pep8的には、加算、減算と乗算、除算があるような式で、計算の優先度がわかるようにする考えがあるようですね。
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:
| select_index(i + 1) | ||
|
|
||
| subsets_list = [] | ||
| is_selected = [False] * len(nums) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_selected を使用する inner function よりあとで定義すると、 inner function を読む際に、変数の意味を知るために定義が書かれている箇所を探さなければならず、余分な目線の移動が発生します。これは読み手に取って思考のノイズになる場合があります。 inner function 内で使用する変数は、 inner function より前で定義するとよいと思います。
| select_index(i + 1) | ||
|
|
||
| subsets_list = [] | ||
| is_selected = [False] * len(nums) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_selected という変数名は、 be 動詞から始まっているせいで、関数名に見えます。 selected はいかがでしょうか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
レビューありがとうございます。
be 動詞から始まっているせいで、関数名に見えます。
確かにおっしゃる通りですね。selectedで十分伝わると思いました。
78. Subsets