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

Refactor Potential Implementation and Design Smells #2733

Open
snehit221 opened this issue Nov 18, 2023 · 2 comments
Open

Refactor Potential Implementation and Design Smells #2733

snehit221 opened this issue Nov 18, 2023 · 2 comments

Comments

@snehit221
Copy link
Contributor

snehit221 commented Nov 18, 2023

Problem

This pull request will use strategic refactoring techniques in the uPortal open-source project to enhance code readability, maintainability, and extensibility.

Solution

These refactorings aim to elevate the quality of the uPortal codebase, making it more maintainable, understandable, and extensible for both current and future contributors.

I have tried my best to apply the below refactoring techniques: (each commit will depict that technique in my PR)

To cater Implementations smells

  1. Extract Method Applied
  2. Rename Method applied for better clarity
  3. Decomposed Conditional to reduce the cyclomatic complexity.

To cater Design Smells:

  1. Move Method Refactoring Applied to cater feature envy
  2. Pull Method / Variable Method applied to remove code duplicate (where encountered)
  3. Replaced Conditional with Polymorphism.

Alternatives

There can be multiple design choices made to make the code more maintainable. Due to trade-offs and complexity, I have not YET used the extract class refactoring yet. There are some monster classes which demonstrate insufficient modularization. Currently, I have used a few of the refactoring as answered above.

@ChristianMurphy
Copy link
Member

Hey @snehit221! 👋

Improvements to code quality and reducing code smells is welcome!
There are a number of tools in place which help with this spotbugs, Error Prone, CodeNarc, ESLint, and stylelint to name a few.
Which could point to areas to focus on.

With any refactor, consider focusing any given PR on a single type of refactor/smell at a time, and applying it module by module if possible.
This both makes changes easier to review, and reduces risk of work being lost/unused if it doesn't fit with the project.


Extract Method Applied

Is there a specific section of code you believe needs to be extracted?
I'm certainly open to it, but don't necessarily want to apply DRY for the sake of being DRY, instead looking for deeper organizational improvements https://medium.com/@kiwiandroiddev/beyond-dry-why-redundancy-makes-your-code-more-robust-and-less-fragile-6a64d0172a57

Rename Method applied for better clarity

Again are there particular sections where you've found names which are confusing or incorrect?
With internals there's plenty of flexibility with naming, for public APIs, we'd want to approach that with more caution to avoid breaking adopters.

Decomposed Conditional to reduce the cyclomatic complexity

If there are good abstractions they can certainly be applied.
There are also parts which have irreducible complexity, breaking those apart for the sake of hitting a cyclomatic complexity target will reduce readability.

Move Method Refactoring Applied to cater feature envy
Pull Method / Variable Method applied to remove code duplicate (where encountered)
Replaced Conditional with Polymorphism.

This is a general list of code smells.
It's a bit vague what specifically you are proposing here.

@snehit221
Copy link
Contributor Author

Hi @ChristianMurphy,

Thanks for your valuable inputs and guidance. I went ahead and used tools such as Designite and SonarQube, which gave me some clues about possible refactoring. I have created two pull requests, where each commit shows the type of refactoring I applied to improve code maintainability.

I have tried to keep each commit relatively simple from my end.
As for the move method, pull method and replaced conditional
I have added the commit message where ever I applied that. I think it would be easier to comprehend once the PR is looked at.

My primary goal was to apply these implementation and design level refactoring as a starter, so the the code base can be more flexible, maintainable and closely follows SOLID design.

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

No branches or pull requests

2 participants