7

In a recent PR, a developer, whom I will call Alice, came across a lot of resistance by a coworker (Bob) because she wrote a utility code unit in a fluent style rather than in a classical style.

In short, Alice had a piece of business logic (BL) similar to this (Java):

Role role = ...;
List<Role> otherRoles = ...;
if ( /* role is higher rank than all other roles */ ) {
  // do stuff
}

where the enumeration Role must have a ranking system according to some business logic and she needed a utility method to compare a single role against each one of a set of other roles.

So a classical approach would be like this:

if (isRoleHigherThanAll(role, otherRoles)) {
  // do stuff
}

And you would have a classic Utility method like this:

public static boolean isRoleHigherThanAll(Role role, Collection<Role> otherRoles) {
  // utility method logic here...
}

However, since those lines of code were part of some complex business logic method, Alice wanted to make it "fluent" to improve the readability and make it clear what the condition is checking at a first glance for those who are not familiar with such code.

if (role(role).isHigherRankThanAll(otherRoles)) {
  // do stuff
}

And the Utility methods would look like this instead:

static ComparableRole role(Role role) {
  return new ComparableRole(role);
}

// intermediate object serving the fluent-style API
@Data
static class ComparableRole {
  private final Role role;

  public boolean isHigherRankThanAll(Collection<Role> otherRoles) {
    // utility method logic here
  }
}

As you can see, the utility logic LOC are sort of doubled, but in return you get a fluent, allegedly more readable condition in the main business logic method.

Now, I have just massively simplified what Alice actually did... The condition was a bit more complex and Alice went much further and added another couple of utility methods in the fluent interface style to create her own domain-specific language (DSL) for this single isolated problem.

The main point is that she received objections in the PR that such style makes a verbose Utility API which is pointless given that such Utility API only exists to break a complex BL class and externalize a particular aspect of the logic into a different unit of code, so as a matter of fact it is only used in one single place. Bob says that the alternative doesn't read that bad (it actually reads more natural) and since nobody would write code like that, such coding style breaks the POLA. Also, with the classic way, the utility code LOC are more than halved.

Alice thinks it is just a stylistic debate and at the end of the day her solution is good enough, but Bob went as far as rejecting the PR as Alice's approach was deemed actually "wrong" and not up to standards.

I thought I'd ask for some external opinions. I am using fictitious names to prevent biases and focus on the matter.

Is there "too far" with a fluent interface and method chaining? And how far is too far?

6
  • 1
    "given that such Utility API only exists to break a complex BL class and externalize a particular aspect of the logic into a different unit of code" - can't say for sure without further context, but on the face of it, this is not necessarily a valid objection. Breaking a class is how you manage complexity, distribute responsibilities, and achieve decoupling - you are supposed to do that, the question is has it been done in the right way. BL classes especially shouldn't be large, complicated beasts - build complicated BL out of smaller units. 1/2 Sep 11 at 13:08
  • I don't know if Bob is right or wrong in this particular case; from what you described, it seems that your team is used to a very procedural style, where you create large classes with a bunch of roughly related methods. The team also seems to harbor the "Adding a class needs justification!" attitude (e.g., the objection that "it is only used in one single place"). This is again related to being more comfortable with procedural code, and not understanding levels of abstraction. On the other hand, "with the classic way the utility code LOC are more than halved" suggests Alice overengineered. 2/2 Sep 11 at 13:08
  • isRoleHigherThanAll is oddly specific. I guess there is a utility set of many such specific use case functions which keeps growing ?
    – S.D.
    Sep 12 at 7:38
  • 1
    In general, you can scrap the LOC arguments from your reasoning. Line count is not a valid measure of code quality, and is not a valid measure of what is and isn't an appropriate implementation. When you skip the LOC arguments, it seems that all that's left is Bob's opinion on what he considers more readable. Is Bob in a position to break ties such as these, or is Alice on equal footing with him? Because this just sounds like 2 devs with differing subjective opinions.
    – Flater
    Sep 12 at 12:09
  • Alice and Bob are peer devs. Sep 12 at 12:21

4 Answers 4

14

When it comes to readability, the opinion of the one who wrote the code should never be trusted over someone who didn't. It's called the curse of knowledge.= If you wrote it, you already know how it works. So you have no idea how easy it is to read. So if Bob is that opposed, Allice's only hope is what Charlee thinks.

