Reactor-core: Source code verbosity

[Kindrat]

Expected behavior

Source code is easy to read without remembering tons of single character variables that are assigned from two character variables

Actual behavior

long r = requested;
                long e = 0L;
                long replenishMain = 0L;

                if (r != 0L && sq != null) {

                    while (e != r) {
                        d = done;

                        R v = sq.poll();

                        boolean empty = v == null;

                        if (checkTerminated(d, false, a)) {
                            return;
                        }

                        if (empty) {
                            break;
                        }

                        a.onNext(v);

                        e++;
                    }

Reactor Core version

3.1.1.Release

We need to debug reactor code from time to time and reading sources is not a trivial task since you need to hold context of all one-letter variables and their inheritance. It's not clear what is the reason of such convention but it really does not influence performance, etc. Is it possible to start using more verbose approach and use descriptive variable names?

[mlozbin]

Agree, and that may also help project to get more pull requests.

[Kindrat]

Any updates, guys. Maybe there is some work I could help with? Don't know if without knowing original context of vars naming I can do much...

[akarnokd]

Believe me, it's not the naming convention that holds off PRs, but the mere fact that concurrency and Reactive Streams are really hard to program for.

Using short local variable names has the benefit for an experienced contributor/reviewer to concentrate on the pattern the operator uses and allows focusing on the differences from it. For example, the quoted queue-drain pattern appears in large number of operators with some slight or moderate modifications. A trained "eye" can match the usages way better than having to read the full variable names all the time, especially if those use very similar letters in their wording.

The naming convention itself is very straightforward:

• pick the first letter of the instance variable,
• if this creates duplicate locals (requested vs result), pick two letters from the word or the camelcase words,
• or else use a short, one word variable name.

However, there are special cases where the function the variable represents is very unique to the particular modification of the pattern, so there is a need to distinguish that with a more longer name. Here, it is replenishMain which is modified throughout the method and as a potential source of lost requests, it should stick out when reviewing the code (especially on GitHub).

[Kindrat]

Sorry, what?) Do you mean Spring framework developers don't have trained "eye" or their framework is created for noobs?) You claim that obfuscated code is better than the one we can read without breaking the brain?

Just to compare: DefaultLifecycleProcessor from spring

    private void startBeans(boolean autoStartupOnly) {
        Map<String, Lifecycle> lifecycleBeans = getLifecycleBeans();
        Map<Integer, LifecycleGroup> phases = new HashMap<Integer, LifecycleGroup>();
        for (Map.Entry<String, ? extends Lifecycle> entry : lifecycleBeans.entrySet()) {
            Lifecycle bean = entry.getValue();
            if (!autoStartupOnly || (bean instanceof SmartLifecycle && ((SmartLifecycle) bean).isAutoStartup())) {
                int phase = getPhase(bean);
                LifecycleGroup group = phases.get(phase);
                if (group == null) {
                    group = new LifecycleGroup(phase, this.timeoutPerShutdownPhase, lifecycleBeans, autoStartupOnly);
                    phases.put(phase, group);
                }
                group.add(entry.getKey(), bean);
            }
        }
        if (!phases.isEmpty()) {
            List<Integer> keys = new ArrayList<Integer>(phases.keySet());
            Collections.sort(keys);
            for (Integer key : keys) {
                phases.get(key).start();
            }
        }
    }

As you can see - no single a= b-1 while (e != null)

Do you understand that there are bugs still in your codebase and debugging this code is really really hard.

Let me enumerate some critical cases I also faced with:
https://github.com/reactor/reactor-core/issues/992
https://github.com/reactor/reactor-core/issues/761
https://github.com/reactor/reactor-core/issues/763

I understand that renaming everything is not a trivial task, but was hoping at least to have some roadmap and help with it instead of hearing "you don't have trained "eye" to work with our sources". This approach sucks when you hope to get community feedback, you know...

[simonbasle]

@Kindrat I don't think that's what @akarnokd was trying to say, but rather that the concurrent programming patterns used in reactor rely on local copies of instance variables, and that short variable names for these copies make these patterns more obvious during review.
Any deviation from these patterns can then be made more obvious by the use of a long-named variable.

These patterns are the real difficult part here. Sure, the short variable naming induces a higher level of entry to understand the code base, and that's not helping guys like you that want to debug the code from time to time. But the level of entry is already pretty high anyway, due to the concurrency patterns in use. What @akarnokd says is that the short naming convention optimizes the task of maintainers, who have experience dealing with these patterns, on the long run.

That said we're still thinking about a better convention that makes things clearer. This is a big task, so we have to wait for the right time to start on it. Unless we take a more incremental approach where new operators are implemented with this readability issue in mind?

[akarnokd]

How many times does that code pattern appear in Spring? How is that code async and reactive?

Yes, there could be bugs lurking in Reactor, but there should be little reason to debug that deep into it. Besides, such action requires deep understanding and ability to abstract away the use case and focus on the code behavior. It is part of the job to undestand the meaning of the variables, after which the abbreviation should no longer matter. In fact, as I mentioned, a sort and distinct naming makes room in the mind for other aspects such as the concurrent dataflow or state evolution in the method.

The linked issues refer to components designed and added after my core operator contributions were taken from the RSC project. In fact, Reactor's style and structure changed so much that it would require quite a lot more effort to bring Reactor's code coverage up to par with RxJava's.

What I'd like to have from community feedback is unit tests that demonstrate a problem. My experience with supporting RxJava is that it is uncommon a reporter identified the correct core problem or bug in the library code, yet alone how to fix it. That needs experience over time.

[Kindrat]

@simonbasle I understand, maybe it's better to close the issue if there is no plan for making the sources more readable for anyone except maintainers... I just hoped that if the issue was not closed at the very beginning and assigned - it is planned for some release, maybe branch created, etc...

@akarnokd we are not talking about experience in any way. The bug https://github.com/reactor/reactor-core/issues/761 has nothing to do with complexity. The bug https://github.com/reactor/reactor-core/issues/992 is so damn critical, I can't even imagine more - make false positive tests for everyone who is going to use reactor. I guess, you'd have more problems after lots of people finding this in their prod if someone with 'not so trained eye' would not have time to debug that deep into it. In my understanding - if library in open-sourced and is intended to be a part of spring core and maybe - maybe - default implementation of java core Flow interface - it should focus on making more user-friendly interface (API, documentation,...) instead of locking it to some non-ordinary convention. Even if you'd claim you have no time and won't work on this issue - it would be more honestly with respect to everyone.

[simonbasle]

@Kindrat it still planned, nothing has started yet is all. it couldn't happen between maintenance releases (hence 3.2.0 milestone), and the sheer size of the change would necessitate to freeze development in order to take time to review (or be faced with continuous merging nightmares).

[smaldini]

I think this is a fair demand but not a trivial one so we put it in 3.2.0. We'll certainly try to make some efforts, we want to improve things and can point to that issue. Such solution include internal abstractions, annotations, eventually variable name alignement and method name alignement.
@akarnokd message was on point, I'm not sure why his answer was received poorly. He essentially explains why it is this way and things we could improve like replenishRequest (in fact short names are annoying when you use "Find Variable Use" tools).

[Kindrat]

@smaldini just because expected something similar to your or @simonbasle responses, but the main point of @akarnokd is still that short names are rather feature of reactor and you do not need to debug that deep when you actually have to, giving the impression nothing will be changed.