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

chore: Refactor the method creating sync tasks #618

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

Conversation

thecooldrop
Copy link

Hello everyone, during my work on ArgoCD I noticed that quite a few methods in GitOps engine are quite large, and are not broken down into smaller, understandable methods. This has made it very hard for me to understand what the methods and functions do, as I had to keep very large information present in my mind, in order to understand them.

In this PR I have extracted blocks of code into hopefully more understandable, smaller methods and functions. For me the code is more readable now, because:

  • I can understand each part independently from other part
  • Names of methods provide some context as to what is being done
  • Names of methods are more aligned with actual functionality provided, make it easier to create a connection between ArgoCD functionality and the code
  • Less context pollution, because there is less variables present now

If this change is welcome I would like to do similar changes other methods as well.

Note that there are few other changes which could be done, but I felt that I would need to write additional tests to cover this behavior, which is currently out of scope for me, as I am both only vaguely familiar with Golang, and even less so with this codebase.

…d methods to increase code readability

Signed-off-by: Vanio Begic <vanio.begic123@gmail.com>
Signed-off-by: Vanio Begic <vanio.begic123@gmail.com>
Copy link

sonarcloud bot commented Aug 11, 2024

@thecooldrop
Copy link
Author

Hello @jessesuen @alexmt , is there anything I can add to this PR that would ease the review process?

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.

1 participant