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

Facade Pattern #72

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

Conversation

KacieAhmed
Copy link

I implemented the facade pattern in-order to make the code more readable. The reason I chose the facade pattern is because the fold.py class was complicated, and I wanted to simplify it.

  • The three main operations of fold.py are parsing the data, unfolding, and simulating the folding. I split the main functions up into three classes (unfold.py, parse/py, and simulate.py). I then updated fold.py to utilize all of these three functions.

So overall, the function of fold is the same, it's just more readable/less complex

@Lajellu
Copy link

Lajellu commented Apr 18, 2021

Hi Kacie, your effort is appreciated and I hope this has been a timely response since your initial request. Here is some feedback to guide your eventually merge into the codebase:

Correctness of Structural Implementation

Points in Favour: The isolation of functionality was done by not only placing the logically separate code in fold.py into different files, but by placing each one in its own class. This especially makes sense for the Parser and Simulate, because each take arguments that would benefit from becoming private members of a class. This is less important for the class Unfold as it doesn't have any fields and only has 1 method, but it does create consistency in the way modules are imported.

Points Against: The indentation levels of the new code don't match the current codebase
(tabs: '/t' are used instead of 2 spaces: ' '). This code would benefit from linting. Although the style is recommended to match, rather than being a requirement, it would make the code look more consistent.

Choice of Structural Implementation

Points in Favour: Using the facade pattern was a good choice indeed because, at the function level, the original codebase was not easy to understand with one look. Previously, it needed to be studied more closely to understand where the boundaries of various functionality ended and started. Now it is clear.

Points Against: On line 14 of unfold.py, the new code still uses the variable name "fasta", which is not very representative of the data. Variables should not be named by their type (eg. a variable should not be named Int), and fasta is a file format for representing amino acids in text. Therefore this variable name should be changed to something like its predecessor, protein_fasta if the author wishes to keep the file format in the name for clarity. However, this was a condition of the original codebase and as this is not this PR's primary objective, it does not need to be included with it.

Correctness of Structural Logic

Points in Favour: The logic is correctly separated into the 3 files (simulate, parser and unfold), with their outputs each corresponding logically. (ie. a simulation, a parsed genome, and an unfolded genome respectively). It reduces the complexity of the code significantly by forcing the user to look in the related files/classes for separate functionality.

Points Against: Separating the code in fold.py does increase the time required to flip between different files to understand the inner-workings of what is ultimately used for the simulation. However, this disadvantage is not enough to discard this PR as its benefits outweigh them.

Clarity of Structural Logic

Points in For: The logic is clearly described in pull request description, although its title and commit messages would benefit from starting with action verbs that end in "-ed" as per the github recommendations.

Points Against: There are no comments added to the new files, and it is not clear from the setout (other than the class and method names) what the contract of each class is.

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