Skip to content
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: redesign ArrayCombination #5181

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

yuvashreenarayanan3
Copy link

@yuvashreenarayanan3 yuvashreenarayanan3 commented May 26, 2024

Related to #5164.

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized it.
  • All filenames are in PascalCase.
  • All functions and variable names follow Java naming conventions.
  • All new algorithms have a URL in their comments that points to Wikipedia or other similar explanations.
  • All new code is formatted with clang-format -i --style=file path/to/your/file.java

@codecov-commenter
Copy link

codecov-commenter commented May 26, 2024

Codecov Report

Attention: Patch coverage is 91.66667% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 38.60%. Comparing base (37c2a96) to head (a991feb).

Files Patch % Lines
...m/thealgorithms/backtracking/ArrayCombination.java 91.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #5181      +/-   ##
============================================
- Coverage     38.60%   38.60%   -0.01%     
  Complexity     2379     2379              
============================================
  Files           516      516              
  Lines         15396    15402       +6     
  Branches       2957     2957              
============================================
+ Hits           5944     5946       +2     
- Misses         9167     9168       +1     
- Partials        285      288       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@vil02 vil02 left a comment

Choose a reason for hiding this comment

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

Thanks for having a look into this class.

import java.util.List;
import java.util.TreeSet;

/**
* Finds all permutations of 1...n of length k
Copy link
Member

Choose a reason for hiding this comment

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

Permutations?

Comment on lines +23 to 25
if (k <= 0 || n < k) {
return new ArrayList<>(); // Return empty list for invalid input
}
Copy link
Member

Choose a reason for hiding this comment

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

There is nothing bad with n >= 0 and k == 0. Please throw IllegalArgumentException if one of the inputs is negative or n < k.

Please add tests checking if the exception is thrown.

Comment on lines +34 to +41
combinations.add(new ArrayList<>(current)); // Copy to avoid modification
return;
}

for (int i = start; i <= n; i++) {
current.add(i);
combine(combinations, current, i + 1, n, k); // Recursive call
current.remove(current.size() - 1); // Backtrack
Copy link
Member

Choose a reason for hiding this comment

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

These comments are not too useful.

Copy link
Member

Choose a reason for hiding this comment

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

Why not to express these tests like that:

package com.thealgorithms.backtracking;

import static org.junit.jupiter.api.Assertions.assertEquals;

import java.util.ArrayList;
import java.util.List;
import java.util.stream.Stream;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

public class ArrayCombinationTest {
    @ParameterizedTest
    @MethodSource("tcStream")
    void testWithRegularInput(final int n, final int k, final List<List<Integer>> expected) {
        assertEquals(expected, ArrayCombination.combination(n, k));
    }

    private static Stream<Arguments> tcStream() {
        return Stream.of(Arguments.of(4, 0, new ArrayList<ArrayList<Integer>>()), Arguments.of(3, 1, List.of(List.of(1), List.of(2), List.of(3))), Arguments.of(4, 2, List.of(List.of(1, 2), List.of(1, 3), List.of(1, 4), List.of(2, 3), List.of(2, 4), List.of(3, 4))));
    }
}

* @param n max value of the array.
* @param k length of combination
* @return a list of all combinations of length k. If k == 0, return null.
* Find all combinations of 1..n using backtracking.
Copy link
Member

Choose a reason for hiding this comment

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

Wat do you think about

Suggested change
* Find all combinations of 1..n using backtracking.
* Find all combinations of 0..n-1 using backtracking.

Clearly this will require changes in the code and tests, but in my opinion it will be much more useful.

Comment on lines +19 to +20
* @return A list of all combinations of length k.
* Returns an empty list if k is 0 or n is less than k.
Copy link
Member

Choose a reason for hiding this comment

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

This will need an update.

@vil02 vil02 changed the title Related to #5164 (Redesign of ArrayCombination) refactor: redesign ArrayCombination May 27, 2024
Copy link

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!

@github-actions github-actions bot added the stale label Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants