3
\$\begingroup\$

I have a bit of code that I would like your collective opinions on. Currently, my working code has two return statements. I'm not sure how I feel about having multiple points of exit, however, the alternative introduces the inclusion of else, which looks like it will eventually be harder to maintain if more logic is added to the code later on.

My current code (Feel free to comment on other aspects as well):

function generateFileName() {
    var k = 0;
    var today = new Date();
    today = (today.getMonth() + 1).toString() + '-' + today.getDate().toString()
            + '-' + today.getFullYear().toString();
    while (true) {
        if (!fs.existsSync('./'+ today + '.pdf')) {
            return './' + today + '.pdf';
        }

        if (!fs.existsSync('./' + today + '(' + k + ').pdf')) {
            return './' + today + '(' + k + ').pdf';
        }

        k++;
    }
}

As you see, here there are two different return statements. If the file does not exist in the system, a file will be generated with the name 5-19-2017.pdf, i.e., the current date. If there is already a file named that, the file will be named 5-19-2017 (0).pdf, then 5-19-2017(1).pdf, and so on...

This code works just fine, however - I am wondering if maybe a single point of exit might be better, such as :

function generateFileName() {
    var k = 0;
    var today = new Date();
    today = (today.getMonth() + 1).toString() + '-' + today.getDate().toString() + '-' +today.getFullYear().toString();
    var result = today;
    var stop = false;
    while (!stop) {
        if (fs.existsSync('./'+ today + '.pdf')) {
            if (fs.existsSync('./' + today + '( ' + k + ' ).pdf')){
                k++;
            } else {
                result = today + '( ' + k + ' )';
                stop = true;
            }
        }
        else {
            stop = true;
        }
    }
    return result;
}

Now, the code has a single point of exit, however the added else statements make the code unbearable to read, in my opinion.

Thoughts?

\$\endgroup\$
4
  • 1
    \$\begingroup\$ @200_success I'm extremely confused as to how it could be unclear what I am asking, care to elaborate? \$\endgroup\$
    – Jaken
    May 19, 2017 at 17:58
  • 1
    \$\begingroup\$ Code Review is not the place to discuss best practices in general (e.g. single point of return), as your title suggests. However, if you were to reframe this question to state the purpose of the code (as per the How to Ask) guidelines, then we could review your function to generate a filename based on the date. \$\endgroup\$ May 19, 2017 at 17:59
  • 1
    \$\begingroup\$ @le_m Yes, when the question is to review code. But not if the main point of the question is to discuss multiple-vs-single return, as the title suggests. \$\endgroup\$ May 19, 2017 at 18:31
  • 1
    \$\begingroup\$ @le_m Reopened. \$\endgroup\$ May 21, 2017 at 1:25

1 Answer 1

3
\$\begingroup\$

Your code can be made much simpler:

var basename = today;
var filename;
for (var k = 0; fs.existsSync(filename = './' + basename + '.pdf'); k++) {
    basename = today + ' (' + k + ')';
}
return filename;
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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