refactor: move validator api endpoints to standalone functions #6238
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6238 +/- ##
=========================================
Coverage 80.95% 80.95%
=========================================
Files 185 185
Lines 17935 17935
Branches 1078 1078
=========================================
Hits 14519 14519
Misses 3389 3389
Partials 27 27 |
Performance Report✔️ no performance regression detected Full benchmark results
|
There was a problem hiding this comment.
I don't see how this makes the code more maintainable. Splitting up code just because the file has above X amount of lines of code does not make sense to me. After this PR a lot more boilerplate code is required to add a new api.
We also lose the git history / blame on the validator api code which is always something we should avoid unless it is really required for a refactoring with strong reasoning behind it (which I don't see from the Motivation in PR description)
|
i think we can have file splits functionalty wise i.e. attestations related, synccommittee related, block related (produce and publish, and hence please no function renaming, also build on top of this PR: #6236 as this and its base PR: #6227 are high priority and need to be in the next milestone for deneb release) boiler plate |
|
If I spin up there is tons of literature on the topic, but the basic rule of thumb have a global agreement. More readable code is more maintainable code. And readability of a 1000 lines of single function nested upto 4-5 levels is less than of a 100 lines of function. Consider the fact that here we are not taking about Few references: A very common used practices is Airbnb Style Guide, which suggest 50 lines per function and 300 lines per file. An other industry best practices guide for Ruby even go strict and suggest to have 10 lines per method. If I remember correctly Bob Martin author of Clean Code suggested to have it upto 100 lines per function. So we can debate on number of lines but bloating a function to unlimited number lines is not something even debatable. Git history is well preserved and can be traced back even if code is moved to different files, you just have to follow a different tree path on some level. If we really make git history a blocker for refactoring, then we can't really improve any product. The other reference point to split by behavior is not gonna work because each team member may have different understanding of it. So splitting per endpoint is straight forward for everyone to understand. And having idiomatic smaller higher order functions have a lot of text book benefits that I don't want to list here. |
That's not a single function, you could use a class instead and would achieve the same result. If indentations are a concern could use a class + methods which would reduce the depth by one but honestly better to review each function see if improvements can be done there.
I don't think either of those are helpful for this argument.
I am not debating this, my points are
Just look at the git diff of this PR (+1,193 −937), it's not reviewable. I would have to trust that you properly copy-pasted the code and made no mistakes.
Again, not part of my argument, never said it is a blocker but there should be strong reasoning to move code around like that which this PR does not have. This is my subjective opinion which can probably only be changed if there is a good and objective argument for this refactoring which I think is something we should have for every proposed code change. |
|
As a contributor who recently picked up the validator api code, let me put in my two cents. I think while it seems large and overwhelming (>1000 lines of code), its organization is good but a little messy. Something I observed:
For 1, I think they can be managed separately and they are trivial enough that losing git history might not be a big deal compare to endpoint functions. (I could be wrong). For 2, it is not clear to me why some are defined in the former way and some are defined in the latter way. My thought was functions with complex logic like My humble suggestion is moving forward, any new endpoint with complex logic should be placed in a standalone file under For the purpose of this PR, I think |
|
Moving everything that is defined before the return statement out of the file would be a good first step. Everything block production related could definitely deserve it's own file, not a big fan of endpoints folder for that but maybe just a What I don't like in this PR are cases were we add 12 lines of boilerplate code for 1 line of actual code. Previously, the api impl type was also inferred when updating the route definition, now it requires a manual step to pick the correct type and clue it together in index file. And the last thing is that the validator impl now deviates from how we define other routes, do we move the others into separate file as well and why is it not part of this PR then? I feel like moving everything into separate files should be a last resort if all other attempts of refactoring failed or are insufficient. |
|
Strongly in favour of keeping git history and not moving existing code around |
|
I don’t work with the codebase as much as you guys but would like to see a decision about it today if possible. There should be a decision on how to organize the existing functionality in which @dapplion is strongly against and also a decision on how to organize this going forward. Some of the newer guys opinions should give a fresh perspective here on what to do. |
|
My 2 cents: generally in favor of improving readability. What could improve it in this specific scenario is having shorter functions, supported by extracted utility functions. It would also help with improving code comprehension (and removing the need for some comments) and testability. |
|
I've pulled the code locally to give a fair review. Tried to navigate and weigh the pros and cons here and in unstable. I don't really read this file as "a single 1000 line function", rather the file is an implementation of a single interface, I think some of the utility functions could be pulled out, and either made pure or pulled into methods. As far as how we should structure code going forward, whether to combine functionality in a single file or move things across files is, imo, somewhat subjective, and shouldn't necessarily be reduced to "smaller file is more readable, so smallest file is most readable".
Block production may warrant its own file, given the importance in the application. It seems that this was also the original catalyst for this PR. |
|
A bit tangential, but relevant IMO: I want to comment on a common trend in JS community which is the phobia to long files. Long may indicate opportunities to split code, but they are not intrinsically bad. Splitting a long file because it's long doesn't always help the overall project. I past example I personally fought to keep as long file is the gossipsub/index.ts file (https://github.com/ChainSafe/js-libp2p-gossipsub/blob/master/src/index.ts). Splitting it always resulted in harder to follow code. We have been doing just fine with it. Even git is smart enough to almost never trigger conflicts even if you edit the same file. |
|
Closing this PR now. Feel free to open a PR to refactor utility functions and / or a PR to refactor the block production code. |
Motivation
Description
Steps to test or reproduce