Skip to content

300. Longest Increasing Subsequence#28

Open
seal-azarashi wants to merge 18 commits intomainfrom
longest-increasing-subsequence
Open

300. Longest Increasing Subsequence#28
seal-azarashi wants to merge 18 commits intomainfrom
longest-increasing-subsequence

Conversation

@seal-azarashi
Copy link
Owner

*/
class Solution {
public int lengthOfLIS(int[] nums) {
int[] lis = new int[nums.length];
Copy link

Choose a reason for hiding this comment

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

maxLengths という変数名はいかがでしょうか?

Copy link
Owner Author

Choose a reason for hiding this comment

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

ありがとうございます。そちらのほうが優れた命名だと思いました。修正して Step 4 に追記しました: 953ac32

return 0;
}

int[] minLISTailValues = new int[nums.length];
Copy link

Choose a reason for hiding this comment

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

これ自体が LIS ですので、変数名を lis にしても良いと思いました。

Copy link
Owner Author

Choose a reason for hiding this comment

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

ありがとうございます。たしかにそうですね。修正して step 4 に追記しました: b6c9a2c

Copy link

Choose a reason for hiding this comment

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

以下のような議論がありました。
https://github.com/Yoshiki-Iwasa/Arai60/pull/46/files/56e8cf4d4efc42c5784108191d1e5fc615de9206#r1716128766

これ自体がLISなのか気になったので、コメントさせていただきました。

}

// is: increasing subsequence
ArrayList<Integer> isMinTailValues = new ArrayList<>();
Copy link

Choose a reason for hiding this comment

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

変数名が is で始まっていると、 boolean 変数または boolean を返す関数の名前のように感じられます。避けたほうが無難だと思います。

Copy link
Owner Author

Choose a reason for hiding this comment

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

苦し紛れにコメント付けて補足してましたが、やはり誤解しますよね。修正案を考えてみます。

Copy link
Owner Author

Choose a reason for hiding this comment

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

上の指摘にならって lis に修正しました: b6c9a2c

- それぞれ何を指すインデックスなのか、ループの内容を把握しないと理解出来なく読みづらいため
- nodchip さんのレビューを参考にしました: https://github.com/shining-ai/leetcode/pull/31#discussion_r1536792178
- Arrays に fill() というメソッドがあったので使ってみる

Copy link

Choose a reason for hiding this comment

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

ありがとうございます!
プルリクエストは一通り目を通してますが、 Discord の過去の投稿は全部確認するのが難しいので助かります。

}
}
}
int longestIncreasingSubsequence = 1;
Copy link

Choose a reason for hiding this comment

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

これはsequenceじゃなくてsequenceの長さですね。どう修飾するかはともかく、何を表す変数かがずれていると結構混乱するかもなと思いました。個人的にはmaxLengthくらいでもいい気がしてます。

Copy link
Owner Author

Choose a reason for hiding this comment

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

問題文に "return the length of the longest strictly increasing subsequence" とあったのでそのまま変数名にしていましたが、確かに length であることが名前に含まれていないのは良くないですね。
longestIncreasingSubsequenceLength とするのも流石に冗長ですし、仰るとおり maxLength に修正して step 4 に書きました。

return longestIncreasingSubsequence;
}

private int findMaxLengthWithNewTail(int[] nums, int[] maxLengthAsTail, int tailIndex) {
Copy link

Choose a reason for hiding this comment

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

関数にしない方が分かりやすいんじゃないかなという気がしました。この処理を分ける旨味もなさそうですし。

for (int i = 1; i < nums.length; i++) {
    for (int j = 0; j < i; j++) {
        if (nums[j] < nums[i]) {
            maxLengthAsTail[i] = Math.max(maxLengthAsTail[i], maxLengthAsTail[j] + 1);
        }
    }
}

Copy link
Owner Author

Choose a reason for hiding this comment

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

ご指摘ありがとうございます。たしかにそうですね。可読性のためネストが3までいったらとりあえず変数に出すことにしていましたが、この場合はむしろ読みづらくなってしまっていたなと改めて思いました。
修正して step 4 に書きました。


### 二分探索を用いた O(n log n) の解法

Java の標準ライブラリの二分探索メソッド、返り値がどうなるか理解するのがしんどかった... 要素が見つからなかったことを示すために負の値にするのはいいんだけど、なんで挿入位置を 1-indexed の値で返すようにしたんだろう🤔
Copy link

Choose a reason for hiding this comment

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

調べてないですがbit反転させれば挿入位置になるからなんですかねぇ...

Choose a reason for hiding this comment

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

index of the search key, if it is contained in the array; otherwise, (-(insertion point) - 1). The insertion point is defined as the point at which the key would be inserted into the array: the index of the first element greater than the key, or a.length if all elements in the array are less than the specified key. Note that this guarantees that the return value will be >= 0 if and only if the key is found.

-(insertion point) - 1(1-indexedとは?)ですね。要素が見つからず挿入位置が0の時と要素が見つかった場合を区別するためではないですか。

Copy link

Choose a reason for hiding this comment

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

https://stackoverflow.com/a/57911407/7503647
こういう回答はありました。たしかに0の挿入位置を表現しないといけないのはありますね

return 0;
}

int[] maxLengthAsTail = new int[nums.length];

Choose a reason for hiding this comment

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

ここはstep3のようにsequenceLengthくらいにしてコメントで詳細を書く方が良いと思いました。
「iで終わる単調増加する部分列の中で最大の長さ」を一つの変数名で表すのはなかなか難しいですね...

Copy link
Owner Author

Choose a reason for hiding this comment

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

そうですね、役割を正確に名前に反映させようと思うと長くなってしまうし、中途半端に削っても意図が伝わらなくなるばかりだなと難しさを感じていました...
名前を変え、コメントで詳細を書くように step 4 で修正しました。

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.

7 participants