Function for getting strings consisting of random characters

Question

I've made this function for to be used while testing.

// ----- THE ACTUAL FUNCTION -------------------------
		
// Returns a string consisting of random alphabetic
//  characters.

// @param { string } separator - Char which shall be 
//  used to separate the single chars.
// @param { number } min - Minimal length of the string.
// @param { number } max - Maximal length of the string.
// @param { string } inCase - Case of characters to use.
//  - Use 'both' for getting a string consisting of 
//   lower- and upper-case character.
//  - Use 'upper' for getting a string consisting of 
//   only upper-case characters.
//  - Leave the parameter out, pass an empty string or
//   any other string for getting a string consisting
//   of only lower characters.
// @return { string } 
// @throws { object } in case of error.
		
function getRandomString(separator, min, max, inCase) {
  var getPartRandomString = function() {
    return Math.random()
    .toString(36)
    .replace(/[^a-z]+/g, '');
  };

  var setReturnString = function() {
    ret += getPartRandomString();

    if (ret.length < min) {
      setReturnString();
    }
  }

  var ret = '';

  if (min >= max) {
    throw {
      message: 'Minimum can not be greater or equal Maximum.'
    }
  }

  min = min || 5;
  max = max || 8;

  separator = separator || '';

  setReturnString();

  ret = ret.split('');

  if (inCase === 'both') {
    ret = ret.map(function(currentChar) {
      if (Math.random() >= 0.5) {
        currentChar = currentChar.toUpperCase();
      }

      return currentChar;
    });
  }

  ret = ret.join(separator);

  if (inCase === 'upper') {
    ret = ret.toUpperCase();
  }

  ret = ret.slice(0, ret.length <= max ? ret.length : max);

  if (separator && ret.charAt(ret.length - 1) === separator) {
    ret = ret.slice(0, ret.length - 1);
  }

  return ret;
}
// ----- THE ACTUAL FUNCTION ---- End ----------------------

function testGetRandomString(min, max, caseToTest, limit, regExp) {
  var logMessage = function(message) {
    console.log('%s%s => %s', 
                receivedResult,
                new Array(max - receivedResult.length + 1)
                .join(' '),
                message
               );
  }

  var receivedResult;

  min = min || 6;
  max = max || 12;
  limit = limit || 10;
  regExp = regExp || /[^a-z]/g;

  for (let i = 0; i < limit; i++) {
    receivedResult = getRandomString('', min, max, caseToTest);

    if (receivedResult.length < min) {
      logMessage('Less then min.');
    } else if (receivedResult.length > max) {
      logMessage('Larger or equal max.');
    } else if (receivedResult.search(regExp) !== -1) {
      logMessage('Unallowed chars.');
    } else {
      logMessage('Test passed.');
    }
  }
}

testGetRandomString(6, 12, '', 10);
console.log('\n');
testGetRandomString(6, 12, 'both', 10, /[^a-zA-Z]/g);
console.log('\n');
testGetRandomString(10, 18, 'upper', 10, /[^A-Z]/g);

console.log('\n');
console.log(getRandomString('-', 1, 2, 'both'));

console.log('\n');
console.log(getRandomString('-', 10, 20, 'both'));

It seem to work alright. But frankly: With all the parameter-checking and taking different possibilities into account it has become a lot of code.

If someone knows a possibility to write it more elegant then I would appreciate his or her answer. Any other hints welcomed too.

Should mention: I can't use ES 6-syntax because we got only Internet Explorer 11 here and there won't be change in any nearer future.

Answer 1

My immediate thought is that the function is just doing too many things.

It'd be simpler to just return a mixed-case string of a specific length. Done.

Want that string to be all-uppercase? Just call getRandomString(n).toUpperCase().
All-lowercase. Call getRandomString(n).toUpperCase().

Want it to have a separator between each letter? Call getRandomString(n).split('').join(separator).

Want it be be somewhere between x and y characters long? Pick a random length yourself, and pass that to the function.

This just seems like a function built for a very narrow purpose. For instance, what if (for whatever reason) you only want a separator every 4 characters? Do you add yet another argument to the function, or do you write a separate function to chopping and glueing the way you want it?

Even if you need a single function to do everything, it'd be simpler to have it just call out to other functions that comprise the building blocks. Lots of the functions you need already exist (toUpperCase, toLowerCase, split, and join), so it's only the randomness you need to add.

A simple and crude structure might be:

function getRandomInt(min, max) {
  var span = parseInt(max, 10) - parseInt(min, 10);
  if(span <= 0) {
    throw { message: 'Min must be less than max.' };
  }
  return min + Math.round(Math.random() * span);
}

function getRandomLetter() {
  var asc = getRandomInt(65, 116);
  if(asc > 90) asc += 6;
  return String.fromCharCode(asc);
}

function getRandomString(length) {
  var i, letters = [];
  length = parseInt(length, 10);

  for(i = 0 ; i < length ; i++) {
    letters.push(getRandomLetter());
  }
  return letters.join('');
}

// this is the equivalent of original function
function getCustomRandomString(separator, min, max, style) {
  var string = getRandomString(getRandomInt(min, max));

  if(style === 'upper') {
    string = string.toUpperCase();
  } else if(style != 'both') {
    string = string.toLowerCase();
  }

  if(separator) {
    string = string.split('').join(separator);
  }

  return string;
}

Answer 2

The main piece of complexity comes from the way you are converting Math.random() to a sequence of letters. Although the solution is short, as you have found out, the number of letters you get back is completely random, which is troublesome. (It's not even clear to me if converting to radix 36 will give you a uniform distribution of letters.) It also makes all of the upper/lower stuff complicated because you end up manipulating a result that isn't exactly what you want.

It is much simpler to do all of your argument processing first, figure out exactly what you want, and then call Math.random() for each character.

function getRandomString(separator, min, max, inCase) {
  if (min >= max) {
    throw new Error ('Minimum can not be greater or equal Maximum.');
  }
  min = min || 5;
  max = max || 8;

  separator = separator || '';

  var numCharsDesired = min + (Math.random() * (max - min));

  var lowers = 'abcdefghijklmnopqrstuvwxyz';
  var uppers = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ';
  var charset;
  if (inCase === 'both') {
    charset = lowers + uppers;
  } else if (inCase === 'upper') {
    charset = uppers;
  } else {
    charset = lowers;
  }

  var retArray = [];
  for (var i = 0; i < numCharsDesired; i++) {
    var index = Math.floor(Math.random() * charset.length);
    retArray.push(charset[index]);
  }
  return retArray.join(separator);
}

The only tricky thing about this is Math.floor(Math.random() * charset.length). This multiplies a random number between 0, inclusive, and 1, noninclusive, with the length of whatever set of characters we want to take from (and then takes the floor so we have an integer between 0 and length-1).

A few other comments:

Although you can technically rely on hoisted variables, it is awfully hard to read code like this. You use ret before it is declared, which is very confusing, because you've got to scan down to see where it is. Declare first, then use. Alternately, pass it in as an argument so you're not using side effects.

Which brings me to my next suggestion: your SetReturnString function combines side-effects and recursion. A more straightforward approach is normal iteration: while (ret.length < x) { ret += getPartRandomString();. You can eliminate the function altogether.