refactor: change List<Integer> to LinkedList<Integer> and remove 'n < 0' #5439
Conversation
xizhengl
commented
Aug 31, 2024
•
xizhengl
commented
- change List to LinkedList and remove 'n < 0'
xizhengl
requested review from
yanglbme,
vil02,
BamaCharanChhandogi,
alxkm and
siriak
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5439 +/- ##
============================================
+ Coverage 50.97% 50.98% +0.01%
+ Complexity 3159 3158 -1
============================================
Files 523 523
Lines 15104 15104
Branches 2874 2874
============================================
+ Hits 7699 7701 +2
+ Misses 7084 7083 -1
+ Partials 321 320 -1 ☔ View full report in Codecov by Sentry. |
| @@ -18,16 +19,16 @@ private ArrayCombination() { | |||
| * @return A list of all combinations of length k. | |||
| */ | |||
| public static List<List<Integer>> combination(int n, int k) { | |||
| if (n < 0 || k < 0 || k > n) { | |||
| if (k < 0 || k > n) { | |||
There was a problem hiding this comment.
The n < 0 check is important because it ensures that the input values are valid within the context of combinations, prevents undefined behavior.
There was a problem hiding this comment.
It looks like if you remove this check everything will work correctly. And you're right, we can remove it. But this check provides readable constraints and with this check the code becomes more readable.
| return combinations; | ||
| } | ||
|
|
||
| private static void combine(List<List<Integer>> combinations, List<Integer> current, int start, int n, int k) { | ||
| private static void combine(List<List<Integer>> combinations, LinkedList<Integer> current, int start, int n, int k) { |
There was a problem hiding this comment.
Using LinkedList instead of List in method parameters is considered bad practice as it provides tight coupling, and issues with extensibility.
There was a problem hiding this comment.
List doesn't seem to have a removeLast method
There was a problem hiding this comment.
It has. In code below, you can see that it already used. And code compile and pass tests.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution! |
|
Please reopen this pull request once you have made the required changes. If you need help, feel free to ask in our Discord server or ping one of the maintainers here. Thank you for your contribution! |
