Conversation
nanae772
left a comment
nanae772
left a comment
There was a problem hiding this comment.
全体的に読みやすく違和感や引っかかりを感じる部分もありませんでした！
（コード参考にしていただきありがとうございます）
| public boolean isValid(String s) { | ||
| // stackとして使用する | ||
| Deque<Character> stack = new ArrayDeque<>(); | ||
| Set<Character> openBrackets = new HashSet<>(); |
There was a problem hiding this comment.
openBrackets は初期化後に変更しないため、 Set.of() を使ってシンプルに書けると思います。
Set<Character> openBrackets = Set.of('(', '[', '{');https://docs.oracle.com/javase/jp/11/docs/api/java.base/java/util/Set.html
There was a problem hiding this comment.
ありがとうございます。静的なSet/Mapはあまり使ったことがなく、この書き方が見えていませんでした。
各リファレンスを読みます。
| openBrackets.add('['); | ||
| openBrackets.add('{'); | ||
|
|
||
| for (Character character : s.toCharArray()) { |
There was a problem hiding this comment.
for (char character : s.toCharArray()) {と値型で受けて、以下の if 文の条件式を character == ')' と == 演算子で書いたほうが、読みやすいと思います。
ボクシング・アンボクシングによってパフォーマンスが変わるかもしれませんが、パフォーマンスにシビアな場面以外ではあまり気にしなくてよいと思います。パフォーマンスにシビアな場面では、マイクロベンチマークを取り、早い書き方を選ぶとよいと思います。ただ、パフォーマンスにシビアな場面では、 C++ 等、より速い言語を使用したほうが良いかもしれません。
There was a problem hiding this comment.
多くの場合ボクシング・アンボクシングの有無によってパフォーマンスが大きく変わることはなさそうですね。
==で書けるなら、そちらを優先してみようと思います。
| if (openBrackets.contains(character)) { | ||
| stack.push(character); | ||
| } else if (character.equals(')')) { | ||
| if (Objects.equals(stack.peek(), '(')) { |
There was a problem hiding this comment.
if (stack.peek().charValue() == '(') {のほうが読みやすいかしれません。この辺りはチームの平均的な書き方に合わせることをおすすめします。
There was a problem hiding this comment.
if (stack.peek().charValue() == '(') {
例示いただいたコードでボクシングに任せず明示的にcharValue()しているのは、char同士の比較であることを明示するためでしょうか?
私が書くならボクシングに任せて
if (stack.peek() == '(') {としますが==で書く場合はcharValue()で明示するのが一般的でしょうか?
There was a problem hiding this comment.
Java の仕様を勘違いしておりました。この場合はアンボクシングされるので
if (stack.peek() == '(') {と書くのが良いと思います。
https://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-15.21
| // stackとして使用する。開き括弧を積む | ||
| Deque<Character> stack = new ArrayDeque<>(); | ||
| // Map<開き括弧, 閉じ括弧> | ||
| Map<Character, Character> brackets = new HashMap<>(); |
There was a problem hiding this comment.
こちらも Map.of() を使ったほうがシンプルに書けると思います。
Map<Character, Character> brackets = Map.of('(', '), '[', ']', '{', '}');https://docs.oracle.com/javase/jp/11/docs/api/java.base/java/util/Map.html
| // 開き括弧の場合 | ||
| if (openToClose.containsKey(bracket)) { | ||
| openBracketsStack.push(bracket); | ||
| continue; |
There was a problem hiding this comment.
If節に入った場合、else節はスキップされるので、continue;は不要なように思いました
There was a problem hiding this comment.
どちらかというと else の方を消したいですね。
continue がないと続きを探して、else の閉じ括弧を探しに行く必要があります。
There was a problem hiding this comment.
continue がないと続きを探して、else の閉じ括弧を探しに行く必要があります。
なるほど、そのような意図だったのですね、ありがとうございます。
いずれにせよおっしゃる通り、continueを使う場合はelseを消した方が自然ですね。
There was a problem hiding this comment.
コメントありがとうございます！
確かにこのelseは余計でした。
step3の方では気づいてelseを消してますね。
|
全体的に読みやすかったです！ |
6 participants
今回解いた問題：
20. Valid Parentheses
次に解く問題：
206. Reverse Linked List