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

"Too many chained references" on fluent interfaces in general #20

Open
egreiner opened this issue Sep 9, 2016 · 9 comments
Open

"Too many chained references" on fluent interfaces in general #20

egreiner opened this issue Sep 9, 2016 · 9 comments

Comments

@egreiner
Copy link

egreiner commented Sep 9, 2016

            var result = nhibernateSession.QueryOver<Employment>()
                                .Left.JoinAlias(x => x.Assistant, () => assistant)
                                .Where(x => x.EmploymentType.Id != 4)
                                .List<Employment>();

-> .List "Too many chained references"
makes no sence for me

@citizenmatt
Copy link
Collaborator

Could you provide more detail, please? This doesn't give me anything I can work with.

@egreiner
Copy link
Author

egreiner commented Sep 9, 2016

I mean the same thing as stated in issue #15 but not only regarding to LINQ.
C# uses fluent programming extensively in LINQ to build queries...
https://en.wikipedia.org/wiki/Fluent_interface#C.23

I find nothing smelly on:
var result = nhibernateSession.QueryOver()
.Left.JoinAlias(x => x.Assistant, () => assistant)
.Where(x => x.EmploymentType.Id != 4)
.List();
so such code shouldn't be marked as "Too many chained references"

@toni1991
Copy link

Without an real codebase I can't provide you any refactoring hints, but your code is kind of smelly ;)

IMHO this code actually should be marked as "Too many chained references".

@egreiner
Copy link
Author

Dear Toni,

Please refactor this code that i can see what you mean with not smelly code.

var result = nhibernateSession.QueryOver()
.Left.JoinAlias(x => x.Assistant, () => assistant)
.Where(x => x.EmploymentType.Id != 4)
.List();

thx

@toni1991
Copy link

In your context your advise might be correct. Without knowing hibernate, it's still obvious to me what your code does.

But the real point is, that in general such method chains lead to code which is hard to understand and to maintain. Don't think of writing code, rather think of reading it later without background knowledge!
Because of the design of the underlying library your code isn't refactorable to me, without any knowledge of the surroundings.

I also suspect that I've got your question wrong. If so, I'm sorry.

@egreiner
Copy link
Author

"Without knowing hibernate, it's still obvious to me what your code does."
This is for me a perfect description of how code should be writen.
I don't know how it exactly works, but i understand what the code should do.

  • hide details, i'm not interessted in details, i want to see details - how it's done - as late as possible
    so that's why .Where(...) should be .Where(x => x.IsNot(EmploymentType.Freelancer)) ...
  • make details easily to UnitTest.

In this case i don't see it as violation of "Law of Demeter".

@openshac
Copy link

This type of "violation" of the the Law of Demeter is discussed in length by Phil Haack

He quotes Martin Fowler:

I’d prefer it to be called the Occasionally Useful Suggestion of Demeter.

http://blog.robustsoftware.co.uk/2010/04/linq-and-law-of-demeter.html
https://lostechies.com/derickbailey/2010/03/25/law-of-demeter-extension-methods-don-t-count/

All these articles seem to suggest that it's not just a "dot counting exercise".

This makes finding a one size fits all solution difficult to code into a CleanCode.Feature.

When is the correct time to ignore warning from the Clean Code extension?

Presumably when they are inappropriate.

@DannyMeister
Copy link

Fluent interfaces which don't violate the Law of Demeter are generally implemented as static extension methods. I think it would be reasonable to (optionally) exclude them.

@KieranJeffreySmart
Copy link

Not just extension methods.
I use builder classes quite a lot, using a similar fluent mechanism is really useful and IMHO very readable.
An example:
var myObject = builder
.SetPropertyA(A)
.SetPropertyB(B)
.SetPropertyC(C)
.SetPropertyD(D)
.Build()

This is a common pattern I use and I feel does not violate LoD

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

No branches or pull requests

6 participants