styleguides icon indicating copy to clipboard operation
styleguides copied to clipboard

Do not chain up-front declarations

Open hapejot opened this issue 5 years ago • 8 comments

Hi, I agree with that rule in general. Until I found the auto-alignment feature of both abap editor in SE80 and also in eclipse. This auto-alignment will align the TYPEs so it looks much cleaner than individual lines. And there is no manual editing necessary.

Additionally, eclipse usually deals pretty well with the chained declarations. So there is typically no need to do manual editing as described in that section.

Wouldn't that be a reason to reconcider that section?

Cheers, Peter

hapejot avatar May 16 '19 15:05 hapejot

The main reason imho is the refactoring issue: it requires a lot more changes to cut&paste chained variables for refactoring than one-line-data-definitions. The most trouble you have when the you need to cut out the first or the last definition in a chained declaration.

tricktresor avatar May 29 '19 07:05 tricktresor

I agree. I am just thinking, that you lose more than you gain: You gain the benefit of copying a complete line when refactoring manually. But you lose any alignment, in the data sections, which usually helps me a lot to quickly see the data types of the variables.

hapejot avatar May 29 '19 14:05 hapejot

Not using colons and aligning does not exclude each other... ;)

tricktresor avatar May 29 '19 15:05 tricktresor

You have to do it manually then. Which includes much more work to keep it up to date. Thats why every styleguide for other languages recommends not to align commands.

But using chaining enables the AUTO-Align feature of the ABAP-Editor.

hapejot avatar May 29 '19 15:05 hapejot

I think this section should also be reconsidered due to the fact that the up-front chaining will also be applied by ADT when using the quick assist functionality for not declared variables.

rdibbern avatar Feb 17 '20 19:02 rdibbern

I don't understand why there should be a Clean Code rule for chaining DATA declarations or not.

The reason is, I don't read the declarations, I read the rest of the code. Clicking F2 gives me information on the variables I need to see, and I just need to see one declaration at a time. I don't bother how the DATA lines are aligned, chained or not.

My personal opinion is that this DATA block is often short because the procedure has to be short and inline variables are to be preferred, and so it doesn't harm my eyes a lot if the variable typing is not aligned. But generally speaking, my eyes really prefer to see them aligned (automatically of course, via the pretty printer). Moreover, the ADT Quick Assists converts inline variables into local variables or attributes by chaining them, so I don't want to spend time to "unchain" them!

So, I would love a lot that this section is removed. Same thing for the section "Don't align type clauses". Or the only thing worth saying is "do NOT spend time in aligning manually" and "do NOT spend time in removing the alignment manually". Even a mix of chained and "unchained" DATA doesn't bother me.

sandraros avatar Mar 21 '20 15:03 sandraros

I think this section should also be reconsidered due to the fact that the up-front chaining will also be applied by ADT when using the quick assist functionality for not declared variables.

Totally agree.

Also using keyword DATA for each line makes code much less readable. Introducing lot's of visual distraction while not providing any important/useful information.

Anyway if defined variables are not connected on logic level - a good sign that method is has more responsibilities than it should. In that way this rule makes no sense at all.

255h avatar Dec 02 '23 22:12 255h

The main reason imho is the refactoring issue: it requires a lot more changes to cut&paste chained variables for refactoring than one-line-data-definitions. The most trouble you have when the you need to cut out the first or the last definition in a chained declaration.

I want to point out that one principle of Clean Code is to optimize for readability, not changeability. We spend a lot more time reading code than writing code. So this must not be our main reason for keeping this section. In my opinion, the style guide only adds this as an additional reason. The main reasons are:

Chaining suggests the defined variables are related on a logical level. To consistently use it, you would have to ensure that all chained variables belong together, and introduce additional chain groups to add variables. While this is possible, it is usually not worth the effort.

A variable and its type belong together and should therefore be visually grouped in close proximity. Aligning the TYPE clauses draws attention away from that and suggests that the variables form one vertical group, and their types another one.

How do you feel about these two reasons?

ConjuringCoffee avatar Dec 04 '23 07:12 ConjuringCoffee