Refactor Properties.isRandomDNA(String) - Issue #73#97
Refactor Properties.isRandomDNA(String) - Issue #73#97VerisimilitudeX merged 16 commits intoVerisimilitudeX:mainfrom
Conversation
Changed calculation to compare max and min value instead of all values, reducing complexity.
Marked methods in utility classes as static and changed their invocations.
Changed calculation to compare max and min value instead of all values, reducing complexity.
VerisimilitudeX
left a comment
There was a problem hiding this comment.
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!
|
@B4CKF1SH |
|
I think this is because |
|
@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. |
|
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 :) |
|
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 :) |
Signed-off-by: B4CKF1SH <43198489+B4CKF1SH@users.noreply.github.com>
|
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
left a comment
There was a problem hiding this comment.
Looks good @B4CKF1SH! Could you please elaborate further on how you simplified the code in the comments?
Signed-off-by: B4CKF1SH <43198489+B4CKF1SH@users.noreply.github.com>
VerisimilitudeX
left a comment
There was a problem hiding this comment.
Looks perfect now! I'll merge it once you clear the merge conflicts.
@B4CKF1SH |
|
|
|
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! |
|
@B4CKF1SH please clear merge conflicts! I'll approve your PR once you do so. |
|
I fixed the conflicts on my fork, it should be good now. |
VerisimilitudeX
left a comment
There was a problem hiding this comment.
Looks great @B4CKF1SH. Sorry for the delay in approving this PR!
|
Looking forward to seeing more PRs from you on DNAnalyzer! |
Changed calculation to compare max and min value instead of all values, reducing complexity.
Closes #73