I need a code review for best practices and code correctness of the following piece of code. run executes a program and validates in output. If the output is valid status can be found in VALID_SOLUTIONS.

elapsed = None
tmpdir = tempfile.mkdtemp(prefix='structure-%s-' % os.path.basename(instance))
  for r in range(options.repeat):
    run_returncode, run_status, run_elapsed = run(
        r, path, program, tmpdir, options.timeout, options.satisfiable)
    if run_status not in VALID_SOLUTIONS:
      returncode, status, elapsed = run_returncode, run_status, None

    if r == 0:
      # Set the expected results
      returncode, status = run_returncode, run_status
      elapsed = [run_elapsed]
      if returncode != run_returncode or status != run_status:
        # Results don't match
        returncode, status, elapsed = None, 'inconsistent_results', None
        # Extra set
except Exception as e:
  # Unexpected error in script itself
  print(e, file=sys.stderr)
  status = 'error'
except KeyboardInterrupt as e:
  print('interrupted', file=sys.stderr)
  returncode, status, elapsed = None, 'interrupted', None
  if status in VALID_SOLUTIONS:
    avg = sum(elapsed) / len(elapsed)
    std = (sum((e - avg)**2 for e in elapsed) / len(elapsed)) ** 0.5
    ci = 1.96 * std / (len(elapsed) ** 0.5)
    print(instance, returncode, status, avg, ci)
    print(instance, returncode, status, None, None)

  exit(int(status not in ['interrupted'] + VALID_SOLUTIONS))

Well, firstly, don't be afraid of empty lines. This code is quite hard to read.

Secondly, functions is good invention. ;) That code as it is now is a bare piece of code, with no indication of where most of the parameters come from, and it seems that the acceptable solutions is a global, as you have capitalized it? That doesn't make much sense to me.

Just as you have a function called run() to run the program, it would make sense to have a function to run it multiple times, with the tests for that you have.

I'm also not sure why you trap KeyboardInterrupt, without seeming to actually do anything with it, except setting the status, and then testing the status. You could just do exit directly from that exception handler, that would be clearer.

Otherwise it seems fine. Feedback on correctness is impossible when your code isn't a running example. But then again, tests generally work better than code reviews there, although they are not exclusive.

  • \$\begingroup\$ Thanks for the feedback. KeboardInterrupt because the script is run inside an infinite loop which stops when the output is invalid. Can't exit directly since exit() throws an exception (SystemExitError I think). \$\endgroup\$ – Alexandru Feb 1 '11 at 14:04
  • \$\begingroup\$ +1 for adding whitespace to enhance readability. Also moving code to separate functions would help. \$\endgroup\$ – Eric Bréchemier Feb 1 '11 at 14:26

Comments. Well written code tells you how a problem is being solved. Well written comments tell you what problem is being solved. Great comments tell you what assumptions the programmer is making about the data going in and coming out.

In the finally clause: You have a magic constant. Best practice is that you declare constants with descriptive variables names.

The variables 'r' and 'e': no single letter variable names please, save for those that are so basic no one will get them wrong, which is basically x,y,z and t, in the context for cartesian coordinates or parameterized plots. And 'e' was an exception a few lines ago, now it's a list element?

Lastly, x**.5 is accurate, but sqrt(x) is more readable.


Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.