-
Notifications
You must be signed in to change notification settings - Fork 19k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: change List<Integer> to LinkedList<Integer> and remove 'n < 0' #5439
base: master
Are you sure you want to change the base?
Conversation
xizhengl
commented
Aug 31, 2024
•
edited
Loading
edited
- change List to LinkedList and remove 'n < 0'
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List doesn't seem to have a removeLast method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has. In code below, you can see that it already used. And code compile and pass tests.