121. Best Time to Buy and Sell Stock#35
Conversation
| return 0; | ||
| } | ||
| int max_profit = -1; | ||
| vector<int> min_price(size, 100000); |
There was a problem hiding this comment.
各要素の初期値を明示的に設定したことと、その初期値に 100000 を選んだことの理由はなんでしょうか?
個人的には両者ともあまり積極的な理由が分かりませんでした。
There was a problem hiding this comment.
一応priceの値の最大値が制約では10 ^ 4なのでそれより大きなてきとうな値にしました。
There was a problem hiding this comment.
3点ほど、設計や実装の観点でコメントしますね。
-
制約を仮定した実装について
(LeetCodeでは制約が保証されますが) 実際の開発では、入力がいつでも制約を満たしてくれるとは限らないため、制約を仮定しすぎない実装を心がけたほうが安全です。以下のPRのコメントやコメントの引用文書が参考になると思います。 -
「適当な大きな値」を初期値にすることについて
"それより大きなてきとうな値にする" というアプローチは、マジックナンバーとなりやすく、将来的なバグを隠蔽する要因になるリスクがあります。(過去にそれで痛い目を見た経験があります…!)
以下のセクションに類似の議論へのリンクがあるので、ぜひ一度目を通してみてください。 -
min_priceの不要な初期値について
より今回の実装に具体的な話になりますが、min_priceに設定した初期値は、実際の計算の前に必ず上書きされるため不必要になっています。
// prices[0] で上書きされるため、ここでの初期値は処理に影響しない
min_price[0] = prices[0];
for (int i = 1; i < size; i++) {
// ループ内でも常に上書きされるため、初期値は使われない
min_price[i] = min(min_price[i - 1], prices[i]);
// (以下略)
}処理に影響しない初期値やマジックナンバーが残っていると、コードの読み手は「この初期値には何か特別な意味があるのでは?」と考えながら読むことになり、認知負荷が無駄に上がってしまいます。読み手への配慮(可読性向上)のためにも、不要な初期値は省くことをおすすめします!
There was a problem hiding this comment.
1については了解です!
放置、Exception を投げる、プログラムを止める、特殊な値を返すなどから状況にあったものを選べるようにしたいと思います。
2に関しては適当な名前を付けた定数でマジックナンバーを準備してそれを代入しておけばいいのでしょうか?
3に関しても了解です!
よく見たら確かにforループで値が上書きされていて初期値は使ってなさそうですね。そこまで深く考えずに初期値を設定してしまっていました。
There was a problem hiding this comment.
2に関しては適当な名前を付けた定数でマジックナンバーを準備してそれを代入しておけばいいのでしょうか?
うーん,「とりあえずこれやっとけばどの状況でも100点!」みたいな方針はないですが,まずはそれで良いと思います.(「わかりやすさ」って結構時と場合によるものなのと,その値が内部利用だけなのか/外部公開してこのメソッドなりを呼び出す側もつかうのか,を始めとする様々な要因が関係しうると思います.)
最初にお渡ししたリンク先で議論されている内容とか,ディスコードで「マジックナンバー」なり「定数」なりを調べると似た議論が拾えるかもしれません.
とりあえず,他の人のコードはたくさん読むと良いと思います.
There was a problem hiding this comment.
そうですね、他の方のコードをたくさん読むのはよさそうです。
そうですね!見てみます。
最初にお渡ししたリンク先で議論されている内容とか,ディスコードで「マジックナンバー」なり「定数」なりを調べると似た議論が拾えるかもしれません.
やりすぎな場合はコメントで軽く意味を書いておくだけというのでもいいようですね。
There was a problem hiding this comment.
私もマジックナンバー気になりました。
今後変わる可能性がある定数を定義した時はなぜその値にしたのかをコメントして、後で別の人が見たときにどういう基準でその値を採用したかがわかると良いと思っています。
今回であれば入力上限に関わる数字なので、"hogehogeな背景で入力が10^4を超えることはないので100000にする"ぐらいのコメントがあると、後からここの上限が2倍になりそうだけど値を変えたほうが良いか?いや10倍の余裕で初期化しているから大丈夫そうだみたいな判断がしやすいです。
LeetCodeの練習で毎回それをやるのも冗長なので、毎回はやらなくても良いと個人的には思っています。
(定数化はしたほうがよいが、"leetcodeの設定で入力上限がhogehogeのため"と毎回コメントするのはあんまり意味のないトレーニングぐらいのイメージです。)
There was a problem hiding this comment.
コメントありがとうございます。たしかにそういったコメントがあると後で変更の判断がしやすそうですね。
今回であれば入力上限に関わる数字なので、"hogehogeな背景で入力が10^4を超えることはないので100000にする"ぐらいのコメントがあると、後からここの上限が2倍になりそうだけど値を変えたほうが良いか?いや10倍の余裕で初期化しているから大丈夫そうだみたいな判断がしやすいです。
| if (size == 1) { | ||
| return 0; | ||
| } | ||
| int max_profit = -1; |
There was a problem hiding this comment.
max_profit の初期値に-1を設定した意図はなんでしょうか?
for文の各ループにおいて必ず0以上の値に更新されるので、0でなく-1を初期値にする必要性が薄いように感じました。
There was a problem hiding this comment.
コメントありがとうございます。そうですね、ここは0でよさそうです。
| return 0; | ||
| } | ||
|
|
||
| int before_buy = 0; |
There was a problem hiding this comment.
before_buy は今回の実装では更新されない定数なので、定数の命名規則に従った名前にしたいと思いました。
There was a problem hiding this comment.
それはそれとして、個人的には、あえてこの定数を置く必要性を感じませんでした、
holding = max(holding, -price);
で十分な気がします。
There was a problem hiding this comment.
たしかにbefore_buyはあえて置く必要はなさそうです。
| } | ||
|
|
||
| int before_buy = 0; | ||
| int holding = -100000; |
There was a problem hiding this comment.
この holding の初期値だとpricesの要素がすべて 100000 を超える場合に正しい答えを計算できなくなると思います。
幅広い入力に対して正しい答えを計算できる値で初期化するべきだと思います。
直前でpricesの長さをチェックしていることも加味すると、-prices[0] を初期値にするなどが候補になるでしょうか。
There was a problem hiding this comment.
一応priceの値の最大値が制約では10 ^ 4なのでそれより大きなてきとうな値としましたが、holding = -*max_element(prices.begin(), prices.end())とかでもいいかもしれないですね。
There was a problem hiding this comment.
適当な初期値を設定するためだけに、max_elementは好ましくないと思います。
There was a problem hiding this comment.
たしかに無駄に遅くなってしまいそうですね。
| ### step1 | ||
|
|
||
| for文を回しながら、それまでの最小値を保存しておいてそれを今売ったらいくらになるかというののmaxを毎回更新して解いた。 | ||
|
|
||
| ### step2 |
There was a problem hiding this comment.
step1からstep2で空間計算量の改善がされていますが、そのあたりの気づきや思考の変化などをメモしていただけると、GitHubでのレビューがより有意義になりそうです!
| if (prices.size() == 1) { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
この問題の場合は1が特殊ケースなので、if文で要素数1の場合を特別に処理する書き方は良さそうだなと思いました!
There was a problem hiding this comment.
問題の制約ではありえないですが、空配列が渡されたときはどうするかとかも考えると良さそうです。
自分はprices.size() < 1は書きたくなくて、入力として想定しているけど仕様上(ある日に買って別の日に売る前提なので株価が1日分しかないのはおかしい)は異常な値とそもそも入力を想定していない値では別に処理したいなというきもちがあるからです。
| return 0; | ||
| } | ||
| int max_profit = -1; | ||
| vector<int> min_price(size, 100000); |
There was a problem hiding this comment.
私もマジックナンバー気になりました。
今後変わる可能性がある定数を定義した時はなぜその値にしたのかをコメントして、後で別の人が見たときにどういう基準でその値を採用したかがわかると良いと思っています。
今回であれば入力上限に関わる数字なので、"hogehogeな背景で入力が10^4を超えることはないので100000にする"ぐらいのコメントがあると、後からここの上限が2倍になりそうだけど値を変えたほうが良いか?いや10倍の余裕で初期化しているから大丈夫そうだみたいな判断がしやすいです。
LeetCodeの練習で毎回それをやるのも冗長なので、毎回はやらなくても良いと個人的には思っています。
(定数化はしたほうがよいが、"leetcodeの設定で入力上限がhogehogeのため"と毎回コメントするのはあんまり意味のないトレーニングぐらいのイメージです。)
この問題:https://leetcode.com/problems/best-time-to-buy-and-sell-stock/description/
次に解く問題:https://leetcode.com/problems/best-time-to-buy-and-sell-stock-ii/