Skip to content

refactor: replace for:in over lambdas - #117

Merged
plozanol merged 4 commits into
mainfrom
refactor/change-user-option-for-each
Dec 18, 2022
Merged

refactor: replace for:in over lambdas#117
plozanol merged 4 commits into
mainfrom
refactor/change-user-option-for-each

Conversation

@mikededo

@mikededo mikededo commented Dec 18, 2022

Copy link
Copy Markdown
Contributor

We currently had many nested for each in order to loop over the different entities and construct our organization. It had reached a point in which it was really nested, lots of indentation and it was not sustaniable/scalable.

Closes #104

We currently had many nested for each in order to loop over the
different entities and construct our organization. It had reached a
point in which it was really nested, lots of indentation and it was not
sustaniable/scalable.
We currently had our `UserOptionRequest` inside the `shell` package and
we were coupling the `github` pacakge to such request.
@mikededo mikededo added type: feat New feature or request status: wip PR work in progress type: refactor Refactors code or similar labels Dec 18, 2022
@mikededo mikededo self-assigned this Dec 18, 2022
@mikededo mikededo removed the type: feat New feature or request label Dec 18, 2022
@mikededo mikededo added this to the 2 - MVP milestone Dec 18, 2022
@mikededo mikededo changed the title refactor: replace over lambdas refactor: replace for:in over lambdas Dec 18, 2022
@mikededo mikededo added status: code review PR ready to be reviewed and removed status: wip PR work in progress labels Dec 18, 2022
@mikededo
mikededo requested review from a team December 18, 2022 10:57
@mikededo
mikededo marked this pull request as ready for review December 18, 2022 11:01

@plozanol plozanol left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The GitHubUserOptionController new approach is clean and easy to read.

I see that we have 6 repositories collaborators here, and despite it seems to respect the single responsibility principle, I feel too many calls for a simple controller, and also that way will be more difficult unit test having to mock all of them.

Just thinking about applying Chain of Responsibility pattern here, and creating domain.services to encapsulate these internal use cases. This way every LoopThroughOrgs (or a better naming) should only call one repository collaborator and the next nested LoopThroughWhatEver use case. This flexibility will allow us to reuse these pieces with the DbController or other future approaches.

What do you think?

@mikededo

Copy link
Copy Markdown
Contributor Author

Completey agree 💪🏼, plus the teacher proposed using CoR patter for such case.

I will apply this refactor in another task (so we can merge this).

@plozanol
plozanol merged commit 4befb19 into main Dec 18, 2022
@plozanol
plozanol deleted the refactor/change-user-option-for-each branch December 18, 2022 17:02
@mikededo mikededo added status: done PR/Issue completed and removed status: code review PR ready to be reviewed labels Dec 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: done PR/Issue completed type: refactor Refactors code or similar

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(proposal): lambas over regular loops

3 participants