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\$
  • 1
    \$\begingroup\$ @200_success I'm extremely confused as to how it could be unclear what I am asking, care to elaborate? \$\endgroup\$ – Jaken Herman May 19 '17 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\$ – 200_success May 19 '17 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\$ – 200_success May 19 '17 at 18:31
  • 1
    \$\begingroup\$ @le_m Reopened. \$\endgroup\$ – 200_success May 21 '17 at 1:25
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, privacy policy and cookie policy

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