Skip to content

Create No2.20. Valid Parentheses.md#2

Open
kzhra wants to merge 2 commits into
mainfrom
No2.20.-Valid-Parentheses
Open

Create No2.20. Valid Parentheses.md#2
kzhra wants to merge 2 commits into
mainfrom
No2.20.-Valid-Parentheses

Conversation

@kzhra

@kzhra kzhra commented May 10, 2024

Copy link
Copy Markdown
Owner

class Solution {
public:
bool isValid(string s) {
stack<char> parenthese;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

parenthesesのtypoでしょうか?

Comment on lines +101 to +102
for(char c: s) {
if(c == '(' || c == '{' || c == '[') {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

https://google.github.io/styleguide/cppguide.html#Formatting_Looping_Branching
リンク先はあくまでルールの例ですが、forやifの後ろは一つスペースを入れる方が良いと思います。

Comment on lines +104 to +109
} else if(!parenthese.empty() && c == ')' && parenthese.top() == '(') {
parenthese.pop();
} else if(!parenthese.empty() && c == '}' && parenthese.top() == '{') {
parenthese.pop();
} else if(!parenthese.empty() && c == ']' && parenthese.top() == '[') {
parenthese.pop();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

(小田さんのコメントにもありますが) もう少しまとめた方が見やすそうですしバグも起こしにくそうです。一箇所だけ parenthese.empty() の頭にある否定演算子を忘れた、とかありがちだと思うので。

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

また、まとめることで拡張性という点でもメリットがあると思います。
もし判定する括弧がもっと増えた場合に修正箇所が少なく済みそうです。

class Solution {
public:
bool isValid(string s) {
stack<char> parenthese;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

実際に入るのは '(', '{', '[' の3種類だと思うので、 leftParentheses とか openParentheses とかの方がいいかもしれません

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

odaさんがよくアドバイスされていますが、変数名に意味を持たせるのは大事だと感じます。
ここでは、fhiyoさんのご提案頂いた変数名にすることで、コードを読まなくてもこのstackに入るのはopen-bracketなんだな、ということが分かるようになり読み手の仕事が捗ると思います。

ご参考
https://discordapp.com/channels/1084280443945353267/1230079550923341835/1230201155619913728

# step3
変数名をジェネリックプログラミングの観点から型名を除去
if continue から if else に変更
条件判定で最初に empty を判定。理由は覚えていないが多分計算量を少しでも減らすため?代わりに可読性の低下

@TORUS0818 TORUS0818 May 11, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ここは本当に計算量が減るのか、そうならばどういう原理で減るのか、調べてみるといいと思います。調べて分からない場合は講師役の方に質問してみるといいと思います。

レビューで指摘された箇所を修正したコード

@fhiyo fhiyo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

良いと思います。

Comment thread step4.md
@@ -0,0 +1,30 @@
# step4

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

インデントが揃ってないです。

Comment thread step4.md
public:
bool isValid(string s) {
stack<char> open_parentheses;
unordered_map<char, char> match_parentheses;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Comment thread step4.md

for(char c: s) {
if (c == '(' || c == '{' || c == '[') {
open_parentheses.push(c);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

私は if-continue が好みです。

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