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

[Tracking] - Code Health Overhaul #9771

Closed
1 of 6 tasks
rauljordan opened this issue Oct 13, 2021 · 3 comments
Closed
1 of 6 tasks

[Tracking] - Code Health Overhaul #9771

rauljordan opened this issue Oct 13, 2021 · 3 comments
Assignees
Labels
Cleanup Code health! Tracking Gotta Catch 'Em All

Comments

@rauljordan
Copy link
Contributor

rauljordan commented Oct 13, 2021

💎 Issue

Background

Over the past few months, our team has been a lot more focused on code health. Namely, we want to adopt successful software engineering practices into our organization to help Prysm succeed as a project, and make our life easier as developers. Better code health means more testable, concise, and simpler code that is easier for contributors to add value to and also for fewer bugs to arise. We have recently published Software Design Principles in Go, and now have a direction for how to start implementing these ideas in Prysm. This is a tracking issue for all code health related tasks grouped by domain.

Description

Separation of Concerns

  • Avoid leaking all instances of CLI flags, cli.Context beyond the main packages in Prysm
  • Use function options for configuring services in Prysm

Encapsulation

  • Encapsulate all feature flag conditionals as close as possible to what they toggle
  • Encapsulate nil checks across the repository and return package-level errors such as ErrNotFound Encapsulate Nil Checks #9454

Single Responsibility Principle

  • Use functional validation pipelines for reusable validity checks on data structures in Prysm

Open/Closed Principle

  • Currently, we use type assertions on the various kinds of Keymanager implementations across Prysm, which defeats the purpose of using interfaces. This is a violation of the open/closed principle. Adding a new keymanager implementation to Prysm shouldn't mean we have to modify the rest of the codebase to add new type assertions for it
@rauljordan
Copy link
Contributor Author

Related: #9883

@rkapka
Copy link
Contributor

rkapka commented Dec 25, 2021

Related: #7492

@terencechain
Copy link
Member

Let's close this and open this in a separate issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup Code health! Tracking Gotta Catch 'Em All
Projects
None yet
Development

No branches or pull requests

3 participants