isocpp / isocpp/CppCoreGuidelines

F.3 Example code has too many issues

Open
#1,279 1 comment 0 reactions 1 assignee View on GitHub

@BenKeeping is already working on this.

Since Oct 11, 2018.

Dominant language
CSS
Stars
45.3k
Forks
5.6k
PR merge metrics
No merged PRs in 30d

Description

I know this code is supposed to need refactoring, but I don't think it's the best example to illustrate this point. We tried it in a job interview context and everyone got confused!

First problem - if flag1 = flag2 = 0, intermediate remains undefined and the function returns
finalize(undefined, 0). So at least make the correction
double intermediate = 0.;

2nd problem - when flag1 = -1, its value is changed to 1 - thus the if on case label 1 can never be true.
Therefore the comment in the refactored code about "handling flag1 = -flag1" is meaningless, and flag1 doesn't need to be passed to func1_tau. Since flag1 is received by value, there is no need to refer to it again at all.

3rd problem - return 0 at the end of the refactored code is wrong (unless this is what finalize returns!)

Here is what I believe is a correct refactoring of the original (with the sole change of initialising intermediate mentioned above). But I still don't think this is a good example, hence issue rather than pull request.

double complete(double intermediate, int flag)
{
    if (abs(flag) > 10)
        intermediate = func2(intermediate);
    if (flag / 10 == 2)
       return finalize(intermediate, 13.1);
    return finalize(intermediate, 0.);
}

double func1_muon(double val, int flag) {
    double intermediate = func1(val);
    if (flag % 2)
        intermediate = sqrt(intermediate);
    return complete(intermediate, flag);
    
}

double func1_tau(double val, int flag) {
    double intermediate = func1(-val);
    if (flag % 2)
        intermediate = sqrt(-intermediate);
    return complete(intermediate, flag);
}

double simple_func(double val, int flag1, int flag2)
// simple_func: takes a value and calculates the expected ASIC output,
// given the two mode flags.
{
    if (flag1 > 0)
        return func1_muon(val, flag2);
    if (flag1 == -1) 
        return func1_tau(-val, flag2);
    return finalize(0., 0.);
}

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.