Create Number of Islands.md

[irohafternoon]

This Problem
Number of Islands

Next Problem
Max Area of Island

[Fuminiton]

ここの(i, j)は(row, column), (r, c)あたりの方が情報量があると思います。

[irohafternoon]

ありがとうございます。r,cが良さそうなのでこのように書きたいと思います

[Fuminiton]

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

The names of variables (including function parameters) and data members are snake_case (all lowercase, with underscores between words). Data members of classes (but not structs) additionally have trailing underscores. For instance: a_local_variable, a_struct_data_member, a_class_data_member_.

[nodchip]

定数ですので、 kColumnDiff といった変数名でよいと思います。

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

Variables declared constexpr or const, and whose value is fixed for the duration of the program, are named with a leading "k" followed by mixed case.

チームの平均的な書き方に合わせることをお勧めいたします。

[Fuminiton]

定数だとルールが変わるのですね。
失礼しました。

[Fuminiton]

'0'がWATERであることを別で定義しておきたい感があります。

[Fuminiton]

いいと思います。

[Fuminiton]

ここ自分も初めはtravelが良いと思っていましたが、
グラフやツリーの走査としてはtraverseの方が一般的みたいです。

• https://en.wikipedia.org/wiki/Graph_traversal
• https://en.wikipedia.org/wiki/Tree_traversal

[irohafternoon]

リンクもいただいてありがとうございます！
traverseという単語を見たことがなかったのですが、このように使われているのですね

[Fuminiton]

再帰上限があることは認識しておいた方がいいと思います。

[irohafternoon]

ありがとうございます。
https://docs.google.com/document/d/11HV35ADPo9QxJOpJQ24FcZvtvioli770WWdZZDaLOfg/edit?tab=t.0
こちらあたりを読んでみます

[Fuminiton]

横に長くなってしまうので、以下のようにするのもありだと思います。

if (!(0 <= column && column < num_column)) {
    return false;
}
if (!(0 <= row && row < num_row)) {
    return false;
}

[Fuminiton]

true or falseではなく、1 or 0なのが気になりました。

[irohafternoon]

vectorは特殊な内部構造となっており、パフォーマンスに影響がある可能性があるため、避けられるなら避けるようにしているのが理由となります。

[huyfififi]

探索すべき島の判定を関数に切り出す手法いいですね。私はnumIslands()に範囲外にでないように判定する条件を書き下していたのですが、確かに言われてみればnumsIslands()に持つにはやや複雑な感覚もあります。

[huyfififi]

nit 多分このis_valid?という関数は存在しなさそうなので消し忘れかと思います。

[huyfififi]

これは趣味の問題でしょうが、入力で島の地図が与えられていて、島の位置が既知なので、"new island"と呼ぶのは少しだけ不思議な感じがします。私だったらですが、isUnvisitedIsland()のようにするかなと思います。

[irohafternoon]

ありがとうございます、isUnvisitedIsland()ニュアンスが的確に伝わる名前と思いました。
今後使っていきたいと思います！

[huyfififi]

ちなみにですが、Command-query separationという考え方があり、（入力の変数を変更するなど）副作用がある関数(Command)は値を返さないように、逆に値を返す関数(Query)は副作用がないように、関数をデザインすることで、Query関数を見ている時は副作用について考えなくても良くなりメンテナンス性が上がるなどのメリットがあります。

[irohafternoon]

ありがとうございます！このような考えもあるのですね、なるべくこれを守れるように（そしてあくまで比較衡量することを忘れずに）していきたいです

[nodchip]

column_num という変数名の付け方はあまり見ないように思います。 num_columns はしばしば見かけます。こちらは number_of_columns を略して付けていると思います。
column_num という名前ですと、列番号を表しているように見えました。

[irohafternoon]

ありがとうございます。
num　の使い方を修正するようにいたします。

[nodchip]

grid は一つ目の次元が row を表すことが多いと思います。 column_num と row_num の変数名を入れ替えたほうがよいと思います。

[irohafternoon]

ありがとうございます。お恥ずかしながら、column と rowの意味を逆に覚えていました。
修正いたします。

[nodchip]

定数ですので、 kColumnDiff といった変数名でよいと思います。

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

Variables declared constexpr or const, and whose value is fixed for the duration of the program, are named with a leading "k" followed by mixed case.

チームの平均的な書き方に合わせることをお勧めいたします。

[nodchip]

C++ では関数名は lowerCamel で書くことはあまりないように思います。 UpperCamel が多いように思います。

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

Ordinarily, functions should start with a capital letter and have a capital letter for each new word.

