Java class that generates a short URL string for an input string

Help me find out if the approach I took to generate random strings here is correct to be able to return the appropriate short URLs.

```
import java.util.HashMap;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Random;
/**
*
* Given a input string create a random
* string
*
* @author
* @creation Mar 23, 2015
*
*/
public class URLTinyMe {

/Map stores 1:1 mapping of input url and tinyurl/
private Map urlMap = new HashMap();

private static URLTinyMe urlTinyMeObj = null;

/**
* return static instance of URLTinyMe
* @return
*/
public static URLTinyMe getInstance(){
if(null == urlTinyMeObj){
synchronized (URLTinyMe.class){
if(null == urlTinyMeObj){
urlTinyMeObj = new URLTinyMe();

}
}
}
return urlTinyMeObj;
}

/*
* Private constructor, restrict instance creation
*/
private URLTinyMe() {

}

/**
* TinyMefy a URL
* @return String
*/
public String getTinyMeUrl(String inputUrl){

String tinyUrl = inputUrl;

if(urlMap.containsKey(inputUrl)){//Check if url already exists
tinyUrl = urlMap.get(inputUrl);
}else{
StringBuilder charList = new StringBuilder();
do{
int random = getRandonNumber(48,122);//Accept only characters and numericals A-Z,0-9,a-z
charList.append((char)random);
}while(urlMap.containsValue(charList.toString()));

urlMap.put(inputUrl, charList.toString());
tinyUrl = charList.toString();
}
return tinyUrl;
}

/**
* From the tinyUrl return the original input url
* @param tinyUrl
* @return
*/
public String getOriginalUrl(String tinyUrl){
String inputUrl = null;
if(urlMap.containsValue(ti

Unit Tests and Single Responcibility

It's always good to test your code. With random data, this is a bit complicated, but at least your original mistake of the same input generating different urls would have been caught by tests.

Now the question becomes how to test that there will be no duplicates when randomness is involved. And the easy answer would be to remove the randomness from it.

As creating random numbers shouldn't be the responsibility of this class anyways (it's already responsible for managing urls), you could create a Random interface (with int getRandonNumber(int lowLimit, int highLimit) as method) whose instance you can inject into the class (via the getInstance(Random random) method). For unit tests, you can then simply create a dummy random object which returns a static value. Your test would then look something like this:

URLTinyMe url = URLTinyMe.getInstance((x, y) -> 60); // inject dummy random object that always returns 60
List urls = new ArrayList<>();
for (int i = 0; i < 500; i++) {
    urls.add(url.getTinyMeUrl("foo" + i));
}
Assert.assertTrue(!hasDuplicates(urls));

Additional tests would be:

// same url creates same tiny url
Assert.assertEquals(url.getTinyMeUrl("foo"), url.getTinyMeUrl("foo"));

// getOriginalUrl works
String test = "foobar";
Assert.assertEquals(test, url.getOriginalUrl(url.getTinyMeUrl("foobar")));

Misc

• Unnecessary assignments just complicate your code. String tinyUrl = inputUrl; isn't needed.

• Your getRandonNumber doesn't do what it says it does, as it also filters out numbers in between the limits. Either change how the method works, or change the comment. I would probably get rid of the parameters, and rename the method to something that expresses that only alphnum is generated.

• Your spacing and indentation is off (and not even internally consistent), which makes your code harder to read.

• I don't think that you will gain any performance increase from using a StringBuilder if you call toString each time you append something.

• You should probably throw an exception if the original url could not be found.

StackExchange Code Review Q#84812, answer score: 4