pret / pokeheartgold

Decompilation of Pokemon HeartGold/SoulSilver
343 stars 107 forks source link

Style: Indent level in switch blocks #227

Open PikalaxALT opened 1 year ago

PikalaxALT commented 1 year ago

Variant A: Case labels flush with the switch statement. This is the standard in pret/pokeruby etc.

switch (state) {
case 0:
    // ...
case 1:
    // ...
}

Variant B: Case labels indented by 1.

switch (state) {
    case 0:
        // ...
    case 1:
        // ...
}
adrienntindall commented 1 year ago

Variant A is better due to how many nested switch statements are in the code. The alternative would be to have the deepest indent in large essential functions be 3-4 indents deeper than they would have been sticking with variant A, which makes the code less readable. Then for consistency we should keep the rest of the code on the same style.

red031000 commented 1 year ago

I have made several PRs converting style A into style B, which have been approved and merged, and during original style guide discussions, we did decided on style B, so imo style B is better, plus, I prefer it because I personally find it much easier to read

AsparagusEduardo commented 1 year ago

I prefer style B.

mid-kid commented 1 year ago

Style A reinforces the fact that the case labels have no scoping significance and have about the same effect as a goto label. It's also wasteful to add an indent level when unnecessary.

luckytyphlosion commented 1 year ago

I abstain from this discussion.

luckytyphlosion commented 1 year ago

There is actually a third variant C: case labels indented by 1, but case contents not indented.

switch (state) {
    case 0:
    // ...
    case 1:
    // ...
}

I only mention this because this is the default in Notepad++. With that, I want to mention that editors are inconsistent as to which style they automatically indent to:

Editor Default behaviour Toggleable
Notepad++ Variant C Unknown (Discussion 1, Discussion 2)
Eclipse (yes this isn't C I know) Variant A Yes, see image below
Visual Studio Code Variant B Yes? (Issue which proves its existence in vscode-cpptools extension)

For eclispe: image

red031000 commented 1 year ago

There is actually a third variant C: case labels indented by 1, but case contents not indented.

switch (state) {
    case 0:
    // ...
    case 1:
    // ...
}

I only mention this because this is the default in Notepad++. With that, I want to mention that editors are inconsistent as to which style they automatically indent to:

Editor Default behaviour Toggleable Notepad++ Variant C Unknown (Discussion 1, Discussion 2) Eclipse (yes this isn't C I know) Variant A Yes, see image below Visual Studio Code Variant B Yes? (Issue which proves its existence in vscode-cpptools extension) For eclispe: image

objectively bad

AsparagusEduardo commented 1 year ago

There is actually a third variant C: case labels indented by 1, but case contents not indented.

It's like the worst of both worlds

tgsm commented 1 year ago

I prefer style B.

github-actions[bot] commented 2 months ago

This issue has had no activity for 60 days and will be marked stale. If there is no further activity, it will be closed in 30 days.

github-actions[bot] commented 4 days ago

This issue has had no activity for 60 days and will be marked stale. If there is no further activity, it will be closed in 30 days.