Conversation
|
すみません、leetcode から貼り付けるとインデントが2つになるみたいなので4つに調整します。 |
|
調整しましたのでレビューお願いします |
stack/20_valid_parentheses/20.md
Outdated
| } | ||
|
|
||
| private boolean isCloseSymbol(char symbol) { | ||
| return validParentheses.containsValue(symbol); |
There was a problem hiding this comment.
お疲れ様です。
指摘するところがなく重箱の隅をつつくような指摘となってしまいますが、
Map.containsValue()は値を順番に走査するO(n)なので、
パフォーマンスの観点ではO(1)のSet.contains()を使った方が良いかもしれません。
※今回の場合、括弧の種類が3つだけなので、そんなに大きな差にならないとは思いますが。
There was a problem hiding this comment.
ありがとうございます、おっしゃる通りかと思います。
修正します。
naoto-iwase
left a comment
naoto-iwase
left a comment
There was a problem hiding this comment.
お疲れさまです。
いくつかコメントさせていただきました。
自分がjavaあまり詳しくないので、話半分で見ていただけたらと思います。
stack/20_valid_parentheses/20.md
Outdated
| ); | ||
|
|
||
| public boolean isValid(String s) { | ||
| Stack<Character> openSymbols = new Stack<>(); |
There was a problem hiding this comment.
https://docs.oracle.com/javase/jp/8/docs/api/java/util/Stack.html
より完全で一貫性のある一連のLIFOスタック・オペレーションが、Dequeインタフェースとその実装によって提供されています。このクラスよりもそれらを優先的に使用するようにしてください。 たとえば、
Deque<Integer> stack = new ArrayDeque<Integer>();
There was a problem hiding this comment.
スタックデータ構造であれば、 Stack より ArrayDeque を優先的に使用することがおすすめです。
Stackはスレッドセーフな実装になっており、低速なためです。
https://docs.oracle.com/javase/jp/8/docs/api/java/util/ArrayDeque.html
このクラスは通常、スタックとして使われるときはStackよりも高速で、キューとして使われるときはLinkedListよりも高速です。
There was a problem hiding this comment.
お二人ともありがとうございます、 Deque を使おうと思います。
マルチスレッド等で共有スタックとして使うなら Stack は便利そうですね
| } | ||
|
|
||
| private boolean isCloseSymbol(char symbol) { | ||
| return validParentheses.containsValue(symbol); |
There was a problem hiding this comment.
containtsKeyはkeyがhashtableを構築しているのでO(1)で存在確認できますが、containsValueはvalueがhashtableを持っておらず、存在確認のたびに配列を線形探査するのでカッコの種類をmとしてO(m)になります。
閉じカッコの存在確認が必要な場合には、Set closeSymbols = Set.of(')', ']', '}');を追加した方が良いと思います。
class Solution {
private static final Map<Character, Character> VALID_PARENTHESES = Map.of(
'(', ')',
'[', ']',
'{', '}'
);
private static final Set<Character> CLOSE_SYMBOLS = Set.copyOf(VALID_PARENTHESES.values());（インスタンス状態に依存しない定数なのでstatic finalでしょうか）
There was a problem hiding this comment.
そうですね、とはいえ3種類しかないので今回はいいかなあと思ってます。
static final はその通りです。
| - コメント集: [20. Valid Parentheses](https://docs.google.com/document/d/11HV35ADPo9QxJOpJQ24FcZvtvioli770WWdZZDaLOfg/edit?tab=t.0#heading=h.ns0bie22a6m) | ||
| - 引数に想定外がきた時はどうする?が気になったので実装に含めてみました | ||
| - 引数に対して、想定しない値がの場合は `IllegalArgumentException` を投げるようにしました | ||
| - が、コンパイルとか Linter のことを考えると、`(ai){ueo}` みたいなのが来たときは continue とかの方が良いのかもと思ったりしたので別解として載せました |
| if (isIllegalSymbol(symbol)) { | ||
| throw new IllegalArgumentException("Invalid Character is detected, symbol is " + symbol); |
There was a problem hiding this comment.
この設計の場合、入力文字列の長さが奇数なら即falseとかもできますよね。
|
自分も今この問題やっているのでコード拝見しました。コンパイラやlinterなど実際のユースケースを想定して例外処理などを考えている点が素晴らしいと思いました。 |
| } | ||
|
|
||
| if (isOpenSymbol(symbol)) { | ||
| openSymbols.push(symbol); |
There was a problem hiding this comment.
この分岐に入った場合、後続の処理がないのであればcontinue;してelse節のインデントも下げてあげたほうが読みやすくなると思います。
There was a problem hiding this comment.
なるほどなと思いました。
continue してあげた方が読みやすいか、確かに。。
| if (openSymbols.empty()) { | ||
| return false; | ||
| } | ||
| char peekedOpenSymbol = openSymbols.peek(); |
There was a problem hiding this comment.
この時点でpop()してもいいかなと思います。
char peekedOpenSymbol = openSymbols.pop();
if (!isMatched(symbol, peekedOpenSymbol)) {
return false;
}There was a problem hiding this comment.
そうですね、手作業をコードに落とした結果 peek してから pop してますが、そちらでも良いと思います。
stack/20_valid_parentheses/20.md
Outdated
|
|
||
| ```java | ||
| class Solution { | ||
| private final Map<Character, Character> validParentheses = Map.of( |
There was a problem hiding this comment.
定数相当ですので、 private static final のほうが良いかなと思いました。
There was a problem hiding this comment.
ありがとうございます、おっしゃる通りです。
6 participants
レビューお願いします！