Allice's real mistake was not shopping the new idea around early and getting buy-in before writing a bunch of code that dies in a big formal peer review. New ideas need time. They need to be shared one on one and hashed out. A process that would shape how Allice applies the new idea and would familiarize her team with the new style.

As for the fluent style itself, I've heard you say some concerning things. I have some experience with the fluent style= myself. One thing I learned is setting up an internal DSL is a fair bit of work. So when you say it's only going to be used in one place that left me very concerned.

The ideal situation for a big DSL is when it will be used often. When its attention to a specialized situation can be leveraged, as it's used over and over. That's when the pain of designing and learning this beastie is really worth it.

Yes, it is a more natural style. But until the team is used to it it's an unfamiliar style. Getting them familiar with it comes at a cost. I know it's fun to play with new toys, but you won't win any friends by throwing your toys at them. Share them gently and you might find they have a few toys worth playing with themselves.

Also, understand that these things come in very different sizes. You'll see a fluent style can be monstrously complex= or more moderate like Java 8 streams= or as simple as a Joshua Bloch Builder.= The simpler they are the less wide spread their use needs to be to make them worth it.

P.S.: I will say though, better names help a fluent interface a bunch:

if ( role(admin).outranks(annoyingUsers) ) { ... }

P.P.S.: Another answer is challenging my central thesis: writers can't be trusted to know what's readable. They need to listen to feedback from readers. So I'll add some links that discuss a closely related issue:

Code is read far more often then it is written.===

P.P.P.S: Seems I need to clarify how the voting should work. It's not that Bob (or any single reader) should have veto power. It's that Alice (our writer) should have no vote. She's biased. So her only hope is that my creatively spelled Charlee (fellow reader) votes for the code producing a tie which we can resolve with a coin flip. Arbitrary, but it gets us out of the conference room in time for lunch.

8
  • 4
    I can't see how a fluent style is a new idea in 2022. This approach just leads to secret rules to trip people up. "Old Bob McBobface hates X and can never be persuaded, so we just don't use it to avoid arguments, but its not an actual rule". He will reject PRs from newbies, but not other seniors or people he likes
    – Ewan
    Sep 11 at 9:09
  • 2
    @Ewan I can’t see how French is a new idea in 2022. It’s about communicating with the team. Not the idea. Sep 11 at 11:58
  • 2
    if you have a team of experienced cunning linguists I don't expect to have to sell them on this great new thing called "French"
    – Ewan
    Sep 11 at 15:42
  • 1
    lol I would but i don't understand it! getting rid of your little 3 line linky things? I embrace your new style of link, no need to sell it to me before posting an answer :)
    – Ewan
    Sep 11 at 15:53
  • 1
    @Ewan hehe yes the 3 line linky thing. OK I'll reject. I feel better knowing it's not just me that likes them. Sep 11 at 16:08
4

I agree with Bob, fluent does nothing to improve code and adds an extra class/method in most cases.

However.

This is why you have rules for PRs and don't just leave it down to preferences. If there isn't a rule which says "you must not use the fluent style" then the PR shouldn't be rejected.

Given that the code works either way, it is just a coding style issue and developers should be able to cope with multiple different coding styles without any problems. If Bob can't, he needs to learn.

If you don't write down your rules first, then you will just make everyone's lives harder as their PRs get rejected for made-up crap.

6
  • 2
    There is an argument to be made that PRs can be rejected when they needlessly deviate from the codebase's established coding style; and it's not realistic to expect a team to pre-emptively and explicitly list every possible way in which a developer could deviate from the codebase's coding style. If Alice is dramatically changing the style with no plan to carry this through to the rest of the codebase, then a different style becomes a justifiable reason to block a PR, or at least to bring it up as a team discussion.
    – Flater
    Sep 12 at 12:20
  • then you would write that rule down along the style of the code base and Anne would have known in advance not to write in a fluent style.
    – Ewan
    Sep 12 at 12:52
  • It is not manageable to explicitly list every way in which someone can deviate from the existing style/syntax. It's not even just a matter of explicitly defined style, it can just as well be a matter of inconsistency in syntax across the codebase without it having been explicitly codified. You can't require every possible aspect of every codebase to be explicitly described up to and including the explicit exclusion of anything that's different. That would literally require you to list every possible thing you could do and label it as allowed or prohibited.
    – Flater
    Sep 12 at 20:34
  • 1
    really? I think its fairly common to have style rules automatically enforced. and "fluent" is hardly some weird off the rails thing that can't have come up before. You are just splitting hairs.
    – Ewan
    Sep 12 at 22:50
  • Honestly, in 15y of my career, I have never seen anyone "explicitly" introducing which code styles are acceptable and which are not. This can change anytime as the dev lead changes. From a very restrictive policy of coding styles to absolute anarchy. Instead of typing down a sort of "Playbook" about "how to write code in my project" would be simple to arrange a meeting with your coworkers, introduce what have you seen, your feelings about those styles and why should be the cause of rejection on PR.
    – Laiv
    Sep 13 at 14:18
