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

Additional refactoring to maintain cohesion and solid principles. #1059

Closed
wants to merge 4 commits into from

Conversation

xcalibur91
Copy link

@xcalibur91 xcalibur91 commented Apr 7, 2023

Hello,
I have refactored some code in the project.

Refactor : Rename Variable
Description : The variables didn’t explain the exact use. The fetched value was a fraction and not percentage. The Aliphatic index is converted into percentage at the end. The formulation was also broken down for readability.

Refactor : Extract Class
Description: Separated file network operations and basic read write operations to different utility class to have single principle property.

Refactor : Move method
Description : The method convertDNAtoProteinSequence in ProteinMappingTools.java was very specific to DNASequence.java class and hence the code was refactored to move the method to maintain cohesion.

Refactor : Pull up method
Description : The GeneSequence class along with other classes qualify for Broken Hierarchy Design smell. Upon further analysis, it can be seen that the getLength() method can be pulled up the hierarchy. That is what this refactoring does.
A new class DNAExtractSequence is introduced to handle this situation which extends the original DNASequence.

Copy link
Contributor

@josemduarte josemduarte left a comment

Choose a reason for hiding this comment

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

I think the changes look good. But it'd be nice to clean the file copy function as commented below.

One general comment: please try to create one PR per concern. This one seems to have 3 independent concerns in it.

* @throws IOException
*/
@SuppressWarnings("resource")
public static void copy(File src, File dst) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this PR is all about cleaning up, how about taking this opportunity to remove this method and replace it by the one provided in the java library (see TODO comment in original javadocs above)

@josemduarte
Copy link
Contributor

I'll close for lack of activity.

@josemduarte josemduarte closed this Mar 4, 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.

None yet

2 participants