1011. Capacity To Ship Packages Within D Days #44
Conversation
| for (int weight : weights) { | ||
| if (load + weight <= capacity) { | ||
| load += weight; | ||
| } |
| load += weight; | ||
| } | ||
| else { | ||
| if (weight > capacity) { |
There was a problem hiding this comment.
迷いました。実世界で手作業で船に積んでいくとしたらどういう順番でチェックするだろうか、ということを考えてここに置いてみましたが、最初に置いてもいいと思います。
そもそも下限を荷物の最大重量から始めているので省くのも選択肢に入るかもしれません。
There was a problem hiding this comment.
そもそも下限を荷物の最大重量から始めている
確かにそうですね。省いても良いと思います。
| } | ||
| } | ||
| return true; | ||
| }; |
There was a problem hiding this comment.
冒頭に書かれたコードの方が読みやすかったです。少なくともここに空行があると良いかなと思います。
| return true; | ||
| }; | ||
| int low = *max_element(weights.begin(), weights.end()); | ||
| int high = accumulate(weights.begin(), weights.end(), 0); |
There was a problem hiding this comment.
c++17から reduce というものがあるそうです。
https://cpprefjp.github.io/reference/numeric/reduce.html
There was a problem hiding this comment.
どっちでもいいかなと思ってたのですが、色々見てみると今回はreduceのほうが良さそうですね！ありがとうございます。
| if (weight > capacity) { | ||
| return false; | ||
| } | ||
| if (++current_day > days) { |
There was a problem hiding this comment.
式の中に副作用があると同時に色々やっていてわかりにくいという意見もありそうですね(練習マニュアルにもあった気はします)
これくらいならそんなに混乱しないかもしれないですが
| public: | ||
| int shipWithinDays(vector<int>& weights, int days) { | ||
| int capacity_min = *max_element(weights.begin(), weights.end()); | ||
| int capacity_max = accumulate(weights.begin(), weights.end(), 0); |
There was a problem hiding this comment.
最大値を探すものはstd::max_elementというのがありますが、sumは直接的なものはないようです。
| public: | ||
| int shipWithinDays(vector<int>& weights, int days) { | ||
| auto can_ship = [&weights, days](int capacity) -> bool { | ||
| int current_day = 1; |
There was a problem hiding this comment.
currentというよりは所要日数の方が実態に近いかと思いました。required_daysなどどうでしょうか。
There was a problem hiding this comment.
そちらの方がわかりやすいかもしれませんね。実際に積んでいく状況をイメージして書くのと、もしも積んでいったらどれぐらい必要だろうか、という計算をするイメージで書くので変わってくるかもしれません。(自分は前者のイメージでした)
| return capacity_min; | ||
| } | ||
| private: | ||
| bool can_ship(vector<int>& weights, int days, int capacity) { |
There was a problem hiding this comment.
複合型で、関数の内部で変更されない変数の型には const を付けることをおすすめします。ついていないと、読み手が、変数が関数内で変更されることを前提に読んでしまい、混乱してしまう場合があると思います。
| return capacity_min; | ||
| } | ||
| private: | ||
| bool can_ship(vector<int>& weights, int days, int capacity) { |
There was a problem hiding this comment.
C++ では関数名を lower_snake とするのは、あまり見ないように思います。 UpperCamel をよく見かけます。
参考までにスタイルガイドへのリンクを貼ります。
https://google.github.io/styleguide/cppguide.html#Function_Names
Ordinarily, functions follow PascalCase: start with a capital letter and have a capital letter for each new word.
上記のスタイルガイドは唯一絶対のルールではなく、複数あるスタイルガイドの一つに過ぎないということを念頭に置くことをお勧めします。また、所属するチームにより何が良いとされているかは変わります。自分の中で良い書き方の基準を持ちつつ、チームの平均的な書き方で書くことをお勧めいたします。
There was a problem hiding this comment.
ありがとうございます。leetcodeのテンプレートに設定されているデフォルトに合わせてlowerCamelで書いてみます。
| class Solution { | ||
| public: | ||
| int shipWithinDays(vector<int>& weights, int days) { | ||
| auto can_ship = [&weights, days](int capacity) -> bool { |
There was a problem hiding this comment.
数行を超えるラムダ関数は、ネストが深くなったり、ラムダ関数の前後を追う際に目の移動量が多くなったりと、読みにくく感じます。メンバ関数にくくりだしたほうが良いと思います。
並列計算フレームワークに渡すロジックを記述する際や、 UI のイベントハンドラーを大量に書かなければならない場合等、ラムダ関数にせざるを得ない場合もあるとは思います。
| int load = 0; | ||
| for (int weight : weights) { | ||
| if (load + weight <= capacity) { | ||
| load += weight; |
There was a problem hiding this comment.
continue して else のネストをなくしたい気がします (全体的にネストが深いので)。
| if (weight > capacity) { | ||
| return false; |
There was a problem hiding this comment.
can_ship を独立の関数と考えるとこの処理は確かにしたいのですが、low を weights の max としているのでなくても動きますね。
https://leetcode.com/problems/capacity-to-ship-packages-within-d-days/description/