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

Warn on presence of too many indirections #129

Closed
chavacava opened this issue May 1, 2019 · 4 comments
Closed

Warn on presence of too many indirections #129

chavacava opened this issue May 1, 2019 · 4 comments
Assignees
Labels
rule proposal Issue proposing a new rule

Comments

@chavacava
Copy link
Collaborator

Expressions that have too many indirections are hard to understand.
For example (from Kubernetes):

o.s.GA.ZoneOperations.Get(o.projectID, o.key.Zone, o.key.Name).Context(ctx).Do()
f.Signature.Receiver.Elem.Name
new(RouteBuilder).typeNameHandler(w.typeNameHandleFunc).servicePath(w.rootPath).Method("DELETE").Path(subPath)

Other than hard to read, this kind of code is fragile (calls and slice accesses are chained without checking for nil returns nor empty slices)

I propose to create a rule that warns when an expression has too many indirections (too many must be configurable)

@chavacava chavacava added the rule proposal Issue proposing a new rule label May 1, 2019
@mgechev
Copy link
Owner

mgechev commented May 1, 2019

This sounds a bit like the Law of Demeter. Makes sense to me.

@chavacava
Copy link
Collaborator Author

Yes, I've did not mentioned Law of Demeter because a rule checking it is fare more complicated than just calculating call chains length. For example a builder like MyObject().WithOption1().WithOption2() chains calls but it respects the Law of Demeter. To detect cases like that we need to typecheck and then control on the types in the call chain.
Most of the time, a simple call chain length is enough to point out aberrations like the ones I've used as example in the proposal.

@mgechev
Copy link
Owner

mgechev commented May 3, 2019

Yes, this makes sense. LGTM!

@mgechev
Copy link
Owner

mgechev commented Oct 17, 2019

@chavacava do you think we can close this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule proposal Issue proposing a new rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants