Some bad code in amrfinder

[andrewdavidsmith]

The code to prevent problems when no amrs are identified is not good:

if (amrs.size() == 0) { cout << "No AMRs found.\n"; return 0; }

This prints a message to cout no matter what, and deviates from our common way of only having the program exit at the bottom of main, unless it has to do with an error (return code not zero) or in parsing options.

[bdecato]

Good catch, the message to standard out probably should have been printed to the output stream specified in the arguments. I know it is bad practice to introduce another exit point, but the way I see it, we have four options:

1) Enclose code that uses the amrs vector in a try catch 2) Enclose the code that uses the amrs vector in an if statement 3) Use a goto statement inside the conditional to goto the exit point. This has been described to me as a good way to keep one exit point, but I couldn't bring myself to write the word goto. 4) Do what I did, and introduce another exit point.

1 doesn't really make sense, since we're not catching an exception: it's a natural case. 2 makes sense, but was ugly stylistically to me. 3 is probably the best practice, but I do not know our lab's policy on it 4 is what I did.

What should we be doing in situations like this?

On Sun, May 25, 2014 at 3:22 PM, Andrew Smith notifications@github.comwrote:

The code to prevent problems when no amrs are identified is not good:

if (amrs.size() == 0) { cout << "No AMRs found.\n"; return 0; }

This prints a message to cout no matter what, and deviates from our common way of only having the program exit at the bottom of main, unless it has to do with an error (return code not zero) or in parsing options.

— Reply to this email directly or view it on GitHubhttps://github.com/smithlabcode/methpipe/issues/14 .

Benjamin Decato PhD Student Computational Biology Section, Department of Biological Sciences, University of Southern California 1050 Childs Way, RRI 408A Los Angeles, CA 90089-2910

[andrewdavidsmith]

if (!amrs.empty()) {

}

On Sun, May 25, 2014 at 4:55 PM, bdecato notifications@github.com wrote:

Good catch, the message to standard out probably should have been printed to the output stream specified in the arguments. I know it is bad practice to introduce another exit point, but the way I see it, we have four options:

1) Enclose code that uses the amrs vector in a try catch 2) Enclose the code that uses the amrs vector in an if statement 3) Use a goto statement inside the conditional to goto the exit point. This has been described to me as a good way to keep one exit point, but I couldn't bring myself to write the word goto. 4) Do what I did, and introduce another exit point.

1 doesn't really make sense, since we're not catching an exception: it's a natural case. 2 makes sense, but was ugly stylistically to me. 3 is probably the best practice, but I do not know our lab's policy on it 4 is what I did.

What should we be doing in situations like this?

On Sun, May 25, 2014 at 3:22 PM, Andrew Smith notifications@github.comwrote:

The code to prevent problems when no amrs are identified is not good:

if (amrs.size() == 0) { cout << "No AMRs found.\n"; return 0; }

This prints a message to cout no matter what, and deviates from our common way of only having the program exit at the bottom of main, unless it has to do with an error (return code not zero) or in parsing options.

— Reply to this email directly or view it on GitHub< https://github.com/smithlabcode/methpipe/issues/14> .

Benjamin Decato PhD Student Computational Biology Section, Department of Biological Sciences, University of Southern California 1050 Childs Way, RRI 408A Los Angeles, CA 90089-2910

— Reply to this email directly or view it on GitHubhttps://github.com/smithlabcode/methpipe/issues/14#issuecomment-44149268 .

[pjuren]

Yep, I think that's the canonical way of dealing with code that doesn't work if something is empty... Then if you need to do something for when it is empty, like print a warning message to stderr (if in verbose mode), add an else block.

[bdecato]

Fixed as suggested above.