HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

UrlRouter which uses regular expressions to find out what to do

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
urlrouterwhatregularexpressionsusesfindwhichout

Problem

I have created UrlRouter. It should call appropriate controllers methods with extracted values as a parameters. It works fine, but my boss told me that I should make it better. Could you give me some advice on how to improve this class, please?

```
public class UrlRouter {

private final static String pageRegex = "/page/\\d{1,5}";
private final static String yearRegex = "/\\d{4}";
private final static String monthRegex = "/\\d{1,2}";
private final static String nameRegex = "/[a-z0-9\\-]+";

private final static Pattern page = Pattern.compile(pageRegex + "$");
private final static Pattern year = Pattern.compile(yearRegex + "$");
private final static Pattern yearPage = Pattern.compile(yearRegex + pageRegex + "$");
private final static Pattern yearMonth = Pattern.compile(yearRegex + monthRegex + "$");
private final static Pattern yearMonthPage = Pattern.compile(yearRegex + monthRegex + pageRegex + "$");
private final static Pattern yearMonthName = Pattern.compile(yearRegex + monthRegex + nameRegex + "$");
private PostController postController;

public UrlRouter(PostController controller) {
this.postController = controller;
}

public void route(HttpServletRequest request) {
String url = request.getRequestURI();

if (yearMonthName.matcher(url).find()) {
showOne(url);
} else if (yearMonthPage.matcher(url).find()) {
showMonthList(url);
} else if (yearMonth.matcher(url).find()) {
showMonth(url);
} else if (yearPage.matcher(url).find()) {
showYearList(url);
} else if (year.matcher(url).find()) {
showYear(url);
} else if (page.matcher(url).find()) {
showPage(url);
} else {
postController.showPage(1);
}
}

private void showPage(String url) {
String[] parts = url.split("/");
int pageNumber = Integer.parseInt(parts[parts.length-1]);

Solution

The first improvement you definitely want is capturing groups. These groups allow you to get rid of splitting at "/" and working on magic indices and instead use a Matcher to extract the parts from a matched string.

This greatly simplifies your delegating methods that work on a String to using the Matcher instance you used and threw away in your if-condition:

private void showPage(Matcher url) {
    postController.showPage(url.group(1));
    // alternatively use
    postController.showPage(Integer.parseInt(url.group(1)));
}


This would only require minor changes for your regexes:

You'd need to enclose the captured groups in parentheses as follows.

private final static String pageRegex = "/page/(\\d{1,5})";
private final static String yearRegex = "/(\\d{4})";
private final static String monthRegex = "/(\\d{1,2})";
private final static String nameRegex = "/([a-z0-9\\-]+)";

Code Snippets

private void showPage(Matcher url) {
    postController.showPage(url.group(1));
    // alternatively use
    postController.showPage(Integer.parseInt(url.group(1)));
}
private final static String pageRegex = "/page/(\\d{1,5})";
private final static String yearRegex = "/(\\d{4})";
private final static String monthRegex = "/(\\d{1,2})";
private final static String nameRegex = "/([a-z0-9\\-]+)";

Context

StackExchange Code Review Q#110164, answer score: 4

Revisions (0)

No revisions yet.