こちらもチームの平均的な書き方に合わせることをお勧めいたします。

[irohafternoon]

ありがとうございます。LeetCodeでの関数がlowerCamelですので、これに合わせる形にと考えていました。
実務ではチームのスタイルを確認するようにいたします。

[nodchip]

関数の引数の順番については、入力を先、出力を後に書くという場合があります。

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

When ordering function parameters, put all input-only parameters before any output parameters.

チームの平均的な書き方に合わせることをお勧めいたします。

[irohafternoon]

ありがとうございます。今後is_visited_island のような関数のoutputの対象になるようなものは後に書くようにいたします。

[nodchip]

) と { の間にスペースを空けることをお勧めいたします。

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

void f(bool b) { // Open braces should always have a space before them.

[irohafternoon]

ご指摘ありがとうございます。　チェック漏れが無いように気を付けます

[nodchip]

number_of_ の略だと思います。

[irohafternoon]

ご指摘ありがとうございます。修正いたします。

[austyhooong]

column と rowの定義が間違ってると思います
row -> 行 (grid.size() -> 行の数)
column -> 列

[irohafternoon]

ご指摘の通りです。
お恥ずかしながらrowとcolumnを逆に覚えておりました

[oda]

このコンストラクター欲しいですかね。暗黙のやつでもいいのではないですか。
何も書かないと POD になりますが、特にこの場合はデメリットはないですか。

[irohafternoon]

コメントありがとうございます。
手元のvscode環境(c++17)で動かした際、emplaceでPositionを構築するのに、コントラクタがなくエラーがでたと記憶しており、（LeetCode上では動いたのですが)、環境によってはコントラクタを書かないと動かないのかなと思い、記載いたしました

[oda]

C++20 からみたいですね。

struct p {
    int a, b;
};

int main() {
    std::vector<p> test;
    test.push_back({3, 5});  // from C++11
    test.emplace_back(3, 5);  // from C++20
    // test.push_back(3, 5);  // Error
    // test.emplace_back({3, 5});  // Error
    return 0;
}

型を書いて p{3, 5} という手もあります。

[irohafternoon]

ありがとうございます。
こちらの記事しか調べられなかったのですが、c++20から丸カッコでも{}と同様になったと理解しました
https://onihusube.hatenablog.com/entry/2019/04/06/123202

[oda]

push_back の場合は型が決まっているので、push_back({}); とすると list-initialization in function call が動きます。
https://en.cppreference.com/w/cpp/utility/initializer_list
https://en.cppreference.com/w/cpp/language/list_initialization

emplace_back の場合は、コンストラクターに渡すものなので、{} が与えられても何か分かりません。一方で、要素を並べてもデフォルトコンストラクターが定義されていないというのが C++ 17 まででした。
それが C++20 からは initializing aggregates も使えるようになり、
p x(3, 5); や test.emplace_back(3, 5); も動きます。
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0960r3.html

push_back(3, 5); emplace_back({}); はいずれにしても駄目ですね。

[oda]

別にこれくらい丁寧に書いてもいいとは思うので、時間は15分を超えてもいいんですが、ちょっと長い気はしますね。
https://docs.google.com/document/d/1bjbOSs-Ac0G_cjVzJ2Qd8URoU_0BNirZ8utS3CUAeLE/edit?tab=t.0

Position は pair<int, int> でもいいですし、is_seen_island は、set<pair<int, int>> でもたとえばいいでしょう。
入れる前にしているチェックを出てきたところでチェックしても構わないなど、です。半分くらいになったりしませんか。

[irohafternoon]

@oda
ありがとうございます。
15分以内に書くのが目標のように思ってしまっていましたが、自分の教育やエンジニアリングのための目標から考えるべきでした。今回はこれくらい長く書くなら自分が満足できるぐらいの時間でスムーズにかければそれでよし、という風にしようと思います

#include <set>
#include <vector>
#include <queue>

class Solution {
public:
    int numIslands(const std::vector<std::vector<char>>& grid) {
        if (grid.empty()) {
            return 0;
        }
        std::set<std::pair<int, int>> is_seen_island;
        auto island_count = 0;
        auto num_row = grid.size();
        auto num_column = grid[0].size();
        for (int r = 0; r < num_row; r++) {
            for (int c = 0; c < num_column; c++) {
                if (isNewIsland(r, c, num_row, num_column, grid, is_seen_island)) {
                    island_count++;
                    traverseIsland(r, c, num_row, num_column, grid, is_seen_island);
                }
            }
        }
        return island_count;
    }

private:
    static constexpr int kRowDiff[4] = {1, -1, 0, 0};
    static constexpr int kColumnDiff[4] = {0, 0, 1, -1};
    static constexpr char kSea = '0';
    static bool isNewIsland(int r, int c, int num_row, int num_column,
                            const std::vector<std::vector<char>>& grid,
                            const std::set<std::pair<int, int>>& is_seen_island) {
        if (!(0 <= r && r < num_row) || !(0 <= c && c < num_column)) {
            return false;
        }
        if (is_seen_island.contains({r, c})) {
            return false;
        }
        if (grid[r][c] == kSea) {
            return false;
        }
        return true;
    }
    static void traverseIsland(int start_row, int start_column, int num_row, int num_column,
                                const std::vector<std::vector<char>>& grid,
                                std::set<std::pair<int, int>>& is_seen_island) {
        std::queue<std::pair<int, int>> island_to_visit;
        island_to_visit.emplace(start_row, start_column);
        is_seen_island.insert({start_row, start_column});
        while (!island_to_visit.empty()) {
            auto [row, column] = island_to_visit.front();
            island_to_visit.pop();
            for (int i = 0; i < 4; i++) {
                auto next_row = row + kRowDiff[i];
                auto next_column = column + kColumnDiff[i];
                if (isNewIsland(next_row, next_column, num_row, num_column,
                    grid, is_seen_island)) {
                    island_to_visit.emplace(next_row, next_column);
                    is_seen_island.insert({next_row, next_column});
                }
            }
        }
    }
};

struct をやめて、is_seen_islandをsetに変えてみたところ、これくらいになりました。
コードは10行強減って、13分ほどで描けるようになりましたが、あまり縮まったと感じていません。。。

入れる前にしているチェックを出てきたところでチェックしても構わない

こちらの意図があまり理解できていないと感じています。queueから出るタイミングでチェックをすると、コードを短くできるのでしょうか

[oda]

これくらいでいいんじゃないですか。

while (!island_to_visit.empty()) {
    auto [row, column] = island_to_visit.front();
    if (!isNewIsland(row, column, num_row, num_column, grid, ise_seen_island) {
        continue;
    }
    is_seen_island.insert({row, column});
    island_to_visit.emplace(row, column + 1);
    island_to_visit.emplace(row, column - 1);
    island_to_visit.emplace(row + 1, column);
    island_to_visit.emplace(row - 1, column);
}

[irohafternoon]

ありがとうございます、
①emplaceを4つベタ書きする
②queueに無効な値が入るのは許容して、取り出すときにスキップ
すると、
iff定数を書く部分やdiffを回すfor文も削除でき、emplaceも実質コピーペーストで簡単にかけて短縮できるのですね
BFS部分は別関数にせずにこのような感じにしました

#include <set>
#include <vector>
#include <queue>

class  Solution {
public:
    int numIslands(std::vector<std::vector<char>>& grid) {
        if (grid.empty()) {
            return 0;
        }
        std::set<std::pair<int, int>> is_seen_island;
        auto island_count = 0;
        auto num_row = grid.size();
        auto num_column = grid[0].size();
        for (int r = 0; r < num_row; r++) {
            for (int c = 0; c < num_column; c++) {
                if (isUnvisitedIsland(r, c, num_row, num_column, grid, is_seen_island)) {
                    island_count++;
                    std::queue<std::pair<int, int>> island_to_visit;
                    island_to_visit.emplace(r, c);
                    while (!island_to_visit.empty()) {
                        auto [row, column] = island_to_visit.front();
                        island_to_visit.pop();
                        if (!isUnvisitedIsland(row, column, num_row, num_column, grid, is_seen_island)) {
                            continue;
                        } 
                        is_seen_island.emplace(row, column);
                        island_to_visit.emplace(row + 1, column);
                        island_to_visit.emplace(row - 1, column);
                        island_to_visit.emplace(row, column + 1);
                        island_to_visit.emplace(row, column - 1);
                    }
                }
            }
        }
        return island_count;
    }
private:
    static const char  kSea = '0';
    static bool isUnvisitedIsland(int r, int c, int num_row, int num_column,
                                    const std::vector<std::vector<char>>& grid,
                                    const std::set<std::pair<int, int>>& is_seen_island) {
        if (!(0 <= r && r < num_row) ||!(0 <= c && c < num_column)) {
            return false;
        }
        if (grid[r][c] == kSea) {
            return false;
        }
        if (is_seen_island.contains({r, c})) {
            return false;
        }
        return true;
    }
};