Skip to content

Add tests SumOfSubset #5021

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

Merged
merged 3 commits into from
Jan 26, 2024
Merged

Add tests SumOfSubset #5021

merged 3 commits into from
Jan 26, 2024

Conversation

bhishma620
Copy link
Contributor

This pull request introduces the implementation of the "Sum of Subset" algorithm in the file sum_of_subset.java. The Sum of Subset problem involves finding subsets of a given set whose sum matches a specific target sum.It Reduces time complexity from O(2^N) to O(n^2) by using extra space O(N).

@vil02
Copy link
Member

vil02 commented Jan 21, 2024

@bhishma620 thanks for your contribution. This case is quite delicate - you are changing the code, which is not tested. Therefore I suggest the following approach:

  • add tests for existing implementation,
  • change the implementation of the algorithms and make sure that the old tests pass.

Feel free to do each of the steps in separate pull-requests.

Besides that, please make sure that all of the CI checks pass. If you have problems with clang-format, please have a look here: #4438 (comment)

In case of any questions: please do not hesitate - just ask them.

@bhishma620
Copy link
Contributor Author

bhishma620 commented Jan 21, 2024

@vil02

Can you tell me how can i add tests ?
You said that to change the implementation of my algorithm , is there any problem with that algorithm?

@bhishma620 bhishma620 force-pushed the dp branch 4 times, most recently from b41962b to 73fb041 Compare January 22, 2024 11:42
@vil02
Copy link
Member

vil02 commented Jan 22, 2024

Can you tell me how can i add tests?

You did quite well. Please have a look on the review (this is only the 1st iteration).

You said that to change the implementation of my algorithm , is there any problem with that algorithm?

I am not sure if there is a problem. But to be safe: always write test first before touching the implementation.

Please make sure that the CI checks pass.

@bhishma620
Copy link
Contributor Author

Not able to pass Build test for Test file , how to resolve ?

@bhishma620 bhishma620 force-pushed the dp branch 3 times, most recently from fa99466 to 1d0970e Compare January 23, 2024 20:04
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.

The main step - adding tests - is done. Few small things are left.

You can also completely remove the main method from Sum_Of_Subset.

@bhishma620
Copy link
Contributor Author

can you check now, i forgot to remove the main method from the SubOfSubset, is it necessary?

@vil02
Copy link
Member

vil02 commented Jan 24, 2024

can you check now, i forgot to remove the main method from the SubOfSubset, is it necessary?

There is not reason here to have the main method.

@bhishma620
Copy link
Contributor Author

Can you check now?

@vil02 vil02 changed the title Updated Sum of Subset solution Add tests SumOfSubset Jan 26, 2024
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.

@bhishma620: looks great! Please open a new pull-request with your original improvements of SumOfSubset. You can mention this PR there (cf. docs).

Thanks!

@vil02 vil02 merged commit 55f08cc into TheAlgorithms:master Jan 26, 2024
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.

2 participants