20. Valid Parentheses#11
Conversation
| {']', '['}, | ||
| }; | ||
| stack<char> opening_brackets; | ||
| opening_brackets.push('*'); // sentinel |
There was a problem hiding this comment.
確かに'\0'いいですね！ありがとうございます！
| if (auto it = closing_to_opening.find(c); | ||
| it != closing_to_opening.end()) { | ||
| auto opening = it->second; | ||
| if (!opening_brackets.empty() && opening_brackets.top() == opening) { | ||
| opening_brackets.pop(); | ||
| } else { | ||
| return false; | ||
| } | ||
| } else { | ||
| opening_brackets.push(c); | ||
| } |
There was a problem hiding this comment.
if else のネストが分かりにくいと私は思います。このあたりをざっと見ておくといいかもしれません。
https://docs.google.com/document/d/11HV35ADPo9QxJOpJQ24FcZvtvioli770WWdZZDaLOfg/edit?tab=t.0#heading=h.9kpbwslvv3yv
There was a problem hiding this comment.
auto itを同じブロックで処理することに気を取られてましたが別に条件式外にすればネストを解消できましたね...
ありがとうございます！
| // - なるほど。そういえばValid ParenthesesもCSZAPで触れていたかもしれない。 | ||
| // https://discord.com/channels/1084280443945353267/1201211204547383386/1202541275115425822 | ||
| // - そうか、closing_to_openingじゃなくてもopening_to_closingでもよかったのか… | ||
| // うーん、でもその場合2回map::findすることになるな |
There was a problem hiding this comment.
直感、あんまり速度が問題にならないと思うので、私はこれは気にしません。
ボトルネックでないところを最適化しない話がこのあたりに。
https://docs.google.com/document/d/11HV35ADPo9QxJOpJQ24FcZvtvioli770WWdZZDaLOfg/edit?tab=t.0#heading=h.agkea65vtdhp
| // - L49を空けるか否か | ||
| // - closing_to_openingとopening_bracketsのどちらを先に宣言するか |
There was a problem hiding this comment.
ありがとうございます！
なんか今更なんですが、色々こうやってレビューもらっているうちに、プログラミングの可読性の面は言語ゲームなんじゃないかと思い始めてきました。
またなんかすごいブレークスルーした気がします。
| // openはなくても伝わると思い消した。 | ||
| // - だが、読む側に取ってどう見えるかは分からない。 | ||
| // - correspondingみたいな意味の名前は付けることが多いと思うが、 | ||
| // もっと短めの単語はないものか。 |
| open_brackets.push(bracket); | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
私の好みとなり恐縮ですが、コメントを残されているので、この空白行はなくても読みやすく感じました。
| for (auto bracket : brackets) { | ||
| // In the case `bracket` is an open bracket. | ||
| if (kOpenToClose.contains(bracket)) { | ||
| open_brackets.push(bracket); |
There was a problem hiding this comment.
これは好みかなと思いますが、わかりやすさ（正しさの保証）でいうと、kOpenToClose.at()をここでやっている方が親切な気がしますね。（containsが必ず保証されていることがすぐわかる、kOpenToCloseをこの後で使わなくなるので構造的な見通しが良くなる）
ただ、エラーケースの計算量がちょっと減るので、シビアな計算のことを考えるならば計算をなるべくあとに回す習慣でも良いかもしれません。
| continue; | ||
| } | ||
|
|
||
| // In the case `bracket` is a close bracket. |
There was a problem hiding this comment.
この問題の前提となる入力の場合はここのコメント通りなんですが、他の文字が入った場合の関数の挙動としては厳密には違うので、入力に対する前提知識がないと微妙に誤った理解を与えるかもしれません。
// In the case where bracket should be a closing bracket
とかですかね。（英語でどうコメントするのがいいかわからなくて、ChatGPTに聞きました）
| class Solution { | ||
| public: | ||
| bool isValid(string s) { | ||
| static const map<char, char> closing_to_opening{ |
There was a problem hiding this comment.
関数内で static 変数を定義した場合、最初に関数が呼ばれたときに初期化されます。その際、競合状態を防ぐためにロックが取られます。結果、関数の呼び出し毎にロックが取られ、パフォーマンスが低下する場合があります。原則、関数の外で定義することをお勧めします。
There was a problem hiding this comment.
static 変数のロックは C++11 からそうなっているんですね。
https://cpprefjp.github.io/lang/cpp11/static_initialization_thread_safely.html
| open_brackets.pop(); | ||
| if (bracket != close_bracket) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
open_brackets.pop();はカッコの不一致を判定した後（L44 直後）に来た方が個人的には分かりやすく感じます。
「カッコの種類が一致したらポップ」という考えだと思うので、
「カッコの種類が一致しないかを判定し、それ以外の場合はポップ」
という流れの方が分かりやすいかなと思いました。
There was a problem hiding this comment.
close_bracketをわざわざ宣言せず
if (bracket != kOpenToClose.at(open_brackets.top())) {としても十分わかりやすいと思います。
kOpenToCloseの名前からOpenをキーとするとCloseが返ってくることが分かり、名前からopen_brackets.top()がOpenであることも分かるので、kOpenToClose.at(open_brackets.top())という形がCloseであることは分かりやすいと思うからです。
| // <時間> | ||
| // 10分 | ||
| // <感想> | ||
| // 多分3.cppよりこっちの方が読みやすい。 |
There was a problem hiding this comment.
同感です。OpenToCloseに入っているかを調べる方が直感的だと思いました。
自分の理解力の問題だと思いますが、CloseToOpenにない、と言われると逆の逆みたいに感じて理解しにくかったです。
8 participants
問題
https://leetcode.com/problems/valid-parentheses/description/
補足
<番号>r.cppは<番号>.cppを書きなおしたものになっています。(微妙に違うか同じです)