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 enumerations over booleans #14

Closed
JLindenberg opened this issue May 3, 2019 · 10 comments
Closed

prefer enumerations over booleans #14

JLindenberg opened this issue May 3, 2019 · 10 comments

Comments

@JLindenberg
Copy link

maybe ABAP was more modern than other languages in not providing booleans, and with the addition of abap_bool became as worse as others?

There is a debate about using booleans outside, e.g.

http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html
https://silkandspinach.net/2004/07/15/avoid-boolean-parameters/
http://www.beyondcode.org/articles/booleanVariables.html (2003)
Reliable Software Technologies - Ada-Europe 2001: 6th Ada-Europe (https://books.google.de/books?id=E0aPZtAMJBQC&pg=PA50&lpg=PA50&dq=boolean+considered+harmful&source=bl&ots=5eXX4ZIMbS&sig=ACfU3U0sAXNMB3Aa-0Y2lo2OF1ua_mekcA&hl=de&sa=X&ved=2ahUKEwjFvay9uP_hAhUB3KQKHahwBrs4FBDoATADegQICRAB#v=onepage&q=boolean%20considered%20harmful&f=false)

In fact it is really hard to date it back as google prefers newer webpages over older…

Can we please add a section “Don´t use booleans where you don´t know it is binary forever?” to https://github.com/SAP/styleguides/blob/master/clean-abap/CleanABAP.md#booleans ?
Not using booleans but enumerations usually improves readability of code significantly unless the domain is in fact binary.
Cheers, Joachim

@HrFlorianHoffmann
Copy link
Contributor

The section Split method instead of Boolean parameter partially covers this. A little unsure whether linking there will do, or whether we should add a new small sub-section on the choice of data type.

@JLindenberg
Copy link
Author

I´d definitely prefer banning booleans in the first place - as a most likely inadequate data type. If then you end up with an enumeration Do.This or Do.That, then you know how to proceed..

@HrFlorianHoffmann
Copy link
Contributor

Added a section Use Booleans wisely that I think closes this issue.

@JLindenberg
Copy link
Author

good start, but...

  • the link to "Split methods..." is broken (mind the s)
  • references 2 & 3 would better fit to "Split methods.." rather than "Use Booleans wisely"
  • I am missing the point that readability of booleans is often worse than enumerations even if the domain is binary

@HrFlorianHoffmann
Copy link
Contributor

There are several reasons why I didn't comment on the readability so far.

For once, this affects other languages far more than ABAP, because ABAP usually requires you to repeat the parameter names:

 // Java
updateDocument(true, true, false);

while

" ABAP
update_document( synchronous = abap_true
                 with_commit = abap_true
                 write_logs  = abap_false ).

ABAP becomes ambiguous only when you're down to one parameter and you can ommit the parameter name:

update( abap_true ).

Although this issue frequently occurs with Booleans, it's not caused by them. The issue is in fact caused by the unavailability of the parameter names in the method call, and can happen with all data types. In most cases, enumerations are not even an option:

// Java
someString.substring(0, 3);  // what's 3? length? end index?
sendEmail("", "", "", "");  // title, body, recipient, sender - or the other way round?
assertEquals(a, b);  // which one is the expected value? a or b?

In return, it's not necessary to use a different data type to provide a meaningful enumeration:

" ABAP
CONSTANTS:
  BEGIN OF save_mode,
    with_commit TYPE abap_bool VALUE abap_true,
    without_commit TYPE abap_bool VALUE abap_false,
  END OF save_mode.

update( save_mode-with_commit ).

Over the years, most IDEs added the feature to show parameter names inline without them actually being present in the code. This greatly reduces this issue's impact. As far as I know, the ABAP Development Tools do not (yet) support this feature.

In summary, yes, I agree that it's a good idea to challenge whether a Boolean is the right choice, but this ambiguous-call argument is a rather weak one.

@pokrakam
Copy link
Contributor

pokrakam commented May 7, 2019

Over the years, most IDEs added the feature to show parameter names inline without them actually being present in the code. This greatly reduces this issue's impact. As far as I know, the ABAP Development Tools do not (yet) support this feature.

If I understood correctly, you can do this with <alt>-F2 from anywhere inside a method.

@HrFlorianHoffmann
Copy link
Contributor

HrFlorianHoffmann commented May 7, 2019

That is Eclipse's "Element Info", a quick preview of the method signature in a separate window.

What I mean are "Parameter Hints", as found for example in IntelliJ:

ScreenshotOfInlinedParameterNamesInIntelliJ

@pokrakam
Copy link
Contributor

pokrakam commented May 7, 2019

Ah, nice!
But I guess since parameter names are required if more than one, then it is less of a priority for ABAP.

@JLindenberg
Copy link
Author

agreed - ABAP is better by naming arguments.

sendEmail("", "", "", "");  // title, body, recipient, sender - or the other way round?

actually signatures like these could use more specific types rather then generic types. E.g. recipient and sender could be a class that also provides validation in addition to having a string representation. Is your body html, rtf, or text? Lazy coders..

// Java
someString.substring(0, 3);  // what's 3? length? end index?
assertEquals(a, b);  // which one is the expected value? a or b?

no idea.

@HrFlorianHoffmann
Copy link
Contributor

After the fixes in #25, is there still something we should change?

xtough added a commit to xtough/styleguides that referenced this issue Mar 29, 2021
* should fix SAP#26 , SAP#24 , SAP#14
 * needs some more polishing of example for SAP#21
xtough added a commit that referenced this issue Apr 1, 2021
* Creative commons license

it's not code, anyway

* Create README.md

initial title

* Scaffolding (#5)

* scaffolding

* remove non existing logo

* update title

* README: additional info for onboarding (#6)

* scaffolding #2 (#7)

* scaffolding #2

* some first intro content

+ added VSCode build task

* first intro content

* Inital diagram

Finally got ditaa to work. Plantuml activity diagrams are apparently not supported. Not really convinced yet of the text-based diagrams
approach.

Co-authored-by: Christoph Pohl <christoph.pohl@sap.com>

* change font to sans-serif (#9)

* introduction, minor changes (#10)

* some more notes (#12)

* some more notes

* some words regarding impact

* mention abapgit restrictions/design

* update scenarios

* add note regarding steampunk

* first attempt at best practices

Co-authored-by: Christoph Pohl <christoph.pohl@sap.com>

* small formatting issues (#16)

* minor changes (#17)

* updates (#23)

* update deps

* updates

* upd

* closes #15

* upd

* small additions and corrections

+ some official text on (g)CTS

Co-authored-by: Christoph Pohl <christoph.pohl@sap.com>

* move the static analysis part into the tooling chapter (#25)

* move the static analysis part into the tooling chapter

* add short abaplint description

* show 3 toc levels

* updates

* upd

* small corrections and enhancements on tooling

Co-authored-by: Christoph Pohl <christoph.pohl@sap.com>

* Update README.md (#31)

* Update README.md

* Update README.md

* fix typos

Co-authored-by: Christoph Pohl <christoph.pohl@sap.com>

* add title page logo (#33)

* add title page logo

* add page numbers

* Build timestamp

* recommend spellchecker
* custom dictionary

* add this document section (#32)

* add this document section

* introduction, sections etc.

* remove heading, move related work to new chapter

* add related work to index

* moved to abstract

Co-authored-by: Christoph Pohl <christoph.pohl@sap.com>

* front page hacks (#34)

* update links (#36)

* update activity diagram (#35)

* update activity diagram

* upper case OK

* polish scenarios (#37)

* should fix #26 , #24 , #14
 * needs some more polishing of example for #21

* publish pdf to latest tag (#38)

* Update build.yml

* Create publish.yml

* Update build.yml

* Update build.yml

* Update publish.yml

* dummy, testing (#39)

* rename release asset (#40)

* mention latest release pdf (#43)

* minor changes (#41)

* link Gerrit

* upd

* add todos

* upd

* minor

* upd

* update abaplint section

* some diagrams

* more diagrams

* I love diagrams

* upd

* remove "to"

* happy -> happen

* Update code_review.adoc (#45)

``todo, wording? remove "qualified" ? 

Done.

`` "gain reputation" -> "developers can actively become more knowledgeable about the codebase" something like that

No need to overdo it.  Meritocracy is all about reputation. I don't see how anyone could take offence in that.

* update url (#46)

* related work: add link (#47)

* workaround page numbering problem (#52)

* adjust headings (#51)

* update README.md with automatic build information (#48)

* update README.md with automatic build information

* Update README.md

Co-authored-by: Christoph Pohl <christoph.pohl@sap.com>

* abapgit examples (#53)

* wip: abapgit examples

* abapgit chapter updates

* split setup steps in two

* scenario updates (#49)

* scenario updates

* upd

* upd

* clarify

* update deps

* minor changes (#54)

* name in italics

* additional abaplint links

* abapGit add note

* add link

* Move gCTS example to separate section (#56)

* fixes from Mike (#58)

* README: add "Thanks To" seciton (#57)

* check links in build (#59)

* check links in build

* test

* move structure

* add link to code review guide in main README.md

* prepare latest release links

release links should point to SAP/styleguides

* use main license

* parent license

* ignore build and node_modules

* Delete LICENSE

redundant to ../LICENSE

* Update .gitignore

moved build and node_modules to top level

* README, add AsciiDoc notes

* code review improvements

* motivation: getting different perspectives
* selection: mention central distribution by team leads

* add missing space

* Automated Review --> Checks

* disambiguation of terms

VCS, git platforms...

Co-authored-by: Lars Hvam <larshp@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants