[#2076] Refactor RepoConfiguration to simplify constructor complexity #2078

asdfghjkxd · 2023-12-29T08:07:05Z

Part of #2076

Proposed commit message

Refactor RepoConfiguration to simplify constructor complexity.

Currently, the constructors for the `RepoConfiguration` class is 
very complex, involving up to 18 parameters at once, and is 
not easily extensible.

With the proposed changes, the constructor now follows the 
Builder pattern, by using a nested class `Builder` that 
helps to compose config parameters in a way that is much more 
modular and comprehensible, enhancing code quality and 
reducing confusion among developers.

However, it must be noted that `Builder` objects are not reusable as of
now, and that more combinations of arguments are permitted due to the
removal of constructors for `RepoBuilder`, which previously helped to
ensure the correctness of parameter combinations.

Let's refactor `RepoConfiguration` to simplify the 
complexity of the constructor and increase modularity 
of the code.

Other information

Apologies if this PR seems rather rough, as this is my first time contributing to an OSS, especially one at this scale!

I labelled this as a partial fix as documentation for the changes has not been made yet, and I'm not sure if that is within the scope of this issue; if required, I am happy to work on the updated documentation, before opening another pull request that seeks to address that, otherwise, I am also happy to upgrade this PR from a partial fix to a full fix.

gok99

@asdfghjkxd kudos for taking on a relatively meaty first issue - changes are looking good and I see no significant issues so far!

for a start, let's keep the documentation changes within this PR for completion, although i suspect there isn't too much to update.

asdfghjkxd · 2024-01-02T02:18:33Z

@gok99 I have gone back and reviewed the user/developer guides and didn't find any part that needs modification.

I believe the only documentation left would be the JavaDocs within the code itself, but all public methods and some private methods should have their JavaDocs already furnished!

MarcusTXK

@asdfghjkxd Great work on this! I was really impressed that you tackled this as your first issue. I think you did a very good job refactoring it into the new builder pattern and making sure to capture all the logic carefully.

I've added some comments as minor nitpicks. Additionally, it would be good if you could address some of these new lines that Codecov flagged as not covered by tests.

MarcusTXK · 2024-01-29T07:46:27Z

src/main/java/reposense/model/RepoConfiguration.java

For lines 38 - 39, new ArrayList<>() Initializer for ignoreGlobList and ignoredAuthorsList is now redundant

MarcusTXK · 2024-01-29T08:10:33Z

src/main/java/reposense/model/RepoConfiguration.java

+ /**
+ * Updates the {@code fileTypeManager} for {@code RepoConfiguration}.
+ *
+ * @param fileTypeManager List of file types and groupings permitted.
+ * @return This builder object.
+ */
+ public RepoBuilder fileTypeManager(FileTypeManager fileTypeManager) {
+ this.fileTypeManager = fileTypeManager;
+ return this;
+ }


Consider accepting a List<FileType> directly in the builder and internally creating the FileTypeManager, for simplicity and maintainability. This way, we encapsulate FileTypeManager creation and make the API easier for users, keeping the builder's usage clean and hiding unnecessary complexity.

public RepoBuilder formats(List<FileType> formats) { this.fileTypeManager = new FileTypeManager(formats); return this; }

asdfghjkxd · 2024-01-29T10:12:28Z

@MarcusTXK Sure thing, I will work on it and add a new commit here once I'm done fixing the code and creating new test cases!

asdfghjkxd · 2024-01-30T05:28:41Z

I have also made some changes to the way the processName and processOrganisation works, whereby if specified by RepoBuilder, the folder name, output folder name and display name will change to the one that is provided by RepoBuilder, rather than defaulting to the default names given to those 3 variables.

I believe that this was an oversight on my part and I had failed to implement checks and allow RepoBuilder configurations to override the default configuration.

This behaviour should be fixed now in the new commit!

gok99

sorry about the review delays! have left some suggestions and discussion points - feel free to comment :)

src/main/java/reposense/model/RepoConfiguration.java

gok99 · 2024-01-31T01:45:49Z

src/main/java/reposense/model/RepoConfiguration.java

