RepoSense icon indicating copy to clipboard operation
RepoSense copied to clipboard

Implement Proper Deep Cloning for `RepoConfiguration` and `CliArguments`

Open georgetayqy opened this issue 1 year ago • 2 comments

What feature(s) would you like to see in RepoSense

Currently, both Builder patterns implemented in RepoConfiguration and CliArguments are implemented in such a way that the Builder objects are one-use and cannot be reused to build new instances of RepoConfiguration and CliArguments.

This is not ideal since this prevents us from reusing Builder instances to construct duplicate instances of RepoConfiguration or CliArguments, which violates the Builder pattern and incurs an overhead if we would like to construct duplicate copies of RepoConfiguration or CliArguments.

Is the feature request related to a problem?

This issue is related to issues #2076, #2117 and PR #2078, #2118.

If possible, describe the solution

We could change the codebase in such a way that classes used in RepoConfiguration and CliArguments, as well as RepoConfiguration and CliArguments, implement the Cloneable interface and provide an implementation of the clone() method to allow for easier duplication and cloning of RepoConfiguration and CliArguments instances.

If applicable, describe alternatives you've considered

N/A

Additional context

N/A

georgetayqy avatar Feb 17 '24 10:02 georgetayqy

It might be worth discussing the possible use-cases for Builder re-use, since this would be a new feature that wasn't previously possible. A clone would have to manually build an object of same semantics so every object down the line would have to do this to allow for deep copies, which would also add significant boilerplate throughout the backend. We might discuss if this tradeoff is worth the code-quality gains.

gok99 avatar Feb 19 '24 04:02 gok99

@gok99 I have provided a proof of concept for the clone feature in the draft PR linked to this issue, for your reference.

I agree that allowing deep cloning would introduce a significant amount of boilerplate code (as seen in the draft PR, with the different classes implementing the Cloneable::clone() method to allow cloning of RepoConfiguration and CliArguments. It might also incur additional performance/memory overhead through the creation of multiple referenced objects on the heap.

I'm not too sure where Builders can be reused efficiently at the moment; the only place I am certain would help would be the test cases (test cases can reuse base Builder objects to create more complex objects, reducing the amount of code needed to execute the test cases), but implementing such a drastic feature just for a slight improvement in memory use for unit testing might not be worth the increase in bloat and complexity of the codebase coupled with the increase in code quality...

georgetayqy avatar Feb 19 '24 04:02 georgetayqy