Skip to content

322. Coin Change#40

Open
dxxsxsxkx wants to merge 1 commit into139_word_breakfrom
322_coin_change
Open

322. Coin Change#40
dxxsxsxkx wants to merge 1 commit into139_word_breakfrom
322_coin_change

Conversation

@dxxsxsxkx
Copy link
Copy Markdown
Owner

class Solution {
public:
int coinChange(std::vector<int>& coins, int target_amount) {
int NOT_FOUND = std::numeric_limits<int>::max() / 2;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

std::numeric_limits<int>::max() / 2は意味が分かりづらかったです。
std::numeric_limits<int>::max() - 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.

自分は std::numeric_limits::max() / 2 でもそれほど意味が分かりづらいと感じませんでした。また、 std::numeric_limits::max() / 2 にしておくと、 std::numeric_limits::max() / 2 を 2 回足してもオーバーフローしないという性質があり、バグが回避できる場合があると思います。趣味の範囲かもしれません。

class Solution {
public:
int coinChange(std::vector<int>& coins, int target_amount) {
int NOT_FOUND = std::numeric_limits<int>::max() / 2;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

この文脈で「見つからない」という変数名は少し浮いているように思ったので、特別良い案は思い浮かびませんが、UNREACHABLE や UNACHIEVABLE などの方が、達成不可能なコイン枚数だという意味が伝えやすいかなと思いました。


int NOT_FOUND = -1;

std::vector<bool> visited_amount(target_amount + 1, false);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

こちらのコメントをご参照ください。
Hurukawa2121/leetcode#17 (comment)


while (!sum.empty()) {
int size = sum.size();
min_num_coins++;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Google Style Guideでは後置する必要性がなければ前置するとのことです.
(あくまでスタイルの一例なので,チームの慣習やスタイルに合わせることをお勧めします)

Comment on lines +10 to +16
std::vector<bool> visited(amount + 1, false);
visited[0] = true;

std::queue<int> sum;
sum.push(0);

int min_num_coins = 0;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

好みだとは思いますが,私ならここは「初期化」の一塊として捉え,空行を入れることはしないと思いました.

#include <vector>
class Solution {
public:
int coinChange(std::add_cv_t<int>& coins, int amount) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

std::add_cv_t<int>&ではなくstd::vector<int>& でしょうか.(寡聞にして前者を存じ上げませんでしたが,調べた限りconst volatile int&に対応することになるので,全く別物かなと思いました)


int min_num_coins = 0;

while (amounts.empty()) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: !が抜けているようです.

amounts.pop();

for (auto coin : coins) {
int next_amount = amount + coin;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

coin (= coins[i])の範囲は$[1, 2^{31} - 1]$なので,オーバーフローの危険がありそうです.

memo[0] = 0;

for (int i = 1; i <= amount; i++) {
for (auto coin : coins) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Google Style Guideによると

型推論は、型推論によって、そのプロジェクトに馴染みのないコード読者にとっても、コードが読みやすくなることを期待できる場合や、あるいは、型推論によってコードをより安全にできる場合に限って使用します

とのことなので,ここはintで良いと感じました.
(あくまでスタイルの一例なのでご参考までに)

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