Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Which one of these two would you prefer writing in your code?

This:

tweet = tweet.replaceAll("@\\w+|#\\w+|\\bRT\\b", "");

tweet = tweet.replaceAll("\n", " ");

tweet = tweet.replaceAll("[^\\p{L}\\p{N} ]+", " ");

tweet = tweet.replaceAll(" +", " ").trim();

Or this:

tweet = tweet.replaceAll("@\\w+|#\\w+|\\bRT\\b", "")
                .replaceAll("\n", " ")
                .replaceAll("[^\\p{L}\\p{N} ]+", " ")
                .replaceAll(" +", " ")
                .trim();

Which one looks cleaner (in my opinion the second one but it obstructs you from commenting every line if you wanted to) and which one should perform better? (my guess is that there is no difference since regex doesn't create a new string everytime and it does everything internally if I am correct)


Additional Information:

Using one .replaceAll() is not possible because if the content that I want to be removed is not removed in this order then the output will be wrong.

Here is a short compilable example:

public class StringTest {

    public static void main(String args[]) {
        String text = "RT @AshStewart09: Vote for Lady Gaga for \"Best Fans\""
                + " at iHeart Awards\n"
                + "\n"
                + "RT!!\n"
                + "\n"
                + "My vote for #FanArmy goes to #LittleMonsters #iHeartAwards\n"
                + "for ART!"
                + "htt… è, é, ê, ë asdf324 ah_";
        System.out.println("Before: " + text + "\n");

        text = text.replaceAll("@\\w+|#\\w+|\\bRT\\b", "")
                .replaceAll("\n", " ")
                .replaceAll("[^\\p{L}\\p{N} ]+", " ")
                .replaceAll(" +", " ").trim();

        System.out.println("After: " + text + "\n");
    }
}

I have tried merging some of the .replaceAll() but the output was always wrong if I did the operations in any other order than this. In the end I want to be left with just the words of the tweet and nothing else. Bare in mind that this is the first time I'm using regex so I am far from a pro at it so if there actually is a way to merge them then do tell.


This is the closest I'm currently at merging the replaceAll():

text = text
    .replaceAll("@\\w+|#\\w+|\\bRT\\b|\n|[^\\p{L}\\p{N} ]+", " ")
    .replaceAll(" +", " ")
    .trim();

I basically made everything a space and then removed all the extra spaces. Is that better than before? Is it even possible to merge the last replaceAll as well?

share|improve this question
1  
FWIW, (and I'm sure you know this) even in the second example you can comment each line by putting the comment at the end of the code with a // comment. I find those pretty readable and helpful if the comment is short enough to fit on the same line. Doesn't always work out. –  matt Apr 17 at 18:10
    
@matt yes I knew that, I just personally do not really like commenting on the side unless it's a closing bracket. –  Dimitris K Apr 17 at 18:13

4 Answers 4

up vote 15 down vote accepted

I agree mostly with @Vogel612's answer, the second version is more concise and there is no need to litter variables all over the place as is done in the first version.

Another point is that code should be self-documenting, which @Vogel612 also suggests, but else you can still add a comment behind the code lines.

My review

I wanted to talk about a more interesting point, as this question seems to be a subproblem of a bigger question of yours, which involves creating a high throughput program that does something with tweets.
Perhaps this is a premature optimization given that I do not know how well your program performs, but your program may very well hit a threshold limit because of the way you handle regexes.

You should consider what a String.replaceAll does:

public String replaceAll(String regex, String replacement) {
    return Pattern.compile(regex).matcher(this).replaceAll(replacement);
}

For every provided regex it creates a pattern and matches and replaces the occurences with the replacement.
It is also important to look into Pattern.replaceAll now:

public String replaceAll(String replacement) {
    reset();
    boolean result = find();
    if (result) {
        StringBuffer sb = new StringBuffer();
        do {
            appendReplacement(sb, replacement);
            result = find();
        } while (result);
        appendTail(sb);
        return sb.toString();
    }
    return text.toString();
}

Here you can see, that because of four regex replaces, you create four times a StringBuffer, which is synchronized, and a String object, which can count up if you are processing tons of tweets.

There are two things you can do:

  1. Create a single regex that captures what you want to do, thus needing only one replaceAll call.
  2. Go really into micro-optimizing and get rid of the regexes in a whole, operate on a char[] and only store the resulting character array.
share|improve this answer
    
The problem with having one replaceAll is that the processing must be done in that order. I will provide an example when I get back home to show you what I mean exactly. –  Dimitris K Apr 17 at 13:33
    
I updated my code to reflect why I can't use a single replaceAll. –  Dimitris K Apr 17 at 15:26
    
I updated it again with a somewhat merged replaceAll but I have a feeling that the | operator will still create separate regex's. –  Dimitris K Apr 17 at 15:47
1  
@SilliconTouch Like skiwi wrote, it may be a premature optimization to combine all of the regexes. If it's hard for you to do, AND you don't expect to process very many strings at any one time, you can get away with performance testing the unoptimized code at some point before production and leaving it unoptimized if you're happy with the way it works. –  Kevin Apr 18 at 0:41
1  
In the end I decided to do a bit of a mix of all the answers here (since every one of them was pretty good) but mainly I liked this answer for the good explanation of replaceAll() which made me understand how to go about this performance wise. I decided to keep things readable for now and not do anything fancy thus going with the second of my original options and in the future if I decide optimization is needed I will make a proper class with pre-compiled patterns. –  Dimitris K Apr 18 at 15:09

This is probably a matter of preference. But performance-wise it's definitely faster to do the second one. Especially as there should be a new instance of String created for every assignment you perform.This means, you create 3 Instances of String, that are just "useless garbage".

@skiwi and @200_success put me right on my incorrect belief, that the second approach were faster. In fact, every Call to .replaceAll() creates a new Instance of String, whether you concat the next call or not.
This makes it a relatively expensive operation.

Concerning the possibility of commenting your code:
your code / regex should be "selfdocumenting" enough to not need comments.

If that is not the case, I suggest you extract at least your regexes to named constants:

tweet = tweet.replaceAll(HASHTAG_REGEX, "")
    .replaceAll("\n", " ")
    .replaceAll(DO_NOT_KNOW_WHAT_REGEX, " ")
    .replaceAll(MULTIPLE_WHITESPACE_REGEX, " ")
    .trim();

Thanks to @Amon for valuable critique on my indentation-style

share|improve this answer
2  
I like the CONSTANTS. I'm not so sure that the second one performs significantly faster, though. It would still create intermediate strings. –  200_success Apr 17 at 12:11
4  
A disadvantage of your suggested indentation style is that when the tweet variable is renamed, the indentation of the other lines will be off again. It is therefore sensible to put a newline in fron of the first method invocation as well: tweet = tweet<NL><TAB>.replaceAll(…)<NL><TAB>.replaceAll(…) .... –  amon Apr 17 at 12:13
    
I agree with having the code documented well enough and I will do that. By the way the third regex removes all special characters. @amon I thought having the first function not indented was the norm... –  Dimitris K Apr 17 at 15:29
    
I tried merging them but I have a feeling that the performance might be the same and the code worse. Will for example the | improve performance or is it the same as before? (see bottom code for latest changes). –  Dimitris K Apr 17 at 15:45

As shown by skiwi each "replaceAll" call compiles a regexp pattern, then apply it to a String and finally performs the replacement.

If you pre-compile your patterns you can have your code running a bit faster (I've tried on my machine, got it 15% faster - not such a big deal but if performances are important for you, you can consider it)

You can do it in this way:

private static class ReplacementPattern {
    ReplacementPattern(Pattern pattern, String replace) {
        this.pattern = pattern;
        this.replace = replace;
    }
    Pattern pattern;
    String replace;
}

Your patterns must be initialized somewhere in your code, just once:

private List<ReplacementPattern> patterns = new ArrayList<ReplacementPattern>(4);

    patterns.add(new ReplacementPattern(Pattern.compile("@\\w+|#\\w+|\\bRT\\b"), ""));
    patterns.add(new ReplacementPattern(Pattern.compile("\n"), " "));
    patterns.add(new ReplacementPattern(Pattern.compile("[^\\p{L}\\p{N} ]+"), " "));
    patterns.add(new ReplacementPattern(Pattern.compile(" +"), " "));


private String cleanTweet(String text) {
    for (ReplacementPattern rp : patterns) {
        text = rp.pattern.matcher(text).replaceAll(rp.replace);
    }
    return text.trim();
}
share|improve this answer
3  
+1 - pre-compiling the regexes is the most obvious performance gain to me. Though if you're going to go to the trouble to put it in its own class, you ought to have an applyTo or replace method in that class rather than accessing the internals. It would aid readability. –  Chris Hayes Apr 18 at 5:21

My recommendation for this would be to first focus on readability and then only concern yourself with performance if you know it's an issue.

I find regex hard to read myself, especially when you're just glancing at it, so my recommendation for that is to move the regexes into their own methods that have better names.

Usage:

string before = "bla bla blah"; 
string after = TweetFormatter.format(before);

System.out.println("Before: " + before + "\n");
System.out.println("After: " + after + "\n");

Implementation:

public class TweetFormatter
{
    public static string format(string tweet)
    {
        tweet = removeHashTags(tweet);
        tweet = someOtherMethod(tweet);
        tweet = convertNewlinesToSpaces(tweet);
        tweet = normalizeWhitespace(tweet);

        return tweet.trim();
    }

    private static string removeHashTags(string tweet)
    {
        return tweet.replaceAll("@\\w+|#\\w+|\\bRT\\b", "");
    }

    private static string someOtherMethod(string tweet)
    {
        return tweet.replaceAll("[^\\p{L}\\p{N} ]+", " ");
    }   

    private static string convertNewlinesToSpaces(string tweet)
    {
        return tweet.replaceAll("\n", " ");
    }

    private string string normalizeWhitespace(string tweet)
    {
        return tweet.replaceAll("\\s+", " ");
    }
}

The advantage to doing this is that if you determine that this performs too slowly, you can write unit tests to ensure the TweetFormatter behaves properly, and then refactor its innards to make it more performant without worrying about duplicated code.

share|improve this answer
2  
Normally I like methods, but convertNewlinesToSpaces seems a bit much. I mean, the content of the method tweet.replaceAll("\n", " ") is a more descriptive than the name. –  Navin Apr 19 at 0:35

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.