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

Builder Pattern Applied #68

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

Builder Pattern Applied #68

wants to merge 4 commits into from

Conversation

KacieAhmed
Copy link

I applied the builder pattern in order to ensure no methods ever accidentally edit the genome. To do this, created a builder that builds the genome as an immutable object. Previously, cc built the genome, but it was mutable and the variable name wasn't informative. Now "GenomeBuilder" builds the genome.

  • Added genome.py (builder class)
  • Modified library to get rid of "cc" (previously cc built the genome)
  • Modified opt and corona to import the builder instead of cc.

@KacieAhmed KacieAhmed changed the title Pattern Builder Pattern Applied Apr 16, 2021
@Lajellu
Copy link

Lajellu commented Apr 18, 2021

Hi Kacie, thank you so much for your hard work and choice of this project to contribute to. Here is some feedback for your pull request:

Correctness of Creational Implementation

Points in Favour: This change does indeed belong in the codebase, and not in an external library because it is specific to the class and libraries like SimTk already have non-specific versions implemented for generic use.

Points Against: There are no comments in the Genome class to explain the reasoning for adding the builder (ie. making its genome immutable)

Choice of Creational Implementation

Points in Favour: The builder was partially an appropriate choice, because as mentioned in the paper, if a method were to change the data in the genome, it would represent a mutated version instead of the version we wish to target. The builder pattern maintain a consistent object state.

Points Against: Currently, there is no multithreading being used in this program, so protecting against changes in other threads is not yet necessarily. Therefore, it would be better to merge this pull request once threading is implemented.

Correctness of Creational Logic

Points in Favour: It is correct to have used the lib.py file as it is where the original codebase included “cc” to store the nucleotides list. The new code replaces this with a genome
object that is immutable, and named more appropriately.

Points Against: Perhaps another method could be added to the Genome class to extend functionality for the future, but this is not currently needed.

Clarity of Creational Logic

Points in Favour: This added code does not create an exorbitant level complexity, as it is easy to read, and coders are not likely to introduce bugs while trying to run or extend this code.

Points Against: A longer explanation could have been provided as to how the builder pattern makes the genome data thread-safe, so that other coders would be able to more clearly understand the logic behind the choice

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