139. Word Break by Ryotaro25 · Pull Request #43 · Ryotaro25/leetcode_first60

Ryotaro25 · 2024-11-28T14:48:25Z

問題へのリンク
https://leetcode.com/problems/word-break/description/

問題文(プレミアムの場合)

備考

次に解く問題の予告
Coin Change

フォルダ構成
LeetCodeの問題ごとにフォルダを作成します。
フォルダ内は、step1.cpp、step2.cpp、step3.cpp、bfs.cpp、dfs.cpp、trie.cppとmemo.mdとなります。

memo.md内に各ステップで感じたことを追記します。

oda · 2024-11-28T17:50:31Z

+   }
+
+private:
+  bool CheckWordBreakable(string_view word, const set<string_view>& word_dict,


string_view word の word が単語一つではなくて結合しているかを知りたいものなのが結構意外感があります。

@oda いい単語が出てこなかったので単純にstrとしました。

oda · 2024-11-28T17:54:06Z

+class Solution {
+public:
+  bool wordBreak(string s, vector<string>& wordDict) {
+    TrieNode* root = new TrieNode();


これ、new したのを delete していないのでメモリーリークしています。

また、できれば Trie の構築はメンバ関数にするときれいでしょう。

@oda
メモリへの意識がまだまだ低いです。deleteを書くのではなく、smart pointerを使う方式に変更しました。
また、Trieの構築と探索をメンバ関数に変更。
vectorをvectorへ変更しました。

trie_step2.cpp
6ba5f48

あ、単に TrieNode root; でもいいでしょう。あとで、&root とする必要がありますが。

oda · 2024-11-28T17:55:39Z

+        TrieNode* node = root;
+        for (int end = start; end < s.size(); end++) {
+          char letter = s[end];
+          if (!node->children.contains(letter)) {
+            break;
+          }
+
+          node = node->children[letter];
+          if (node->is_word) {
+            is_segmented[end] = true;
+          }
+        }


ここもメンバ関数にしてしまってもいいかもしれません。

oda · 2024-11-28T17:58:07Z

+      return word_to_validity[word];
+    }
+
+    for (int i = 0; i < word.size(); i++) {


ここ、word_dict 側で調べるのも手で、
https://en.cppreference.com/w/cpp/string/basic_string_view/starts_with
starts_with が C++20 からあります。

word_dict側で調べる方式でstep4に追加しました。
6ba5f48

hayashi-ay

Bottom Up DPでも書いてみると良いかなと思います

philip82148 · 2024-11-29T04:24:38Z

+    }
+
+    for (int i = 0; i < word.size(); i++) {
+      string_view left_side_word = word.substr(0, i + 1);


こちら、wordがstring_viewなので動いていますが、wordがstringだった場合動かなくなります。
でも、wordがstringでも、以下のコードなら動きます。

auto left_side_word_str = word.substr(0, i + 1); string_view left_side_word = left_side_word_str;

どうしてか説明できますでしょうか…？(C++って難しいですね)
(理解して使われているかが気になりました)

@philip82148 さん

どうしてか説明できますでしょうか…？

私も守りきれているわけではないですが、レビューで疑問文を使うパターンはあまり多くなく、これはそのパターンから外れているように思います。通常は、提案と意図の確認ですね。

提案「こちらの方がいいのではないでしょうか?」

意図の確認「A, B, C が選択としてあると思いますが、A を選んだ意図はなんでしょうか?」

この2つは自分が知りようがないので聞いています。自分が知っているならば、書いてしまったほうがコミュニケーションコストが低いですからね。
コードレビューの目的は、よりよいコードにすることなので、この2つはそれに向いています。

ここの場は、「ソフトウェアエンジニアの入口レベルに到達するための訓練」という側面(もう一つの目的)もあるので、面接の情報提供、言葉使いや理解の修正が行われていることもあります。

ただ、この疑問文は「私の知っている正解を述べられるか」で疑問文を使う場面ではありません。

そういうわけで、この場合は「このように書いた場合はこの理由で動きません。(あれば)信頼できるソースへのポインターはこれです。」とまで書いてください。

@oda 承知しました！
すみません、では修正してもう一度コメントさせていただきますmm

元のコードが悪い訳ではありませんが、string_viewを使うときは、誰が文字列データ本体を持っているかを意識する必要があるということが元々言いたかったです。
それをwordがstringだと動かないという話で説明したいと思います。

wordがstringの場合、word.substr()は新しいstringを生成します。これは元のwordをコピーしたもので、文字列データ本体(メモリ)が別に用意されます。
string_viewは、文字列データ本体を誰かに持ってもらって、それを覗き見るデータ構造になっています。
(先頭ポインタと文字数だけ保存していて、文字列データ本体は誰か別の人が持っている状態。)
なので、

string_view left_side_word = word_str.substr(0, i + 1);

だと、word_str.substr()で一時的にstringが生成され(これを一時オブジェクトAとします)、その先頭ポインタとサイズがstring_viewに保存されますが、この式全体が実行された直後に一時オブジェクトAはdestructされ、文字列データ本体(メモリ)が解放されてしまいます。
しかし、以下のようにしていれば、一時オブジェクトAはleft_side_word_strというローカル変数になり、(同じブロックスコープ内では)destructされなくなります。なので動きます。

auto left_side_word_str = word.substr(0, i + 1); string_view left_side_word = left_side_word_str;

なので、string_viewを使うときは、誰が文字列データ本体を持っているかを意識する必要があります。
なお例えば、wordの文字列データ本体を持っているのはwordBreak()のstring型の引数sです。

※文字列データ本体=charの配列

※ちなみに僕は一瞬元のコードを見てびっくりしてしまった(なんでこれ動いてるんだ?→ああwordってstring_viewなのか)のでこの話を出したのですが、よくよく考えてみるとそんなに変なコードでもないのかなと思い。
ただ、個人的な好みとしてはauto left_side_word = ...のようにしておくとこういう違和感(+途中でやっぱstringの方がいいやとなった時に変更箇所)を生まなくていいと思っています(右辺がstringでもstring_viewでも問題ない)

後ついでに left_side_wordって長いのでleftかprefixはどうでしょうか？
(一応どなたか講師役の方リアクションしていただけると)

https://abseil.io/tips/1

Because the string_view does not own its data, any strings pointed to by the string_view (just like a const char*) must have a known lifespan, and must outlast the string_view itself.

@philip82148 良くなったと思います。ありがとうございます。

そうですね。簡単にまとめます。
string_view は典型的にはポインターと長さの組で、文字列のオーナーシップを持っていないので、指しているデータが破壊されると、string_view 自体が invalidate になりますね。

string_view left_side_word = word.substr(0, i + 1);

の行を見たときに、word が string だとすると、これは一時オブジェクトを返し、left_side_word は生存期間が終了している string を指していることになりますね。

こういうとき、たとえば、「substr が string を返すかと思い生存期間の問題があるように勘違いした。一回 auto で受けたら勘違いしないのでは。」というのは、よりよいコードの提案(コードレビューの類型)として成立していますね。

string_view は C++17 からなのであまり馴染がない人は多いでしょう。(私は昔、StringPiece という似たような非標準ライブラリーを使っていましたので馴染みがないです。)

提案自体へのコメントとしては、

元のコードの設計思想として、wordBreak がオーナーシップを持って、CheckWordBreakable は string_view だけで処理するということかと思うので、それが通じれば自然かと思います。

auto で受けた後に string_view に代入しているが、それだったら、単に auto で受けるのも一つです。ただ、これは勘違いさせたままで続きを読ませることになります。

するとコメントを一言書くくらいがバランスとしていいかなあ。たとえば、// Note: This substr is of type string_view, not string. Be mindful of ownership and lifetime issues.

@philip82148
レビューとまた詳細な質問ありがとうございます。
string_viewを用いたのは以下のコメントを読んで調べたのがスタートです。
philip82148/leetcode-swejp#8 (comment)

なので、string_viewを使うときは、誰が文字列データ本体を持っているかを意識する必要があります。なお例えば、wordの文字列データ本体を持っているのはwordBreak()のstring型の引数sです。

この理解でした。今回はコピーを作って、コピー文字列に対して何かしたい、返却したいといった問題ではなかったのでstring_viewを使うことにしました。

個人的な好みとしてはauto left_side_word = ...のようにしておくとこういう違和感(+途中でやっぱstringの方がいいやとなった時に変更箇所)を生まなくていいと思っています

これは確かにその通りでautoだとstringなのかstring_viewなのかとなりますね。誤解のないようなコードの書き方を心がけたいと思います🙇‍♂️

@oda レビューありがとうございます。

string_view は C++17 からなのであまり馴染がない人は多いでしょう。(私は昔、StringPiece という似たような非標準ライブラリーを使っていましたので馴染みがないです。)

自分もこの問題を通してstring_viewを知りました。string_viewがどういったものなのかコード内にコメントで書いておくべきでしょうか?

まだGitに上げていないのですが下記のようなコメントを追記する予定です。
// string_viewのstrはデータを持っていない
// 元の文字列データ(ここではwordBreak関数のstring str)へのポインターを管理している
// コピーを発生させないため

いや、それは不要でしょう。それをするとほとんどすべての関数呼び出しにその粒度でコメントをつけることになりますね。

読む人が、「適切なドキュメントに当たることができる人物である」ことは仮定して書いていいです。これは知識を仮定しているというよりは「調べる能力」を仮定しています。
https://docs.google.com/presentation/d/1Ny4kmHE2FZMI0AuPxImokweGoAE73RAGivjDJg0kG80/edit#slide=id.g2194639112d_0_294

oda · 2024-11-29T08:25:14Z

+      return true;
+    }
+
+    if (word_to_validity.contains(word)) {


これはある種のキャッシュのつもりで足してあると思うのですが、実際に使われる場合は基本ないように思います。合ってますでしょうか。

あ、誤読ですね。これ false の場合使われていますね。

frinfo702

c++あまり詳しくなくてすみません。ただ、ネストを少しでも減らせるのはいいのかなと思います

frinfo702 · 2024-11-30T12:12:58Z

+public:
+  bool wordBreak(string s, vector<string>& wordDict) {
+    set<string> word_dict(wordDict.begin(), wordDict.end());
+    map<string, bool> word_to_validity;


コメントでこのmap部分の定義の意図を書いておいてはいかがでしょうか？一度解いた身なので察しはつきますが、この時点では何をするためのものかわからず、読み進めながら何度かこちらに目を移し役割を確認してしまったので

@frinfo702
レビューありがとうございます。
確かに不親切ですね。。読み手のことを考えコメントも積極的に使おうと思います。
別レビュー事項を反映し変数名も変わっておりますがコメントを追記しました。

step4.cpp
6ba5f48

frinfo702 · 2024-11-30T12:32:26Z

+    for (int i = 0; i < word.size(); i++) {
+      string left_side_word = word.substr(0, i + 1);
+
+      if (word_dict.contains(left_side_word)) {


ここ、

if (!word_dict.contains(left_side_word)) { continue; }

と書くと深いネストを一段改善できると思います

nodchip · 2024-11-30T12:57:07Z

+    set<string_view> word_dict(wordDict.begin(), wordDict.end());
+    queue<int> dividing_positions;
+    dividing_positions.push(0);
+    vector<bool> visited(s.size(), false);


vector は特殊化されており、速度がやや遅くなる可能性があります。
https://cpprefjp.github.io/reference/vector/vector.html
vector<char> visited(s.size(), false); を使ったほうが速くなるかもしれません。

@nodchip

メモリの効率化を行っているから[]の場合、インデックスでアクセスする場合に遅い場合があるのですね。。。
この辺り読んでみました。
https://qiita.com/voidhoge/items/383244bad2d728a18dbe
https://learn.microsoft.com/ja-jp/cpp/standard-library/vector-bool-class?view=msvc-170
https://kmyk.github.io/blog/blog/2017/06/07/bitset-vector-bool/

vectorに置き換えてみました。
6ba5f48

nodchip · 2024-11-30T12:58:46Z

+        return true;
+      }
+
+      for (int end_position = start_position + 1; end_position <= word.size(); end_position++) {


文字列の長さの最大値で打ち切ったほうが、計算量が落ち、速くなると思います。

nodchip · 2024-11-30T12:59:58Z

+        if (visited[end_position]) {
+          continue;
+        }
+        if (word_dict.contains(word.substr(start_position, end_position - start_position))) {


文字列のコピーを避けるため、
string_view(word.begin() + start_position, word.begin() + end_position)
とできるでしょうか。

@nodchip
上の方でstring_view word = s;としていたのですが微妙でしたね。。。

string_view(word.begin() + start_position, word.begin() + end_position)
この書き方知りませんでした🙇‍♂️ありがとうございます。

bfs_step2.cppに追加しました。
6ba5f48

nodchip · 2024-11-30T13:05:07Z

+  bool wordBreak(string s, vector<string>& wordDict) {
+    string_view word = s;
+    set<string_view> word_dict(wordDict.begin(), wordDict.end());
+    queue<int> dividing_positions;


queue<int> を使って、分割地点の候補を管理するより、 start_position を 0 から word.size() - 1 まで回したほうがシンプルだと思いました。ただし、 start_position が分割の開始地点の候補でないことを確認するため、

if (!visited[start_position]) { continue; }

をどこかに入れることになると思います。

@nodchip
for.cppとして新たにファイルを追加しました。

nodchip · 2024-11-30T13:07:32Z

+    set<string_view> word_dict(wordDict.begin(), wordDict.end());
+    queue<int> dividing_positions;
+    dividing_positions.push(0);
+    vector<bool> visited(s.size(), false);


visited という名前は、グラフ等で探索済みのノードを表すのであれば自然だと思うのですが、今回のように、すでに分割地点として調べたかどうかを表すのには不適切だと感じました。あまりいい変数名が思いつかないのですが、分割済みである divided や、処理済みである proceeded あたりはいかがでしょうか？

nodchip · 2024-11-30T13:08:54Z

+      return true;
+    }
+
+    for (const auto word : word_dict) {


const auto で受けると文字列のコピーが起こり、処理速度がやや落ちると思います。 const auto& と const 参照で受けるとよいと思います。

nodchip · 2024-11-30T13:11:36Z

+  }
+
+private:
+  bool CheckWordBreak(string word, const set<string>& word_dict, map<string, bool>& word_to_validity) {


文字列のコピーにより遅くなるのを避けるため、 const string& word とするとよいと思います。

nodchip · 2024-11-30T13:19:12Z

+public:
+  bool wordBreak(string s, vector<string>& wordDict) {
+    set<string_view> word_dict(wordDict.begin(), wordDict.end());
+    map<string_view, bool> word_to_validity;


validity という単語が何を表そうとしているのか、直感的ではないように感じました。また、 word とありますが、実際には複数の word が結合されている場合もあると思いました。 substring_to_breakable はいかがでしょうか？

@nodchip
もう少し意味のある変数名を練らないとですね。
substring_to_breakableを使わせていただきました🙇

finish

0453bca

oda reviewed Nov 28, 2024

View reviewed changes

hayashi-ay reviewed Nov 29, 2024

View reviewed changes

philip82148 reviewed Nov 29, 2024

View reviewed changes

oda reviewed Nov 29, 2024

View reviewed changes

frinfo702 reviewed Nov 30, 2024

View reviewed changes

nodchip reviewed Nov 30, 2024

View reviewed changes

mod reflecting review

6ba5f48

Conversation

Ryotaro25 commented Nov 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ryotaro25 Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hayashi-ay left a comment

Choose a reason for hiding this comment

Uh oh!

philip82148 Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philip82148 Nov 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ryotaro25 Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frinfo702 left a comment

Choose a reason for hiding this comment

Uh oh!

frinfo702 Nov 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Ryotaro25 Dec 4, 2024 •

edited

Loading

philip82148 Nov 29, 2024 •

edited

Loading

philip82148 Nov 30, 2024 •

edited

Loading

Ryotaro25 Dec 4, 2024 •

edited

Loading

frinfo702 Nov 30, 2024 •

edited

Loading