-
Notifications
You must be signed in to change notification settings - Fork 0
127. Word Ladder #20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
127. Word Ladder #20
Conversation
- ただしBFSでないとだめ。 | ||
- DFSの場合、 `nit` から削除してしまう可能性があるから。 | ||
|
||
- 以下の処理がEBCDICでは通らない。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
私はこの環境を触ったことがないので基本的には気にしなくて構わないと思います。
for (int next_index = 0; next_index < current_word.size(); ++next_index) { | ||
for (char next_char = 'a'; next_char <= 'z'; ++next_char) { | ||
std::string next_word = current_word; | ||
next_word[next_index] = next_char; | ||
if (!candidate_words.contains(next_word)) { | ||
continue; | ||
} | ||
candidate_words.erase(next_word); | ||
words_to_explore.push(next_word); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
四重ループはさすが厳しいです。
BFS をしている部分と、ここの距離が1の言葉を列挙してみるところに分けるとか、なんらかの方法はあるように思います。
BFS の方法も、長くなると読みにくいものだと思います。これだと current_candidate_size が途中で変更されていないことを確認しないと BFS のつもりであることも分からないのです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
step3で少し関数に切り分けましたが、指摘から考えて、より切り分けれることに気づきました。
少なくとも current_candidate_size を cost にすべきでした。
読み手の感覚まで教えていただきありがとうございます。
public: | ||
int ladderLength(std::string beginWord, std::string endWord, std::vector<std::string>& wordList) { | ||
std::set<std::string> candidate_words = std::set(wordList.begin(), wordList.end()); | ||
std::queue<std::string> words_to_explore; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
好みの問題かもですが、XXX_to_YYY
という命名規則は、一瞬std::map
の変数っぽく見えてしまうので、別の名前を使うと良いかもしれません。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
おっしゃる通りです。
少なくとも exploring_words
などにすべきです。
ありがとうございます。
6.graph/word-ladder/step3.cpp
Outdated
if (candidate_words.contains(next_word)) { | ||
continue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここの処理の意図を日本語で説明してもらえますか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
「candidate_words
に含まれてない場合は continue する」という意図のつもりでしたが、コードは逆になっています。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
こちらのコードはテストケースは全て通りましたか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
いえ通りませんでした。
step3終了後のリファクタリングでタイプミスしてしまったようです。
修正をコミットしました。
if (current_word == endWord) { | ||
return ladder_length; | ||
} | ||
for (int next_index = 0; next_index < current_word.size(); ++next_index) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
next_
という接頭辞が頻出しますが、分かりづらかったです。
隣接する単語関連の変数というのはわかりますが、各変数がロジックの中でどういう役割を負っているのかが不明瞭に感じます。
例えば、next_index
はword_index
、next_char
はconverting_char
、next_word
はcanditdate_word
などにするとコード内の実態と合って読みやすくなると思いました。
https://github.com/Hurukawa2121/leetcode/pull/20/files#r1912426507
でも指摘されていますが、このfor文のロジックをメソッド抽出すると読みやすくなるかもしれません。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
converting_char
などは自分の選択肢にありませんでした。
ループに関しておっしゃる通りです。
step3で少し関数に切り分けましたが、指摘されてより切り分けれると思いました。
ありがとうございます。
|
||
class Solution { | ||
public: | ||
int ladderLength(std::string beginWord, std::string endWord, std::vector<std::string>& wordList) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この問題ではstring_viewを使えば、文字列のコピーは不要かもしれません。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
確かに、単語の同士のハミング距離を見れば隣接リストが作れます。
ありがとうございます。
for (int index = 0; index < word.size(); ++index) { | ||
for (char next_letter = 'a'; next_letter <= 'z'; ++next_letter) { | ||
std::string transformed_word = word; | ||
transformed_word[index] = next_letter; | ||
if (candidate_words.contains(transformed_word)) { | ||
adjacency_list[word].push_back(transformed_word); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ループは2重までにしたい気持ちが私にはあって、
たとえば、この部分を lambda とかでもいいので、
void AddWordToAdjacencyList(const string& word, ... &adjacency_list);
などとしてもいいかもしれません。これはこれでと思います。
あと、Index で隣接リストを作ると速そうですね。(map をたどるときに文字列の比較が多数行われるため。)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ループは2重までにしたい気持ちが私にはあって
確かに、AddWordToAdjacencyLis
のアイデアを用いてstep4を実装したら、とても読みやすくなりました。
「ループは2重まで」をルールにしたいと思います。
map をたどるときに文字列の比較が多数行われるため
これに気づきませんでした。
こちらも実装し、だいぶ早くなりました。
教えていただきありがとうございます。
class Solution { | ||
public: | ||
int ladderLength(std::string beginWord, std::string endWord, std::vector<std::string>& wordList) { | ||
std::set<std::string> candidate_words = std::set(wordList.begin(), wordList.end()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::set<std::string> candidate_words(wordList.begin(), wordList.end());
と書いたほうがシンプルだと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
存じ上げませんでした。
次からそのように書きます。
ありがとうございます。
while (!words_to_explore.empty()) { | ||
int current_candidate_size = words_to_explore.size(); | ||
ladder_length++; | ||
for (int i = 0; i < current_candidate_size; ++i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
現在のキューの長さを保持し、その長さ分だけ先頭から取り出して処理するという実装は、個人的にはあまり見ません。キューに ladder_length 相当の値も一緒に入れて 1 要素ずつ処理するか、二つの配列を用意し、交互に入れ替えていく実装のほうが分かりやすいと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ありがとうございます。
Editorialの実装なのですね。
確かに、自分のコードと比べると、動きが格段に把握しやすいです。
「よく見る実装に寄せる」を読みやすく観点として持ちます。
for (int i = 0; i < current_candidate_size; ++i) { | ||
int current_index = indexes_to_explore.front(); | ||
indexes_to_explore.pop(); | ||
if (!PushCandidateIndex(current_index)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wordList[candidate_index] == endWordという条件判定は関数内ではなく外で行い、PushCandidateIndexはindexes_to_exploreにpushすることに専念した方が、個人的にはわかりやすいと思います。
indexes_to_explore.push(begin_index); | ||
std::vector<std::vector<int>> adjacency_list = ComposeAdjacencyList(wordList); | ||
|
||
auto PushCandidateIndex = [&](int current_index) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
candidateという語が指すものに一貫性がないように感じました。(BFSで次に調べるものなのか、今調べているものなのか)
そのせいで、L30でcandidate_words.erase(wordList[candidate_index]);という、candidateからなぜかcandidateを除くという一見したわかりにくい動作になっていると思います
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
確認済みのインデックスをセットに保存したほうが分かりやすいように思いました。
int current_candidate_size = indexes_to_explore.size(); | ||
for (int i = 0; i < current_candidate_size; ++i) { | ||
int current_index = indexes_to_explore.front(); | ||
indexes_to_explore.pop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
上のodaさんやnodchipさんのコメントとも重なりますが、current_candidate_sizeがindexes_to_exploreのsizeを参照していて、それを元にループを回したあと、indexes_to_exploreがfrontされたりpopされたりすると、やや不安を感じます。
} | ||
|
||
private: | ||
int HammingDistance(std::string& word1, std::string& word2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
word1とword2のsizeが等しくない場合にどうなるか考えてもいいかもしれません
class Solution { | ||
public: | ||
int ladderLength(std::string beginWord, std::string endWord, std::vector<std::string>& wordList) { | ||
std::set<std::string> candidate_words = std::set(wordList.begin(), wordList.end()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the words in wordList are unique.
と問題文にあるので、文字列をコピーしてセットを作るのではなく、すでに確認済みのインデックスをセットに保存すると効率的でしょう。
問題へのリンク
127. Word Ladder
次に解く問題
Maximum Depth of Binary Tree