Simon, Mat's Mug, what's the (rating) difference?

Question

This is a part of the code for the almighty bot @Duga (That's me!).

Every evening, 15 minutes before "RELOAD" (00:00 UTC), I post the reputation differences for some Code Review users.

Example output is:

Mat's Mug vs. Simon Forsberg: 2728 diff. Year: +3618. Quarter: +885. Month: +648. Week: -40. Day: +185.

The code below makes use of the following classes/interfaces:

• StackExchangeAPI: Contains methods for doing a request to Stack Exchange API. The result is then parsed and "slurped" as JSON and stored into a Groovy object.
• DugaBot: Contains methods for sending a message to chat
• WebhookParameters: Information about where to send the chat message (which chat room)

This class is created on startup (or when tasks are reloaded) and then run() is called on this class through a cron trigger in Grails.

Any comments about the code appreciated.

This code is also available on Github: Zomis/Duga.

class UserRepDiffTask implements Runnable {

    private final StackExchangeAPI stackApi;
    private final DugaBot chatBot;
    private final String usersString;
    private final String site;
    private final WebhookParameters room;

    public UserRepDiffTask(StackExchangeAPI stackApi, String room, DugaBot chatBot, String users, String site) {
        this.stackApi = stackApi;
        this.chatBot = chatBot;
        this.usersString = users.replace(',', ';');
        this.room = WebhookParameters.toRoom(room);
        this.site = site;
    }

    @Override
    public void run() {
        try {
            def result = stackApi.apiCall("users/" + usersString, site, "!23IYXA.sS8.otifg5Aq.2");
            List users = result.items
            if (users.size() != 2) {
                throw new UnsupportedOperationException("Cannot check diff for anything other than two users");
            }

            def max = users.stream().max(Comparator.comparingInt({it.reputation})).get();
            def min = users.stream().min(Comparator.comparingInt({it.reputation})).get();
            StringBuilder str = new StringBuilder();
            str.append(clearName(max.display_name) + " vs. " + clearName(min.display_name) + ": ");
            str.append((int)max.reputation - (int)min.reputation);
            str.append(" diff. ");
            diffStr(str, max, min, "Year", {it.reputation_change_year});
            diffStr(str, max, min, "Quarter", {it.reputation_change_quarter});
            diffStr(str, max, min, "Month", {it.reputation_change_month});
            diffStr(str, max, min, "Week", {it.reputation_change_week});
            diffStr(str, max, min, "Day", {it.reputation_change_day});
            chatBot.postSingle(room, str.toString());

        } catch (IOException e) {
            e.printStackTrace();
        }
    }

    public static String chatName(String displayName) {
        return clearName(displayName).replace(" ", "");
    }

    public static String clearName(String displayName) {
        while (displayName.contains("&#")) {
            String replacement = displayName.substring(displayName.indexOf("&#") + 2);
            try {
                replacement = replacement.substring(0, replacement.indexOf(';'));
                int ch = Integer.parseInt(replacement);
                displayName = displayName.replaceFirst("&#\\d+;", String.valueOf((char) ch));
            } catch (RuntimeException ex) {
                displayName = displayName.replaceFirst("&#", "");
            }
        }
        return displayName;
    }

    private void diffStr(StringBuilder str, max, min, String string, ToIntFunction<?> function) {
        str.append(string);
        str.append(": ");
        int maxValue = function.applyAsInt(max);
        int minValue = function.applyAsInt(min);
        int diff = maxValue - minValue;
        str.append(diff > 0 ? "+" : "");
        str.append(diff);
        str.append(". ");
    }

}

Answer 1

• The static chatName() method isn't used at all. Just get rid of it because it is only adding noise.

• Why is the static clearName() method public ? Make it either private and non static or place it in some other class which has the responsibility to do such cleaning to get the clear name.

  Or to ask different, what is wrong with using org.apache.commons.lang.StringEscapeUtils.unescapeHtml like you do in AnswerInvalidationCheck ?

• StringBuilder

  • IMO if you "know" or if you can narrow the resulting capacity of the StringBuilder you should use the overloaded constructor which takes the initial capacity as a parameter.

  • I like the fluent methods of the StringBuillder. IMO taking advantage of the fluent way makes it easier to read the code.

  • doing string concatenation to build a parameter for the append() method is somehow strange. Why don't you just use the append() method ?

• Some vertical spacing would be good for your code. It groups related code and makes it more readable.

  def max = users.stream().max(Comparator.comparingInt({it.reputation})).get();
  def min = users.stream().min(Comparator.comparingInt({it.reputation})).get();
  
  StringBuilder str = new StringBuilder();
  str.append(clearName(max.display_name) + " vs. " + clearName(min.display_name) + ": ");
  str.append((int)max.reputation - (int)min.reputation);
  str.append(" diff. ");
  
  diffStr(str, max, min, "Year", {it.reputation_change_year});
  diffStr(str, max, min, "Quarter", {it.reputation_change_quarter});
  diffStr(str, max, min, "Month", {it.reputation_change_month});  
  

Answer 2

Firstly I'll get rid of all the semicolons. They are not helping at all here.

Then I'll add two setters for usersString and room and just use the map constructor provided by groovy? Do you really want to read

this.a = a;
this.b = b;

You should just keep something that actually matters. Simple assignments? Let groovy do that for you.

And rather than this

users.stream().max(Comparator.comparingInt({it.reputation})).get();

why not just use this?

users.*reputation.max()

It is much more readable. Ditto for min.

Also why are you waiting till the API call is complete to throw exception in case there are more than two users? Just stop the whole thing when usersString has more than 2 user Ids when the class is created.

And instead of this

diffStr(str, max, min, "Year", {it.reputation_change_year});
diffStr(str, max, min, "Quarter", {it.reputation_change_quarter});
diffStr(str, max, min, "Month", {it.reputation_change_month});
diffStr(str, max, min, "Week", {it.reputation_change_week});
diffStr(str, max, min, "Day", {it.reputation_change_day});

It would probably be more readable to do this

['year', 'quarter', 'month', 'week', 'day'].each {
   diffStr(str, max, min, it)
}

And then change diffStr like this

private diffStr(StringBuilder str, max, min, String duration) {
    //Rest same
    int maxValue = max."reputation_change_$duration"
    int minValue = min."reputation_change_$duration"
    //Rest same
}

I removed ToIntFunction but you do realize that that it was Closure<Integer> actually there. No need to use Java 8 everywhere. And notice that if you don't return anything you don't need the void in case you specify the access

The part about using + for StringBuilder and using Apache commons has already been said. Just giving my upvote.

Lastly you are aware of @CompileStatic. Right? Use it everywhere unless you are using dynamic things in groovy.