+ */
+ private void processNames(RepoBuilder repoBuilder) {
+ String repoName = this.location.getRepoName();
+ this.displayName = repoBuilder.displayName == null


would an Optional be a good idea for these? we probably should be using more Optionals in RepoSense to enforce a check, but that's a separate discussion...

I agree, Optional would eliminate the == null checks, making the code a lot more understandable and readable! Will look into usingOptional here!

Should we look into creating an issue for these separately?

@MarcusTXK I agree, most of the issues we had discussed above are rather complicated (parameter verification, duplication of RepoConfiguration, etc.) and seem to warrant the creation of separate issues.

Yeah I think we can focus on improving the RepoConfiguration incrementally, as @gok99 has brought up alot of good points.

asdfghjkxd · 2024-02-04T14:11:14Z

I'm reverting the changes back to before the refactor, it seems like something has caused all the system test cases to break.

EDIT: After looking through the code, it seems that there was some mistake in how processOrganization handled the different names of the RepoConfiguration object. This issue has been resolved and the code will be updated back to where it originally was.

src/main/java/reposense/model/RepoConfiguration.java

src/test/java/reposense/parser/ArgsParserTest.java

…ub.com/asdfghjkxd/RepoSense into enhancement-repoconfig-builder-pattern

gok99

LGTM, thanks for the work!

MarcusTXK

LGTM, great job with this PR!

github-actions · 2024-02-16T18:58:56Z

The following links are for previewing this pull request:

Dashboard Preview: https://dashboard-2078-pr-reposense-reposense.surge.sh
Docs Preview: https://docs-2078-pr-reposense-reposense.surge.sh

Refactor RepoConfiguration to obey Builder Pattern

7c321de

ckcherry23 requested review from a team and MarcusTXK December 31, 2023 16:28

gok99 reviewed Jan 1, 2024

View reviewed changes

ckcherry23 requested a review from a team January 8, 2024 06:37

ckcherry23 added the s.ToReview label Jan 8, 2024

Merge branch 'master' into enhancement-repoconfig-builder-pattern

6228ec8

asdfghjkxd requested a review from a team January 23, 2024 16:46

Merge branch 'master' into enhancement-repoconfig-builder-pattern

be1ef45

MarcusTXK requested changes Jan 29, 2024

View reviewed changes

MarcusTXK requested a review from a team January 29, 2024 08:19

Fix bugs and implement new unit test cases

ae80634

asdfghjkxd requested a review from MarcusTXK January 30, 2024 05:31

gok99 requested changes Jan 31, 2024

View reviewed changes

Merge branch 'master' into enhancement-repoconfig-builder-pattern

a50e488

asdfghjkxd force-pushed the enhancement-repoconfig-builder-pattern branch from 07c7506 to a50e488 Compare February 4, 2024 14:27

Refactor RepoBuilder into Builder

052050b

asdfghjkxd requested a review from gok99 February 5, 2024 02:55

asdfghjkxd and others added 3 commits February 5, 2024 13:24

Combine processOrganization with processNames

d58ccee

Remove redundant copy constructor

4b5cf86

Merge branch 'master' into enhancement-repoconfig-builder-pattern

749f005

gok99 reviewed Feb 9, 2024

View reviewed changes

src/main/java/reposense/model/RepoConfiguration.java Show resolved Hide resolved

gok99 reviewed Feb 9, 2024

View reviewed changes

src/test/java/reposense/parser/ArgsParserTest.java Outdated Show resolved Hide resolved

asdfghjkxd added 2 commits February 9, 2024 22:24

Add new unit tests and remove print statements

237f8d7

Merge branch 'enhancement-repoconfig-builder-pattern' of https://gith…

2e8cc4c

…ub.com/asdfghjkxd/RepoSense into enhancement-repoconfig-builder-pattern

gok99 approved these changes Feb 9, 2024

View reviewed changes

MarcusTXK approved these changes Feb 9, 2024

View reviewed changes

MarcusTXK added s.ToMerge and removed s.ToReview labels Feb 9, 2024

gok99 and others added 2 commits February 11, 2024 21:15

Merge branch 'master' into enhancement-repoconfig-builder-pattern

53d8a3e

Merge branch 'master' into enhancement-repoconfig-builder-pattern

0e9e213

ckcherry23 merged commit d2018d2 into reposense:master Feb 16, 2024
10 checks passed

This was referenced Feb 17, 2024

Refactor CliArguments to conform to RepoConfiguration's Builder Pattern #2117

Closed

Implement Proper Deep Cloning for RepoConfiguration and CliArguments #2119

Open

Parameter Verification for RepoConfiguration and CliArguments #2121

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#2076] Refactor RepoConfiguration to simplify constructor complexity #2078

[#2076] Refactor RepoConfiguration to simplify constructor complexity #2078

asdfghjkxd commented Dec 29, 2023 •

edited

Loading

gok99 left a comment

asdfghjkxd commented Jan 2, 2024

MarcusTXK left a comment

MarcusTXK Jan 29, 2024

MarcusTXK Jan 29, 2024

asdfghjkxd commented Jan 29, 2024

asdfghjkxd commented Jan 30, 2024

gok99 left a comment

gok99 Jan 31, 2024

asdfghjkxd Jan 31, 2024

MarcusTXK Feb 5, 2024

asdfghjkxd Feb 5, 2024

MarcusTXK Feb 6, 2024

asdfghjkxd commented Feb 4, 2024 •

edited

Loading

gok99 left a comment

MarcusTXK left a comment

github-actions bot commented Feb 16, 2024

[#2076] Refactor RepoConfiguration to simplify constructor complexity #2078

[#2076] Refactor RepoConfiguration to simplify constructor complexity #2078

Conversation

asdfghjkxd commented Dec 29, 2023 • edited Loading

Proposed commit message

Other information

gok99 left a comment

Choose a reason for hiding this comment

asdfghjkxd commented Jan 2, 2024

MarcusTXK left a comment

Choose a reason for hiding this comment

MarcusTXK Jan 29, 2024

Choose a reason for hiding this comment

MarcusTXK Jan 29, 2024

Choose a reason for hiding this comment

asdfghjkxd commented Jan 29, 2024

asdfghjkxd commented Jan 30, 2024

gok99 left a comment

Choose a reason for hiding this comment

gok99 Jan 31, 2024

Choose a reason for hiding this comment

asdfghjkxd Jan 31, 2024

Choose a reason for hiding this comment

MarcusTXK Feb 5, 2024

Choose a reason for hiding this comment

asdfghjkxd Feb 5, 2024

Choose a reason for hiding this comment

MarcusTXK Feb 6, 2024

Choose a reason for hiding this comment

asdfghjkxd commented Feb 4, 2024 • edited Loading

gok99 left a comment

Choose a reason for hiding this comment

MarcusTXK left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 16, 2024

asdfghjkxd commented Dec 29, 2023 •

edited

Loading

asdfghjkxd commented Feb 4, 2024 •

edited

Loading