Skip to content

127. Word Ladder#19

Open
seal-azarashi wants to merge 8 commits intomainfrom
word-ladder
Open

127. Word Ladder#19
seal-azarashi wants to merge 8 commits intomainfrom
word-ladder

Conversation

@seal-azarashi
Copy link
Copy Markdown
Owner

Comment on lines +166 to +167
// 時間計算量: O(N^2)
// 空間計算量: O(N)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nが何か分からないです & この2つの計算量って合ってますか?

Copy link
Copy Markdown
Owner Author

@seal-azarashi seal-azarashi Sep 29, 2024

Choose a reason for hiding this comment

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

@fhiyo
返信が大変遅くなりました。
N を明示する際改めて計算してみたのですが、間違っていますね。次のように修正しました。

/**
 * 時間計算量: O(n * m^2):
 *     - O(n): wordList のクローン (n は wordList の要素数)
 *     - O(n * m^2): Map の生成 (n は wordList の要素数、 m は要素の文字数)
 *     - O(n * m^2): wordQueue の要素の走査 (n は wordQueue の最大要素数、 m は要素の文字数)
 * 空間計算量: O(n * m)
 *     - O(n): wordListClone
 *     - O(n * m): globPatternToWords (n は Map の要素数、 m は value の要素数)
 *     - O(n): wordQueue
 *     - O(n): visitedWords
 */


private String createGlobPattern(int index, String word) {
StringBuilder patternBuilder = new StringBuilder(word);
patternBuilder.setCharAt(index, '?');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

leetcodeの制約上問題ないですが、'?'が使える文字に含まれる場合は上手くいかないですかね。

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.

確かにそうですね。一般的に String 値に含まれない null 文字を変わりに使ってみようと思います。

Set<String> visitedWords = new HashSet<>();
visitedWords.add(beginWord);

int distance = 1;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

(細かい気はしますが) この変数名はちょっと違和感ありました。
beginWord = "a", endWord = "b", wordList = ["b"] だと答えは2になりますが、distanceとしては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.

仰るとおりだと思いました。Step 4 で numberOfWordsInSequence としてみましたが、こちらはいかがでしょうか?

private static final int NO_SEQUENCE_EXISTS = 0;

public int ladderLength(String beginWord, String endWord, List<String> wordList) {
Deque<WordDistance> wordDistances = new ArrayDeque<>();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

これ Queue<WordDistance> wordDistances = new ArrayDeque<>(); で良かったりしますか?

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.

はい、特に問題ないです。
Queue にするなら使うメソッドも offer(), poll() に書き換えたいですね

while(!wordQueue.isEmpty()) {
distance++;
int wordQueueSizeSnapshot = wordQueue.size();
for (int wordQueueIndex = 0; wordQueueIndex < wordQueueSizeSnapshot; wordQueueIndex++) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

wordQueueIndex, 意味ありげに長いけど使ってない変数なので気になりました。中のiをjとかに変えてiで良いような気がします

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.

そうですね、ロジックを書いててややこしくならないように命名してましたが、これは i とかで良いように思います。

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.

step 4 の修正案ではこちらのレビューを踏まえ、そもそもキューの扱い方を変えてこの for 文に相当するものでイテレータを使わなくなってしまいましたが、今後はそのようにしようと思います。

for (String word : wordList) {
for (int i = 0; i < word.length(); i++) {
String globPattern = createGlobPattern(i, word);
List<String> list = adjacencyMap.getOrDefault(globPattern, new ArrayList<>());
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

adjacencyMapと言いつつそのkey-valueの関係はpatternとそれにマッチするwordsの関係なのでちょっと気になりました。patternToWordsとかで良い気がします。

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.

step 4 で globPatternToWords に修正しました。

addedWords.add(targetWord);
wordDistances.addLast(new WordDistance(targetWord, wordDistance.distance + 1));
}
wordList.removeAll(addedWords);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

wordList -> remainedWords, addedWords -> waypointWordsとかでどうでしょうか?

continue;
}
wordQueue.offer(adjacentWord);
visitedWords.add(adjacentWord);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ネストが少しキツイ気がします。
BFSなら配列を用意してword.length()のループを無くしたり、197-205辺りを別途関数として切り出すのは如何でしょうか。

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.

step 4 で対応しました。

```

- Levenshtein distance, Edit distance, Hamming distance についての知識があれば `step` よりも `distance` の方が役割を理解しやすいなと思ったので変数名を修正 ([こちらのコメント](https://github.com/Ryotaro25/leetcode_first60/pull/22#issuecomment-2255580125)とその引用から知りました)
- クラスの外から渡される引数 wordList を書き換える副作用があっていいのかは議論の余地ありなので、一応その点コメント
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

個人的にwordListは弄らずにそっとしておきたい気がします。

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.

そうですよね。step 4 では元はいじらずにクローンオブジェクトを作ってそれを扱うようにしました。

Comment on lines +310 to +312
for (int i = 0; i < word.length(); i++) {
String globPattern = createGlobPattern(i, word);
for (String adjacentWord : adjacencyMap.get(globPattern)) {
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

Choose a reason for hiding this comment

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

step 4 で対応しました。

Comment on lines +304 to +307
int distance = 1;
while(!wordQueue.isEmpty()) {
distance++;
int wordQueueSizeSnapshot = wordQueue.size();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

個人的に、一つの Queue と distance 変数を使って、ループごとに distance が一定な要素だけが出てくるという処理、間違ってはいないんですが、これが「ループごとに distance が一定な要素だけが出てくる」ことを示すためには、ループ全体で変なこと(たとえば、頭から追加されていないことなど)がなされていないことを確認しないといけないので、個人的には、ループごとに別の Deque なり Array なりを使って切り替えていくほうが好みではありますね。

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.

そうですね、Deque など new する処理は比較的重いから避けたかったのと、この書き方に慣れてて違和感無かったのでこの実装にしましたが、コメント頂いた書き方のほうが自然に思います。
他の指摘と合わせて後ほど修正します。

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.

あとネストも深くなりますしね今の実装だと

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.

修正結果を step 4 に書きました。

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