hhariri / CleanCode

ReSharper Plugins
MIT License
122 stars 37 forks source link

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

Open egreiner opened 8 years ago

egreiner commented 8 years ago
            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 commented 8 years ago

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

egreiner commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

"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.

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

openshac commented 7 years ago

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 commented 7 years ago

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 commented 6 years ago

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