Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions grind75_hard/11_895. Maximum Frequency Stack/level_1.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# 回数ごとに違うstackを作る
class FreqStack:

def __init__(self):
self.freq = defaultdict(int)
self.stack = [[]]
self.max_freq = 0
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

レビュー前に自分で解いてみたときには self.max_freq を持たせるという発想がなかったので、なるほどと思いました。(ただのコメントです。)

(自分で解いたときは Step1 と同様に list で扱っていて、self.stack の長さをコントロールすることで list の右端がここでいう self.max_freq に対応するようにしていて、常に self.stack[-1] を参照するようにしていました)


def push(self, val: int) -> None:
self.freq[val] += 1
if len(self.stack) < self.freq[val]:
self.stack.append([])
self.max_freq = max(self.max_freq, self.freq[val])
self.stack[self.freq[val] - 1].append(val)
Comment on lines +9 to +14
Copy link
Copy Markdown

@thonda28 thonda28 Jun 11, 2024

Choose a reason for hiding this comment

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

L.14 で self.freq[val] - 1 のように1つずらしてインデックスを指定するのは少しわかりづらいので、1つずらさなくても一致するように工夫するとよさそうです。具体的には L.11 の if 文で < から <= に条件を微修正するだけで対応できそうです。(そうすると pop() でも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.

おっしゃるとおり、self.freq[val]をそのままインデックスにしたほうが分かりやすいですね。
stack[0]のlistが使われないのを気にしましたが、分かりやすさを優先したいのは納得感があります。


def pop(self) -> int:
val = self.stack[self.max_freq - 1].pop()
self.freq[val] -= 1
if not self.stack[self.max_freq - 1]:
self.max_freq -= 1
return val
21 changes: 21 additions & 0 deletions grind75_hard/11_895. Maximum Frequency Stack/level_2.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# stacksをdefaultdict(list)を使うことで空の場合の確認を不要にした。
class FreqStack:

def __init__(self):
self.frequency = defaultdict(int)
self.stacks = defaultdict(list)
self.max_frequency = 0

def push(self, val: int) -> None:
self.frequency[val] += 1
self.max_frequency = max(self.max_frequency, self.frequency[val])
self.stacks[self.frequency[val]].append(val)

def pop(self) -> int:
if not self.stacks[self.max_frequency]:
return None
top_value = self.stacks[self.max_frequency].pop()
self.frequency[val] -= 1
while self.max_frequency != 0 and not self.stacks[self.max_frequency]:
self.max_frequency -= 1
Comment on lines +19 to +20
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

この部分は while 文でなく if 文でも十分だったりしませんかね?(push() で1つずつ積み上がるので、連続でself.max_frequency -= 1 が起きることはないのではと思いました。)

また条件としては後者のみで十分なように思いました。前者が false となるのは self.max_frequency = 0 のときのみで、そのケースはそもそも不適切なケースなので pop() を呼び出したタイミング(L.15 の Valiadtion 部分)で処理するのがシンプルに思いました。

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

追加で L.15 の validation ももしかしたら不要かも?と思ってきました。(上で書いた self.max_frequency = 0 の考慮だけあれば十分?)

self.max_frequency はこのクラス内の push()pop() でのみ操作がされるインスタンス変数で、外から与えられる引数とは違ってこちらが完全に管理しているものかと思います。そのため self.stacks[self.max_frequency] が空であるケースはそもそもロジックが誤っているので正常に None を返すのではなく、例外を投げるのが適切かと思いました。例外を投げるのであれば if 文での判定がなくても self.stacks[self.max_frequency].pop() が IndexError を投げるので(特別な例外を投げたいケース以外では)L.15 の if 文は不要かと思いました。

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.

(push() で1つずつ積み上がるので、連続でself.max_frequency -= 1 が起きることはないのではと思いました。)

そのことをコードの読み手が考えるのは辛いかなと思いましたが、自明かもしれませんね。
stackの中身があるところをまで下っていくという意図を表すためにwhileにしましたが、逆に読みづらくなってるということですね。

空に対するpopの例外処理についてのコメントもありがとうございます。

Copy link
Copy Markdown

@thonda28 thonda28 Jun 12, 2024

Choose a reason for hiding this comment

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

そうですね、コードを読んだときに while を使っている理由がなにかあるんじゃないかと考えてしまったので、不要な付加情報があるとむしろ読みづらい(または誤解を招きやすい)んじゃないかと思いました。

例外処理に関してはあくまで僕個人の意見なので、他の方の意見も気になるところです。

return top_value
18 changes: 18 additions & 0 deletions grind75_hard/11_895. Maximum Frequency Stack/level_3.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
class FreqStack:

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
Owner Author

@shining-ai shining-ai Jun 11, 2024

Choose a reason for hiding this comment

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

pep8とgoogleのコーディング規約を再度確認しました。
開ける必要ないですね。
https://peps.python.org/pep-0008/#blank-lines
https://google.github.io/styleguide/pyguide.html (3.5 Blank Lines)

def __init__(self):
self.frequency = defaultdict(int)
self.stacks = defaultdict(list)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

stacks は、下から整数が入っているはずなので、これ、リストでもいいですかね。ちょっと書くのが面倒になりますか。

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

あ、step 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しか思いつかず、リファクタリングしててdefaultdictに気づきました。

self.max_frequency = 0

def push(self, val: int) -> None:
self.frequency[val] += 1
self.stacks[self.frequency[val]].append(val)
self.max_frequency = max(self.max_frequency, self.frequency[val])

def pop(self) -> int:
value = self.stacks[self.max_frequency].pop()
self.frequency[value] -= 1
while self.max_frequency != 0 and not self.stacks[self.max_frequency]:
self.max_frequency -= 1
return value