search

samba-in-kubernetes / samba-operator

An operator for a Samba as a service on PVCs in kubernetes
Apache License 2.0
103 stars 24 forks source link

resources: fixed confusing if-else-if patterns #264

Closed synarete closed 1 year ago

synarete commented 1 year ago

The code fragment:

    if cond1 {
    return foo
    } else if cond2 {
    do-bar
    }

may be replaced with:

    if cond1 {
    return foo
    }
    if cond2 {
    do-bar
    }

The second pattern is considered more readable (also in other programming languages[1]).

[1] https://clang.llvm.org/extra/clang-tidy/checks/readability/else-after-return.html

Signed-off-by: Shachar Sharon ssharon@redhat.com

obnoxxx commented 1 year ago

Quite a few commits have beenn added to this PR that do not relate to its original topic of improving if-else patterns at all but rather relate to grouping of shares. why is that? it seems to me that this PR was rebased on top of #240 at some point which has merged into master meanwhile. ... am I right, @synarete @phlogistonjohn ?

commits in a PR should imho implement what the PR description states. if ta PR depends on another unmerged PR, it should be mentioned explicitly.

Also, regarding commit messages and PR titles, I suggest to use present tense (or imperative) rather than past tense. e.g. "Fix confusing if-else-patterns " instead of "fixed confusing if-else-patterns.

phlogistonjohn commented 1 year ago

Yes, this PR is based on the patches from #240 which just got merged yesterday. I think @synarete has not gotten a chance to rebase it yet. :-)

synarete commented 1 year ago

Quite a few commits have beenn added to this PR that do not relate to its original topic of improving if-else patterns at all but rather relate to grouping of shares. why is that? it seems to me that this PR was rebased on top of #240 at some point which has merged into master meanwhile. ... am I right, @synarete @phlogistonjohn ? commits in a PR should imho implement what the PR description states. if ta PR depends on another unmerged PR, it should be mentioned explicitly. Also, regarding commit messages and PR titles, I suggest to use present tense (or imperative) rather than past tense. e.g. "Fix confusing if-else-patterns " instead of "fixed confusing if-else-patterns.

@obnoxxx You are correct: I need to rebase this one on top of latest-master and re-send this PR. Will do so soon. You are also correct on the commit message: past tense is just wrong and confusing. Will fix both -- thanks for pointing out.