3
\$\begingroup\$

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<String,String> urlMap = new HashMap<String,String>();

    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(tinyUrl)){        
       for(Entry<String,String> entry:urlMap.entrySet()){
         if(entry.getValue().equals(tinyUrl)){
           inputUrl = entry.getKey();
         }
       }
      }
      return inputUrl;
    }
    /*
     * Return random number for the limit
     * */
    private int getRandonNumber(int lowLimit, int highLimit){
        int random = 0;
        Random randomObj = new Random();
        do{
            random = randomObj.nextInt(highLimit);

        }while(!(random >= lowLimit && random <= highLimit) ||
                (random > 57 && random < 65) || 
                (random > 90 && random <97));

        return random;
    }

}
\$\endgroup\$
4
\$\begingroup\$

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<String> 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.
\$\endgroup\$
  • \$\begingroup\$ Can you explain the below code in a simple form? I am guessing this one is using Java 8 lamda expressions. <br/> URLTinyMe url = URLTinyMe.getInstance((x, y) -> 60); // inject dummy random object that always returns 60 List<String> urls = new ArrayList<>(); for (int i = 0; i < 500; i++) { urls.add(url.getTinyMeUrl("foo" + i)); } Assert.assertTrue(!hasDuplicates(urls)); \$\endgroup\$ – Ajo Paul Mar 24 '15 at 17:37
  • 1
    \$\begingroup\$ @AjoPaul the Assert.assertTrue is part of JUnit which provides unit tests for Java. And (x, y) -> 60 is indeed using lamda, it just stands for an anonymous class implementation of the Random interface you would have to create: new Random() {int getRandonNumber(int lowLimit, int highLimit){return 60;}}. \$\endgroup\$ – tim Mar 24 '15 at 17:47

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.