2

It was this remark by @candied_orange that prompted me to add this answer:

the opinion of the one who wrote the code should never be trusted over someone who didn't

I would adopt the opposite approach by default, that the decisions of the writer(s) should be accorded respect.

It is the writer who has to do the bulk of the work and get to grips with the problem in the greatest detail, and allowing any one reader to override them is far too capricious.

The criteria for override should be that "the results are so bad that no reasonable programmer would have produced them in the circumstances".

In other words, you are orienting the question not about whether the code is as good as it possibly can be (which is bound to create bitter arguments on minor points), but whether it is bad beyond the standard of competence.

I'm not myself a fan of the "fluent" style (perhaps because I've always been a user of VB and it has a With block that solves the same problem), but I haven't heard that the general level of regard for the fluent style is so low that no reasonable programmer would ever use it.

When conformity is required on aesthetic matters, the best approach would be to either involve more people in decisions earlier, or to give design responsibility to a single person, rather than to delegate but quibble with the results afterwards.

8
  • 2
    Just to clarify, I'm not claiming Bob (one reader) should have veto power. I'm saying Alice (the writer) gets no vote. That's why her only hope is what Charlee (another reader) thinks. If that doesn't solve it then flip a coin and get back to work. Sep 11 at 14:27
  • 1
    @candied_orange, I agree that if you've got several objective people saying the code is unfathomable, then there must be some problem. But the OP has one guy saying only that the code could have been simpler, not two guys saying they can't grasp it. I fully agree with your thesis that code should be readable - the questions are "what standard" and "who decides", and I don't think any one reader should be capable of vetoing code just because they have an idea that it could be simpler. That would be an extreme burden for writers.
    – Steve
    Sep 11 at 17:45
  • "decisions of the writer(s) should be accorded respect." - The writer may not be even be here in the company next year, so if a reviewer has trouble with their style, reviewer and writer should find a compomise. I don't want writers in my team who stubbornly insist on "my code - my decision".
    – Doc Brown
    Sep 12 at 5:49
  • ... with the only valid criteria for declining a PR is "the results are so bad that no reasonable programmer would have produced them in the circumstances", a code base will quickly start to get a little bit less readable with each merge, which over time will turn the whole thing into an unreadable mess. The road to hell is paved with good intentions.
    – Doc Brown
    Sep 12 at 8:05
  • Obviously its hard to judge the readableness of your own code. However all work is a compromise between competing demands. Letting people veto for style reasons doesn't result in readableness, They can just pick up on single instances they don't like without having to work out a solution that works holistically.
    – Ewan
    Sep 12 at 13:11
2

From what I see, the pure functionality should have been implemented as a method in the Role class, named outranks taking another role.

So everybody in the future can check whether one role outranks another without jumping through silly hoops like making a list out of the one element they have first to then call a function that only works on lists.

From there, it really just becomes a matter of how well your language can solve simple problems.

Role thisRole = ?;

if(otherRoles.All(otherRole => thisRole.outranks(otherRole)))

or even shorter:

if(otherRoles.All(thisRole.outranks))

this is C#, but I'm sure the Java equivalent looks about the same.

Once you have your basic methods straightened out, there is little need for this "utility" function.

You said your example is more complicated, but my advice would be the same: boil it down to the basics and implement those basics that are missing. Then use your programming language to solve problems, I'm sure it can. That's way better than writing "utility" and mixing your basic domain concepts with your logic at the site you want to use them.

1
  • That's a good advice, though probably I should have mentioned that this option was evaluated and discarded because this ranking of roles only pertains the use case handled by this BL and there will be other use cases that will require different rankings. Although it is possible to add an extra argument to your outranks() method holding the array against which one wants to compare the role, but then you'd have yet another debate about readability in the BL class. Sep 12 at 7:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.