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

Prefer INSERT INTO TABLE to APPEND TO #352

Open
matthewdjb opened this issue Jun 11, 2024 · 11 comments
Open

Prefer INSERT INTO TABLE to APPEND TO #352

matthewdjb opened this issue Jun 11, 2024 · 11 comments
Labels
Adjustment Of Rule The issue or PR proposes an adjustment of a rule or set of rules clean-abap

Comments

@matthewdjb
Copy link

matthewdjb commented Jun 11, 2024

Relevant sections of the style guide
[> Please link here to all relevant sections of the Clean ABAP guide
](https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#prefer-insert-into-table-to-append-to)
Description
Remove

Use APPEND TO only if you use a STANDARD table in an array-like fashion, if you want to stress that the added entry shall be the last row.

Examples
When using a STANDARD table is changed to a SORTED or HASED table - which is quite common during code optimisation - you get a dump which is only detected at run time.

Since INSERT .. INTO TABLE with a STANDARD table puts the new record at the end, there's really no need for APPEND; except perhaps it's a bit more explicit.

To be really explicit you could use the rather wordier
DATA(pos) = lines( itab ) + 1.
INSERT was INTO itab INDEX pos.

In the past (oh shoot) 28 years of doing ABAP I've occasionally needed to insert a record at the last position - for example, with error log processing. But I've far more frequently encountered a dump when a SORTED or HASHED table has hit an APPEND.

Frankly, I wish ABAP syntax check would just warn about APPEND. :-)

@matthewdjb matthewdjb added Adjustment Of Rule The issue or PR proposes an adjustment of a rule or set of rules clean-abap labels Jun 11, 2024
@ConjuringCoffee
Copy link
Contributor

Honestly, I’m not quite sure I understand your suggestion. Could you please clarify how you’d like the rule to be adjusted?

@matthewdjb
Copy link
Author

Remove entirely from the section, the text

Use APPEND TO only if you use a STANDARD table in an array-like fashion, if you want to stress that the added entry shall be the last row.

@pokrakam
Copy link
Contributor

I agree INSERT should be preferred, but the way I interpret it I think makes sense. APPEND documents the fact that the sequence of entries in the table is important. Perhaps it could be reworded to:
Use APPEND TO only if you use a STANDARD table in an array-like fashion and you want to stress that the order in which entries are added is significant.

@nununo
Copy link

nununo commented Jun 25, 2024

I agree INSERT should be preferred, but the way I interpret it I think makes sense. APPEND documents the fact that the sequence of entries in the table is important. Perhaps it could be reworded to: Use APPEND TO only if you use a STANDARD table in an array-like fashion and you want to stress that the order in which entries are added is significant.

The problem with these rules of thumb is that they will probably not become that widely adopted. So in the end they become irrelevant. And suggesting APPEND in some cases is just leaving space for ambiguity.

I think INSERT should be used always. It's cleaner. People should know that INSERT always appends in the case of standard tables. If a specific behavior is needed then that the internal table type should be appropriately defined to reflect it.

@matthewdjb
Copy link
Author

matthewdjb commented Jun 25, 2024

@nununo Agreed.

@pokrakam I see what you mean. However it seems to me that if using an internal table in this way is intrinsically unsafe - an anti-pattern. If you need the records in a particular order, well that sounds like a use case for a sorted table! Clean Code is about building robust applications.

I've suggesting in Code Cleaner that a rule be added to change APPENDs to INSERTS, and it was this section in the guide that caused some of the objections.

@ConjuringCoffee
Copy link
Contributor

The aforementioned issue: SAP/abap-cleaner#8

@pokrakam
Copy link
Contributor

pokrakam commented Jun 25, 2024

I'll admit I'm playing devil's advocate a bit here and am broadly in favour of INSERT, but is APPEND really such an anti-pattern? Arrays exist in most languages and we seem to manage OK with them.

If you want to implement ordering via sorted table then you need an additional sequence field, which adds unnecessary complication for simple use cases as we already have the builtin index field.

The only 'unsafe-ness' is that someone may come along and sort the table, but a use case that qualifies for an APPEND should be obvious enough that this shouldn't be a risk.

METHOD push. 
  APPEND input TO stack.
ENDMETHOD. 

METHOD pop. 
  DATA(last) = lines(stack).
  READ TABLE stack INDEX last INTO result. 
  DELETE stack INDEX last.
ENDMETHOD.

Naming is also useful here, guesses_in_entry_order works for me. Maybe the wording could be changed to a slightly stronger 'Avoid APPEND unless ...'

Maybe, to go along with the new FINAL operator, SAP should also introduce a WORM table type? 😉 (I actually think this isn't a totally crazy idea...)

@nununo
Copy link

nununo commented Jun 25, 2024

I'll admit I'm playing devil's advocate a bit here and am broadly in favour of INSERT, but is APPEND really such an anti-pattern? Arrays exist in most languages and we seem to manage OK with them.

Well, if someone changes the table definition from STANDARD to SORTED, can't the APPEND fail?

@matthewdjb
Copy link
Author

@nununo It's the failure when you change the table definition that causes me to raise the issue in the first place. When optimising unperformant code, the first step is usually changing the table definition.

@pokrakam

Nice try, but...

METHOD push. 
  INSERT input INTO  TABLE stack.
ENDMETHOD. 

METHOD pop. 
  DATA(last) = lines(stack).
  READ TABLE stack INDEX last INTO result. 
  DELETE stack INDEX last.
ENDMETHOD.

Works fine. No need for APPEND at all. And since you've encapsulated it very nicely, there's no ambiguity. :-)

@pokrakam
Copy link
Contributor

@matthewdjb Ah, but if you change the table type to SORTED, then the INSERT can start to behave differently. APPEND is more intuitive and more explicit. In the push/pop example it will probably dump. To me a dump is preferable to silent logic/data corruption, which can even make APPEND safer in some scenarios.

My point is that the (few) use cases for APPEND should be so simple and clear that there should be no reason to change it.

If I put my devil's advocate hat back on, the argument of someone changing table types is not that different from the risk of someone changing a sort key. Both are done in the name of performance and both can inadvertently break things.

@matthewdjb
Copy link
Author

As with all Clean precepts - it's a guide, not a must. If no one used APPEND, the world would be a better place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Adjustment Of Rule The issue or PR proposes an adjustment of a rule or set of rules clean-abap
Projects
None yet
Development

No branches or pull requests

4 participants