Here is some code I’ve written to save a UrlEntity :
public UrlEntity saveUrlEntity(String longUrl, LocalDate dateAdded) {
int urlLength = longUrl.length();
if (urlLength >= Constants.MAX_LONG_URL_LENGTH) {
throw new LongUrlLengthExceededException("URL with length " + urlLength + " exceeds the max length of " + Constants.MAX_LONG_URL_LENGTH + " characters");
} else {
List<UrlEntity> urlEntity = urlRepository.findByLongUrl(longUrl);
if (urlEntity.size() > 0) {
return urlEntity.get(0);
} else {
final String shortUrl = urlShorten.shortenURL(longUrl);
if (urlRepository.findFirstByShortUrl(shortUrl).isPresent()) {
logger.error("A short short URL collision occured for long URL: " + longUrl + " with generated short URL" + shortUrl);
throw new ShortUrlCollisionException("A short URL collision occured");
} else {
logger.info("Shortened URL: " + shortUrl);
final UrlEntity urlEntityToSave = new UrlEntity(dateAdded, longUrl, shortUrl);
return urlRepository.save(urlEntityToSave);
}
}
}
}
The above code exists in a service class and looks unnecessarily complex. I’m attempting to refactor so that the intent is clear. This is a basic refactoring I’ve written:
public UrlEntity saveUrlEntity(String longUrl, LocalDate dateAdded) {
int urlLength = longUrl.length();
if (urlLength >= Constants.MAX_LONG_URL_LENGTH) {
throw new LongUrlLengthExceededException("URL with length " + urlLength + " exceeds the max length of " + Constants.MAX_LONG_URL_LENGTH + " characters");
} else {
List<UrlEntity> urlEntity = urlRepository.findByLongUrl(longUrl);
if (urlEntity.size() > 0) {
return urlEntity.get(0);
} else {
return saveUrlEntityValue(longUrl, dateAdded);
}
}
}
private UrlEntity saveUrlEntityValue(String longUrl, LocalDate dateAdded){
final String shortUrl = urlShorten.shortenURL(longUrl);
if (urlRepository.findFirstByShortUrl(shortUrl).isPresent()) {
logger.error("A short short URL collision occured for long URL: " + longUrl + " with generated short URL" + shortUrl);
throw new ShortUrlCollisionException("A short URL collision occured");
} else {
logger.info("Shortened URL: " + shortUrl);
final UrlEntity urlEntityToSave = new UrlEntity(dateAdded, longUrl, shortUrl);
return urlRepository.save(urlEntityToSave);
}
}
This refactoring does not improve the code substantially. Is there a code pattern or idiomatic way to refactor the method saveUrlEntity
? I’m using Java11
Advertisement
Answer
This is very subjective, but… Since most of your if
statements are guard/short-circuit clauses, which throw
or return
, there is no need to use else
. I think this simple change makes the code much more readable.
public UrlEntity saveUrlEntity(String longUrl, LocalDate dateAdded) {
final int urlLength = longUrl.length();
if (urlLength >= Constants.MAX_LONG_URL_LENGTH) {
throw new LongUrlLengthExceededException("URL with length " + urlLength + " exceeds the max length of " + Constants.MAX_LONG_URL_LENGTH + " characters");
}
final List<UrlEntity> urlEntity = urlRepository.findByLongUrl(longUrl);
if (urlEntity.size() > 0) {
return urlEntity.get(0);
}
final String shortUrl = urlShorten.shortenURL(longUrl);
if (urlRepository.findFirstByShortUrl(shortUrl).isPresent()) {
logger.error("A short short URL collision occured for long URL: " + longUrl + " with generated short URL" + shortUrl);
throw new ShortUrlCollisionException("A short URL collision occured");
}
logger.info("Shortened URL: " + shortUrl);
final UrlEntity urlEntityToSave = new UrlEntity(dateAdded, longUrl, shortUrl);
return urlRepository.save(urlEntityToSave);
}
I’d also recommend replacing urlEntity.size() > 0
with !urlEntity.isEmpty()
.
The method does seem to be doing several things, which violates the Single Responsibility Principle; you might want to think about breaking that out better.