Skip to content

Confusing code #17

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

Open
pwaller opened this issue Jul 20, 2019 · 1 comment
Open

Confusing code #17

pwaller opened this issue Jul 20, 2019 · 1 comment
Assignees

Comments

@pwaller
Copy link
Contributor

pwaller commented Jul 20, 2019

This is only a minor suggestion, but I found this confusing:

perf/record_test.go

Lines 219 to 225 in 239c48f

const errDisabledTestEnv = "PERF_TEST_ERR_DISABLED"
func init() {
// In child process of testErrDisabledProcessExist.
if os.Getenv(errDisabledTestEnv) != "1" {
return
}

  1. Variables/consts beginning with the name err are usually errors.

  2. It refers to testErrDisabledProcessExist which doesn't exist.

  3. It's unclear from the comment which side of the branch is "in" something. Is it in the returning side, or the non-returning side? This might be a rare case where "early return" is a minor hinderance and perhaps it would be better to phrase it positively, and move the body of code out, to make it clear when the special condition matches:

    func init() {
      if specialCondition {
        doSpecialCondition()
      }
    }
@acln0
Copy link
Owner

acln0 commented Jul 20, 2019

This is indeed quite confusing, and bad. It's probably a relic from before a refactor. All of these self-exec tests need to be checked out more intimately anyway (#12), so I'll clean this up when I get to that. Thanks.

@acln0 acln0 self-assigned this Jul 20, 2019
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

2 participants