msg_service_router: reduce code complexity #7570
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7570 +/- ##
==========================================
- Coverage 54.77% 54.77% -0.01%
==========================================
Files 601 601
Lines 38145 38142 -3
==========================================
- Hits 20894 20891 -3
Misses 15126 15126
Partials 2125 2125 |
| @@ -92,3 +88,10 @@ func (msr *MsgServiceRouter) RegisterService(sd *grpc.ServiceDesc, handler inter | |||
| func (msr *MsgServiceRouter) SetInterfaceRegistry(interfaceRegistry codectypes.InterfaceRegistry) { | |||
| msr.interfaceRegistry = interfaceRegistry | |||
| } | |||
|
|
|||
| // gRPC NOOP interceptor | |||
| func noopInterceptor(_ context.Context, _ interface{}, _ *grpc.UnaryServerInfo, _ grpc.UnaryHandler) (interface{}, error) { | |||
There was a problem hiding this comment.
Are you planning to use this function elsewhere? What's the benefit of extracting a tiny snippet of code that just return nil, nil when it's not yet reused anywhere else?
This smells like early refactoring, and as such, pointless (best case scenario) if not potentially harmful.
There was a problem hiding this comment.
Please check the PR description.
Are you planning to use this function elsewhere?
The function is private
What's the benefit of extracting a tiny snippet of code
improving readability
There was a problem hiding this comment.
Does it really improve readability to extract an empty function? I too tend to prefer named functions, though it generally boils down to one question to me: Am I going to use this function elsewhere?
If I am, then it's a no-brainer and I would define it as a separate function right away. If not well, does the benefit overweight the cost of increasing file size and polluting the private namespace?
There was a problem hiding this comment.
Good questions. In fact, sometimes it's opposite - extracting a function it can even reduce the compiled code, because we don't need to carry the context. So depending on the compiler.
Again - function is private, and it's used. Going further with your argument we would prefer to have big functions, which, IMHO is not good.
So, I don't see any benefit. I would see some if the parent function would be smaller and less nested.
| @@ -92,3 +88,10 @@ func (msr *MsgServiceRouter) RegisterService(sd *grpc.ServiceDesc, handler inter | |||
| func (msr *MsgServiceRouter) SetInterfaceRegistry(interfaceRegistry codectypes.InterfaceRegistry) { | |||
| msr.interfaceRegistry = interfaceRegistry | |||
| } | |||
|
|
|||
| // gRPC NOOP interceptor | |||
| func noopInterceptor(_ context.Context, _ interface{}, _ *grpc.UnaryServerInfo, _ grpc.UnaryHandler) (interface{}, error) { | |||
There was a problem hiding this comment.
| func noopInterceptor(_ context.Context, _ interface{}, _ *grpc.UnaryServerInfo, _ grpc.UnaryHandler) (interface{}, error) { | |
| func noopInterceptor(_ interface{}, _ interface{}, _ interface{}, _ interface{}) (interface{}, error) { |
There was a problem hiding this comment.
I think this would work too
There was a problem hiding this comment.
true, though I prefer seeing the exact signature from gogogrpc
There was a problem hiding this comment.
exactly, no need to generalize here.
|
Adding Stargate backport label on this, as otherwise i'm getting merge conflicts when backporting #7671 into |
* msg_service_router: reduce code complexity * order imports Co-authored-by: Alessio Treglia <alessio@tendermint.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Description
As the old saying goes:
When reading the PR (after it was merged) I was wondering when some functions begin and when end. So I propose to reduce the nested functions and extract out what's possible.
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes