An in memory Url Shortener in Java

Question

This is a popular question in machine coding rounds. The requirements are to build a URL shortener. There are certain registered users who have the capability to define a URL and an optional time to live. The system generates a shortened URL. The unregistered users can also generate a shortened URL with a predefined TTL. This is the URLshortener Class.

package urlshortener;
import hashalgorithm.IHashStrategy;
import java.util.*;
public class UrlShortener {
 private IHashStrategy iHashStrategy;
 private Map<String, List<Url>> userToUrl;
 private List<String> registeredUser;
 static final int DEFAULT_TIME_TO_LIVE_IN_MILLIS = 500000;
 public UrlShortener(IHashStrategy instanceOfConcreteHashStrategy) {
 iHashStrategy = instanceOfConcreteHashStrategy;
 userToUrl = new HashMap<>();
 registeredUser = new ArrayList<>();
 registeredUser.add("abc");
 registeredUser.add("bad");
 registeredUser.add("bac");
 registeredUser.add("lom");
 registeredUser.add("DEFAULT");
 }
 public List<Url> getListOfShortenedUrlForUser(String userId) {
 List<Url> urlList = getUrls(userId);
 userToUrl.put(userId,urlList);
 return urlList;
 }
 public void addNewEntryForUser(String url,String userId) {
 addNewEntryForUserWithTTL(url,userId,DEFAULT_TIME_TO_LIVE_IN_MILLIS);
 }
 private boolean checkExistenceForShortenedUrl(String shortenedUrl) {
 for(Map.Entry<String,List<Url>> entry : userToUrl.entrySet()) {
 List<Url> urlList = entry.getValue();
 for(int i = 0 ; i < urlList.size() ; i++) {
 if(urlList.get(i).getShortenedUrl().equalsIgnoreCase(shortenedUrl))
 return true;
 }
 }
 return false;
 }
 public void addNewEntryForUserWithTTL(String url,String userId,int timeToLive) {
 if(!registeredUser.contains(userId)) {
 System.out.println("userId is not registered. Treated as default!");
 userId = "DEFAULT";
 }
 List<Url> urlList = userToUrl.get(userId);
 String shortenedUrl = iHashStrategy.getHashFromUrl(url);
 while(checkExistenceForShortenedUrl(shortenedUrl)) {
 shortenedUrl = iHashStrategy.getHashFromUrl(url);
 }
 System.out.println(shortenedUrl);
 Url urlToInsert = new Url(url,shortenedUrl,timeToLive,new Date());
 if(urlList == null)
 urlList = new ArrayList<>();
 urlList.add(urlToInsert);
 userToUrl.put(userId,urlList);
 }
 public String getShortenedUrlForUser(String url,String userId) {
 if(!registeredUser.contains(userId)) {
 System.out.println("userId is not registered. Treated as default!");
 userId = "DEFAULT";
 }
 List<Url> urlList = getUrls(userId);
 for(int i = 0 ; i < urlList.size(); i++) {
 if(urlList.get(i).getOriginalUrl().equalsIgnoreCase(url))
 return urlList.get(i).getShortenedUrl();
 }
 return null;
 }
 private List<Url> getUrls(String userId) {
 List<Url> urlList = userToUrl.get(userId);
 for (int i = 0; i < urlList.size(); i++) {
 Url currentUrl = urlList.get(i);
 if (!currentUrl.isValid()) {
 urlList.remove(i);
 }
 }
 return urlList;
 }
 public String getLongUrlForUser(String shortUrl,String userId) {
 List<Url> urlList = getUrls(userId);
 for(int i = 0 ; i < urlList.size(); i++) {
 if(urlList.get(i).getShortenedUrl().equalsIgnoreCase(shortUrl));
 return urlList.get(i).getOriginalUrl();
 }
 return null;
 }
}

And this is URL.java

package urlshortener;
import java.util.Date;
public class Url {
 private String originalUrl;
 private String shortenedUrl;
 private int timeToLive;
 private Date date;
 public Url(String originalUrl, String shortenedUrl, int timeToLive, Date date) {
 this.setShortenedUrl(shortenedUrl);
 this.setTimeToLive(timeToLive);
 this.setOriginalUrl(originalUrl);
 this.setDate(date);
 }
 public boolean isValid() {
 Date timeNow = new Date();
 Date expireDate = new Date(this.getDate().getTime() + getTimeToLive());
 return timeNow.before(expireDate);
 }
 public String getOriginalUrl() {
 return originalUrl;
 }
 public void setOriginalUrl(String originalUrl) {
 this.originalUrl = originalUrl;
 }
 public String getShortenedUrl() {
 return shortenedUrl;
 }
 public void setShortenedUrl(String shortenedUrl) {
 this.shortenedUrl = shortenedUrl;
 }
 public int getTimeToLive() {
 return timeToLive;
 }
 public void setTimeToLive(int timeToLive) {
 this.timeToLive = timeToLive;
 }
 public Date getDate() {
 return date;
 }
 public void setDate(Date date) {
 this.date = date;
 }
}

For the rest of the code please refer to https://github.com/suryasis-hub/urlshortener/tree/master/src

• dariosicily: Welcome to Code Review. You are using Date class , some reason to not use java time API ?

Answer 1

Usability

In your github mainclass, you're asking the user:

System.out.println("Enter command type");

Then expecting them to enter a number. Consider adding some kind of menu so that people that haven't used the application before know what number 3 does before selecting it and getting the message 'UnRegistered user wants to enter a website'.

Usage

You haven't specified how you're expecting your URL shortener to be used. To me, the current setup doesn't really make sense. The reason I shorten a URL is so that I can share it with other people, so I'm expecting two main uses:

1. Create short URL from real URL, as a registered/unregistered user
2. Get real URL from short URL

The way you've coded it, it looks like as a registered user, only I can access my shortened URL mappings. This seems flawed in a practical usage sense.

Time To Live

You've got two public methods on the class

public void addNewEntryForUser(String url,String userId) {
public void addNewEntryForUserWithTTL(String url,String userId,int timeToLive) {

The first simply calls the second passing in the default TTL. According to your requirements, only registered users should be able to set a TTL, however the class supports anybody setting a TTL, as long as they call the method with the time to live parameter. This might be OK, but it really depends on who owns the logic to use a default TTL for certain users. At the moment that's not clear. I'd also consider overloading these methods as addNewEntryForUser, I don't think having the WithTTL in the method name really gives you anything over the extra parameter.

Time to Die

There doesn't appear to be any expiry logic - Not really true, but it's not obvious. Rather than calling the method isValid, something like isExpired, isTTLExpired would make it more obvious that the expiration is triggered by this method.

Responsibilities

It feels like your shortener is doing too much. It could just shorten URLs. However, it's also responsible for storing a list of registered users, the list of all shortened URLs by user, setting TTL. At a minimum, I'd extract the registered user list from the class and provide some way to get it via the classes constructor (either a repository, or even just a supplied list) to give some separation. Hard coded user Id's really shouldn't be in this class.

Answer 2

I'm concentrating on...

Naming

private IHashStrategy iHashStrategy;

The name iHashStrategy tells nothing about the purpose of the field. It simply repeats the name of the type. The field is used for hashing the URLs so urlHashStrategy would be a more descriptive field name.

Since the IHashStrategy class is in hashalgorithm package, nothing in their names suggest that the interface is specific to hashing URLs (I'm assuming this is the case because the interface contains a very specific getHashFromUrl(String) method). UrlHashStrategy would be a better name. I personally do not prefer the I-prefix in interfaces as it is not something that is done in the Java standard libraries. I would probably separate the hashing completely from the fabrication of shortened URLs. That way the domain (and possible URL path prefixes) could be configured separately from the hash algorithm. The ShortenedUrl class would then contain the original URL and the hash only.

public UrlShortener(IHashStrategy instanceOfConcreteHashStrategy) {

The parameter name instanceOfConcreteHashStrategy is incredibly long considering that it provides absolutely no additional information. :) An object that is passed around is always a pointer to an instance of a concrete class and the type can already be inferred from the parameter type. A plain urlHashStrategy would be a big improvement in the naming.

public List<Url> getListOfShortenedUrlForUser(String userId) {

I find repeating the return type in the method name redundant. If the Url class had a more descriptive name this could be List<ShortenedUrl> getShortenedUrls(String userId). The plural implies that the method returns a collection.

public void addNewEntryForUser(String url,String userId) {

You're not adding a generic entry. This would be more descriptive as shortenUrl(String url,String userId). Unless you're planning on adding anonymous URL shortening, there is no point in repeating the ForUser part. And even if you planned, the method can be overloaded with different parameters. This is also a method that probably would benefit from returning the shortened URL as a return value.

private boolean checkExistenceForShortenedUrl(String shortenedUrl) {
...
while(checkExistenceForShortenedUrl(shortenedUrl)) {

The checkExistenceForShortenedUrl name does not imply anything about the return value. It just states that something is checked. If renamed isDuplicate the name would signal a boolean return value and the code would be more fluent: while (isDuplicate(shortenedUrl)) {. But the whole concept of duplicate hashes is pretty novel. Most URL shorteners use the same hash for a given URL.

Calling the operation of shortening a URL "hashing" is misleading, as hashing is a stable operation: the same input always produces the same output. So calling a hash algorithm in a loop with same input until it produces a different output would be a never-ending loop. If you're intent on providing different shortened URLs for the same long URL for different users, you're looking for a random number generator, not a hashing strategy.

public class Url {

This class does not represent a URL. Since a URL is a very specific concept (and Java already has a URL class) the name is very confusing. When your method returns a Url the reader expects it to be a URL, not a container for two URLs and a TTL. This should be renamed to ShortenedUrl. I would also offload the TTL to the registry class that keeps track of the URLs that have been shortened by different users. Once you separate these two responsibilities you can also easily reuse the hashes and use the same instance for all users who want to pass the latest meme around.

Once you have removed the TTL from the ShortenedUrl class, the responsibility of storing return values for a given time starts to sound a lot like a cache. If the personalized TTL for a shortened URL is not an important feature, you could replace the TTL management with a ready made cache that covers your URL shortener.

Answer 3

Thread safety

getUrls() is modifying a collection (urlList.remove(i)), without any locking on the list. This is a potential thread safety problem. if multiple threads will attempt to access the method (and consequently modify the list), it is possible that the result will be wrong.

Collection iteration

your collection processing uses the old int index mechanism. this is both inefficient (when it comes to large collections) and too verbose. you should convert all iterations to Java 8 collection streams.

here is an example:

private boolean checkExistenceForShortenedUrl(String shortenedUrl) {
 return userToUrl.values().stream()
 .anyMatch(urlList -> urlList.stream().anyMatch(
 url -> url.getShortenedUrl().equalsIgnoreCase(shortenedUrl)
 ));
}