322. Coin Change by Ryotaro25 · Pull Request #44 · Ryotaro25/leetcode_first60

Ryotaro25 · 2024-12-02T16:11:27Z

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

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

備考

次に解く問題の予告
35. Search Insert Position

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

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

oda · 2024-12-03T08:12:18Z

+  int coinChange(vector<int>& coins, int target_amount) {
+    vector<bool> visited(target_amount + 1, false);
+
+    queue<NumCoinsAndAmount> num_coins_and_amount;


私は、個人的には pair<int, int> のほうが読みやすいかなと思いますが、これは色々意見があるでしょう。

oda · 2024-12-03T08:14:27Z

+          continue;
+        }
+
+        int next_amount = amount + coin_val; // ここでintのrangeを超える場合がある


このコメントは、どちらかというと、「このため上で引き算によって先に判別した」という意図でしょうか。

@oda
レビューありがとうございます。
見返してみて、いきなりこのコメントが出てくると意味が分からないですね。
bfs_step2.cppにコメントを追加しました。

oda · 2024-12-03T08:17:13Z

+
+    num_min_coins[0] = 0;
+    for (int amount = 1; amount <= target_amount; amount++) {
+      for (const auto& coin_val : coins) {


私は、これは単に int のほうが好みです。

64bit CPUだとintが4バイトでreferenceが8バイトなのでintの方が良いのかなと思います。コンパイラ側で最適化などあるかもしれないですが。（詳しくないので変なこと言ってたら教えて下さい）

@oda @hayashi-ay
レビューありがとうございます。
林さんの指摘の通りreferenceにすると追加で4バイト使用することになるので、あえてintを使わない選択はしないほうが良さそうでした。intに変更しました。

oda · 2024-12-03T08:22:30Z

+https://github.com/goto-untrapped/Arai60/pull/34
+https://github.com/sakupan102/arai60-practice/pull/41
+## Discorなど
+


どこかでいいと思うのですが、C++ はある程度大規模になると .cc と .h に分けて書くのが普通です。

宣言と実体を分けて書く練習はどこかでしたほうがいいでしょう。
https://source.chromium.org/gn/gn/+/main:src/base/files/file.cc

「メモ」
c++で用いるファイルの種類
https://qiita.com/Nabetani/items/9345f764c716dcf7c3f6
ファイル分割に関するグーグルガイド
https://google.github.io/styleguide/cppguide.html#Header_Files

@oda
頂いた例とグーグルガイドを見ながらファイルを分けてみました。
新たにmainディレクトリを作成し.hと.ccファイルに分割したものをgitに上げました🙇‍♂️

hayashi-ay · 2024-12-03T09:14:41Z

+  int coinChange(vector<int>& coins, int target_amount) {
+    vector<int> num_min_coins(target_amount + 1, UNDEFINED);
+
+    num_min_coins[0] = 0;


英語の語順としては、min_num_coinsの方が自然だと思います。

https://github.com/hayashi-ay/leetcode/pull/68/files#r1546152560

あとは、min_coinsでも十分伝わるような気がします

hayashi-ay · 2024-12-03T09:24:55Z

+  }
+
+private:
+  static constexpr int UNDEFINED = std::numeric_limits<int>::max();


UNDEFINEDという定数を定義するのにすこし違和感があります。UNMAKABLE、UNREACHABLEなどの方がより意図が明確になるかなと思います。

確かにUNREACHABLEの方がより正確だと思いましたのでこちらを使いました！ありがとうございます。

nittoco · 2024-12-04T12:57:24Z

+    if (num_min_coins.back() != UNDEFINED) {
+      return num_min_coins.back();
+    }
+    return -1;


そこまでやるかは微妙ですが、自分なら-1を変数で定義してマジックナンバーぽくしないようにすると思います。

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

If that amount of money cannot be made up by any combination of the coins, return -1.

こうあったのでそこまで考えておりませんでした。UNACHIVABLEとして定義してみました。

nittoco · 2024-12-04T13:20:10Z

+        visited[next_amount] = true;
+        num_coins_and_amount.push({num_coins + 1, next_amount});


細かくて恐縮ですが、L7~8ではpushした後にvisitedを更新してますが、こちらでは逆ですね

@nittoco
統一感なかったですね。指摘ありがとうございます。修正しました。

Yoshiki-Iwasa · 2024-12-05T00:54:10Z

+private:
+  static constexpr int UNDEFINED = std::numeric_limits<int>::max();
+};


これはSolutionの先頭においたほうがいいんじゃないでしょうか
C++のお作法知らないのですが、基本的に上から下に読んでそのまま理解できることが望ましいと思います

半分肯定ですね。プロダクションコードだと、.h, .cc に普通は分けるんです。
https://source.chromium.org/gn/gn/+/main:src/base/files/file.h シグネーチャー等。
https://source.chromium.org/gn/gn/+/main:src/base/files/file.cc 実体。
一部抜き出すと .h は下のような感じです。そして、実装は .cc に回します。

namespace base { class File { public: File(); void Initialize(const FilePath& path, uint32_t flags); void Close(); private: void DoInitialize(const FilePath& path, uint32_t flags); File(const File&) = delete; }; }

こっちが .cc 側の抜き出し。

#include "base/files/file.h" namespace base { void File::Initialize(const FilePath& path, uint32_t flags) { if (path.ReferencesParent()) { #if defined(OS_WIN) ::SetLastError(ERROR_ACCESS_DENIED); #elif defined(OS_POSIX) || defined(OS_FUCHSIA) errno = EACCES; #else #error Unsupported platform #endif error_details_ = FILE_ERROR_ACCESS_DENIED; return; } DoInitialize(path, flags); } } // namespace base

なので本当はこれに慣れた状況にしないといけません。

元の質問に戻ると、class 定義は .h に書かれてどういうクラスであるだけが書かれているので、外からのインターフェースが前で(サイズを決めるのに必要な) private 情報は後ろに来ます。ただ、普通は実体がさらに後ろに回っているので、半分は肯定ですね。

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

ヘッダーファイルと実態を分ける等は知りませんでした。
下記のガイドに従ってprivateを後半に持ってきておりました。
https://google.github.io/styleguide/cppguide.html#Declaration_Order

前に共有したものはコンパイルできておりませんでした。
コンパイルできたものはこちらになります。
d5c48dd

hayashi-ay · 2024-12-11T01:48:16Z

@@ -0,0 +1,29 @@
+#include "322.CoinChange/main/step4.h"


あれ？Step4のコードはコンパイルできましたか？

@hayashi-ay
分割してみただけで確認しておりませんでした。コンパイル自体初めてなのですがやってみます。

たぶんコンパイルできないです。何箇所か問題があると思いますが、試してみてください。
ぱっと気になるところだと、ヘッダファイルのパス、cc側でのクラス定義の実装部分ですかね？Makefile（CMakeLists）なども軽く触ってみても良いかもしれないです。

@hayashi-ay
長らく空いてしまい、また後回しにしてしまい申し訳ございません。
なんとかコンパイルできるところまで完了しました。
d5c48dd

これまでLeetCode用に書いてきたものと大きく異なりまだ理解が出来ておりません。
google guideには下記のように方針がございましたがもう少し初学者向けに何をどこに書くのか書かれたものがございましたら教えていただきたいです🙇‍♂️

Header files should be self-contained (compile on their own) and end in .h. Non-header files that are meant for inclusion should end in .inc and be used sparingly.

https://google.github.io/styleguide/cppguide.html#Self_contained_Headers

TORUS0818 · 2024-12-20T21:51:54Z

+      // ここにvisited[amount] = true;をおくとMemory Limit Exceeded
+      // 各コインを探索した後でvisitedが更新されるので


以下は通るのですが、こういう話とは違いますか？

class Solution { public: int coinChange(vector<int>& coins, int target_amount) { vector<bool> visited(target_amount + 1, false); queue<NumCoinsAndAmount> num_coins_and_amount; num_coins_and_amount.push({0, 0}); // visited[0] = true; while (!num_coins_and_amount.empty()) { auto [num_coins, amount] = num_coins_and_amount.front(); num_coins_and_amount.pop(); if (amount == target_amount) { return num_coins; } // ここにvisited[amount] = true;をおくとMemory Limit Exceeded // 各コインを探索した後でvisitedが更新されるので if (visited[amount]) { continue; } visited[amount] = true; for (int coin_val : coins) { // target_amountを超えないか確認 // next_amount計算時にオーバーフローが発生するのを防ぐため if (amount > target_amount - coin_val) { continue; } int next_amount = amount + coin_val; num_coins_and_amount.push({num_coins + 1, next_amount}); } } return -1; } private: struct NumCoinsAndAmount { int num_coins; int amount; }; };

Ryotaro25 · 2025-02-07T14:43:27Z

Compilerの関係でディレクトリ名を変更しました。
d5c48dd

nodchip · 2025-02-11T05:15:02Z

+      // ここにvisited[amount] = true;をおくとMemory Limit Exceeded
+      // 各コインを探索した後でvisitedが更新されるので
+
+      for (const auto& coin_val : coins) {


変数名は coin で十分伝わるように思いました。

また、参照で受けると、間接アドレッシングで値にアクセスするコードが生成される場合があり、無駄な処理が行われる場合があります。レジスターサイズと比べて大きくない値については、値型で受け取ることをお勧めいたします。

詳しくは「Effective C++ 第3版」の「20項　値渡しよりconst参照渡しを使おう」をご覧ください。 (自分は第 2 版しか読んでおらず、この項の次の項に値渡しを使うことが書かれていました。第 3 版はこの項に値渡しを使う場合のことが書かれているのではないかと予想します。)

nodchip · 2025-02-11T05:18:29Z

+class Solution {
+public:
+  int coinChange(vector<int>& coins, int target_amount) {
+    vector<int> num_min_coins(target_amount + 1, UNDEFINED);


num_min_coins という名前ですと、 number of minimum coins といった英語句の省略形のように見え、意味が通らないように思います。min_coins で十分意味が通り、かつシンプルになると思います。

nodchip · 2025-02-11T05:21:13Z

+          continue;
+        }
+        visited[next_amount] = true;
+        num_coins_and_amount.push({num_coins + 1, next_amount});


next_amount == target_amount の判定をここで行ったほうが、 push() pop() の回数が減り、処理が少し軽くなると思います。ただ、ネストが深くなるため、一長一短だとも思います。

finish

4fc643c

oda reviewed Dec 3, 2024

View reviewed changes

hayashi-ay reviewed Dec 3, 2024

View reviewed changes

nittoco reviewed Dec 4, 2024

View reviewed changes

Yoshiki-Iwasa reviewed Dec 5, 2024

View reviewed changes

mod reflecting review

075fdd6

hayashi-ay reviewed Dec 11, 2024

View reviewed changes

TORUS0818 reviewed Dec 20, 2024

View reviewed changes

mod dir name and add compliked file

d5c48dd

changed dir name

4dacd80

nodchip reviewed Feb 11, 2025

View reviewed changes

mura0086 mentioned this pull request Apr 8, 2025

Create 560. Subarray Sum Equals K.md mura0086/arai60#20

Open

added top down solution

50bc028

nodchip mentioned this pull request Aug 2, 2025

560. Subarray Sm Equals K maeken4/Arai60#15

Open

		visited[next_amount] = true;
		num_coins_and_amount.push({num_coins + 1, next_amount});

		// ここにvisited[amount] = true;をおくとMemory Limit Exceeded
		// 各コインを探索した後でvisitedが更新されるので

Conversation

Ryotaro25 commented Dec 2, 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!

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

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

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 commented Feb 7, 2025

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants