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.

Sign up for GitHub

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

Jump to bottom

Refactor Properties.isRandomDNA(String) - Issue #73 #97

Merged
merged 16 commits into from
Oct 20, 2022
Merged

Refactor Properties.isRandomDNA(String) - Issue #73 #97

merged 16 commits into from
Oct 20, 2022

Conversation

yeehawbeans
Copy link
Contributor

@yeehawbeans yeehawbeans commented Oct 1, 2022 •
edited by VerisimilitudeX
Loading

Changed calculation to compare max and min value instead of all values, reducing complexity.

Closes #73

yeehawbeans and others added 4 commits October 1, 2022 10:53
@yeehawbeans
Refactor Properties.isRandomDNA(String)
46492ea 
Changed calculation to compare max and min value instead of all values, reducing complexity.
@yeehawbeans
Refactor for Utility classes
e3904d5 
Marked methods in utility classes as static and changed their invocations.
@yeehawbeans
Refactor Properties.isRandomDNA(String)
3cf0858 
Changed calculation to compare max and min value instead of all values, reducing complexity.
@VerisimilitudeX
Merge branch 'main' into refactor1
12f4e16
VerisimilitudeX
VerisimilitudeX requested changes Oct 1, 2022
View reviewed changes
Copy link
Owner

@VerisimilitudeX VerisimilitudeX left a comment

Choose a reason for hiding this comment

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

The code looks fine but when it is tested on dnalong.fa, it doesn't say that it is random, even though it is. Could you please look into this? Thanks!

@VerisimilitudeX VerisimilitudeX added this to the Strengthen Codebase milestone Oct 2, 2022
@VerisimilitudeX
Copy link
Owner

@B4CKF1SH

@Nv7-GitHub
Copy link
Contributor

I think this is because nucleotideCount contains a raw number of nucleotides and not a percentage, so you cant compare the values to percentages.

@Speedro
Copy link
Contributor

Speedro commented Oct 7, 2022

@B4CKF1SH Intellij Idea marks that the method is no longer used but remains in the code, I believe it can be deleted and this PR closed.

@VerisimilitudeX
Copy link
Owner

Good catch @Speedro! I didn't realize that this method call got deleted. I'll be adding it back promptly.

@Speedro
Copy link
Contributor

Speedro commented Oct 8, 2022

Good catch @Speedro! I didn't realize that this method call got deleted. I'll be adding it back promptly.

we need to add the unit tests :)

@VerisimilitudeX
Copy link
Owner

Yes, exactly. I'll be working on that this weekend. That is unless you want to help!

@Speedro
Copy link
Contributor

Speedro commented Oct 8, 2022

Yes, exactly. I'll be working on that this weekend. That is unless you want to help!

sure I can help, but I can start tomorrow if thats ok :)

@yeehawbeans
Fix calculation bug.
c0c0c5c 
Signed-off-by: B4CKF1SH <[email protected]>
@yeehawbeans
Copy link
Contributor Author

The issue was a missing pair of brackets, messing up the calculations in the new method. I added a fix, it now correctly identifies dnalong.fa. Please let me know if any further changes are required.

VerisimilitudeX
VerisimilitudeX requested changes Oct 8, 2022
View reviewed changes
Copy link
Owner

@VerisimilitudeX VerisimilitudeX left a comment

Choose a reason for hiding this comment

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

Looks good @B4CKF1SH! Could you please elaborate further on how you simplified the code in the comments?

@yeehawbeans
Add Comments
4318735 
Signed-off-by: B4CKF1SH <[email protected]>
VerisimilitudeX
VerisimilitudeX requested changes Oct 10, 2022
View reviewed changes
Copy link
Owner

@VerisimilitudeX VerisimilitudeX left a comment

Choose a reason for hiding this comment

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

Looks perfect now! I'll merge it once you clear the merge conflicts.

@VerisimilitudeX
Copy link
Owner

Looks perfect now! I'll merge it once you clear the merge conflicts.

@B4CKF1SH

@CLAassistant
Copy link

CLAassistant commented Oct 11, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@VerisimilitudeX
Copy link
Owner

VerisimilitudeX commented Oct 15, 2022 •
edited
Loading

Hey @B4CKF1SH I will be out of office for some time, so it will take me some time merge the pull request. However, I will approve the pull request once you clear the merge conflicts, in case you are participating in Hacktoberfest. Thanks for working on this, we at DNAnalyzer really appreciate your help!

@VerisimilitudeX
Copy link
Owner

@B4CKF1SH please clear merge conflicts! I'll approve your PR once you do so.

@yeehawbeans
Copy link
Contributor Author

I fixed the conflicts on my fork, it should be good now.

VerisimilitudeX
VerisimilitudeX approved these changes Oct 20, 2022
View reviewed changes
Copy link
Owner

@VerisimilitudeX VerisimilitudeX left a comment

Choose a reason for hiding this comment

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

Looks great @B4CKF1SH. Sorry for the delay in approving this PR!

@VerisimilitudeX
Copy link
Owner

Looking forward to seeing more PRs from you on DNAnalyzer!

@VerisimilitudeX VerisimilitudeX merged commit 06a0900 into VerisimilitudeX:main Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VerisimilitudeX VerisimilitudeX VerisimilitudeX approved these changes

Assignees

@yeehawbeans yeehawbeans

Labels
enhancement New feature or request
Projects
None yet
Milestone
Strengthen Codebase
Development

Successfully merging this pull request may close these issues.

Refactor Properties.isRandomDNA(String)
5 participants
@yeehawbeans @VerisimilitudeX @Nv7-GitHub @Speedro @CLAassistant