7
\$\begingroup\$

I recently got this question in an exam, this is how I implemented, it is working fine. However, I would like to know if the code can be improved, as I think it for sure needs to be improved. The question was:

In the past, Twitter had a 140 characters limitation per tweet. If you wanted to enter longer tweets, you had to split it into several. We want to create a function that receives a String, which is the content of the tweet, that returns all the tweets with numbered prefixes (a List). It does not matter if the words are splitted. Something like this: (Tweet 1/5)tweetcontent.... (Tweet 2/5)tweetcontent...

public class Exercise {

static int MAX_CHARACTERS_OF_MESSAGE_IN_TWEET = 129;

public List<String> tweets(String message) {
    List<String> tweetsToReturn = new ArrayList<>();
    //calculate numberOfTweets needed to tweet
    int numberOfTweets = message.length() / MAX_CHARACTERS_OF_MESSAGE_IN_TWEET;
    //check if we need to add another tweet for the latest characters of the message
    int restOfTWeet = message.length() % MAX_CHARACTERS_OF_MESSAGE_IN_TWEET;
    if (restOfTWeet != 0) {
        numberOfTweets++;
    }
    //if there is only one tweet
    if (numberOfTweets == 1) {
        tweetsToReturn.add(message);
    } else { //if there is more than one tweet
        for (int i = 0; i < numberOfTweets - 1; i++) {
            tweetsToReturn.add("(Tweet " + (i + 1) + "/" + numberOfTweets + ")" + message.substring(i * MAX_CHARACTERS_OF_MESSAGE_IN_TWEET, (i+1) * MAX_CHARACTERS_OF_MESSAGE_IN_TWEET));
        }
        //create the latest tweet
        if (restOfTWeet != 0) {
            tweetsToReturn.add("(Tweet " + numberOfTweets + "/" + numberOfTweets + ")" + message.substring((numberOfTweets - 1) * MAX_CHARACTERS_OF_MESSAGE_IN_TWEET));
        }
    }
    return tweetsToReturn;
}}

Bonus point: Do it now but without the words being splitted. (This one I did not manage to do it).

Thank you!!!

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Do you have a constraint that the number of tweets will be fewer than 10? If not, the problem becomes a bit more challenging to use up every available character. \$\endgroup\$ Aug 14 at 14:45
  • \$\begingroup\$ From what I remember, the number of tweets were not going to be very big. However, I would like to know the different solutions depending on the number of tweets. Thanks! \$\endgroup\$
    – isanma
    Aug 14 at 16:20
7
\$\begingroup\$

Overall, your code does the job correctly, and does it in a way that is not unnecessarily complicated. Good job. The rest of my comments are just how you can make the code a little easier to read, because the simpler it is to read, the easier it will be to tackle the bonus problem.

To start, when you share code with others you should run the auto-format in your IDE, and you should include the necessary imports.

MAX_CHARACTERS_OF_MESSAGE_IN_TWEET is too long, I would prefer MAX_TWEET_LENGTH and it should be marked final. The value should reflect the knowledge of the domain, i.e. the 140 character length. Then you can show how you got to 129:

    static final int MAX_TWEET_LENGTH = 140;
    static final int PREFIX_LENGTH = 11;
    static final int CHARS_PER_TWEET = MAX_TWEET_LENGTH - PREFIX_LENGTH;
public List<String> tweets(String message) {

This should marked static, because it doesn't use any instance variables. This makes it available to its consumers without having to make a new Exercise(). And it makes the code slightly easier to read for people reading the code, because they know they don't need to consider any variables defined elsewhere, except for other static variables (which are usually final constants that are self-descriptive).

Variables should be declared close to where they are first used, so List<String> tweetsToReturn should come later. Variables that you are intending to return are often take the name of the method itself, and therefore the toReturn part is implied and can be left out.

        // if there is only one tweet

This comment is unnecessary - it just says what the code already says. Most of the comments fall into this category. Once you are "fluent" in "reading" code, this just becomes obvious. It's as though you've written something to a bilingual person, but said the same thing twice, once in each language.

        // if there is more than one tweet

Same with this comment, but I will add that it is sometimes nicer to avoid else blocks altogether in cases where the if block can simply return out of the function. You can use Arrays.asList to simply return an array if all the values are already known. In this case, there will only be one value, and we already know it (message).

If you calculate a value, check it once, and don't use it again, you don't need to create a variable name for it (restofTWeet), just put it "inline". Here you are using the value again later, but I get the feeling we can make changes so that won't actually be necessary, so I would still inline it for now.

To build the list of tweets, you are using a for loop, which works in cases where we know how many times we are going to loop, and we indeed do know. But it takes more mental effort to verify that we have calculated the number of loops properly, compared to if we just set up a simple "loop until done" using while, so I would use while and that turned out to make the bonus problem a little easier too.

As for building the tweets the way you have with + on Strings, that's fine performance-wise, and it's not so bad in terms of readability, but you should be aware of the other ways of writing the same thing, which you may sometimes prefer if it makes it even easier to read. Specifically, also be aware of how to use String.format and StringBuilder, which have their own pros and cons. I used both in the examples below.

Here's the version after the after the above changes, and still solving the original problem:

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

public class Exercise {

    static final int MAX_TWEET_LENGTH = 140;
    static final int PREFIX_LENGTH = 11;
    static final int CHARS_PER_TWEET = MAX_TWEET_LENGTH - PREFIX_LENGTH;

    public static List<String> tweets(String message) {
        if (message.length() <= MAX_TWEET_LENGTH) {
            return Arrays.asList(message);
        }
        
        int tweetCount = message.length() / CHARS_PER_TWEET;
        if (message.length() % CHARS_PER_TWEET != 0) {
            tweetCount++;
        }

        List<String> tweets = new ArrayList<>();
        int cursor = 0;
        int currentCount = 1;
        while (cursor < message.length()) {
            final int contentLength = Math.min(message.length() - cursor,
                    CHARS_PER_TWEET);
            final String content = message.substring(cursor, cursor + contentLength);
            tweets.add(String.format("(Tweet %d/%d)%s", currentCount, tweetCount, content));
            cursor += contentLength;
            currentCount++;
        }
        return tweets;
    }

}

Then for the bonus points, we can get an array of the words using String.split, and instead of working out how many characters we are going to add per loop, we do an inner loop, adding words from the list until the next word won't fit or there are no more words:

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

public class Exercise {

    static final int MAX_TWEET_LENGTH = 140;
    static final int PREFIX_LENGTH = 11;
    static final int CHARS_PER_TWEET = MAX_TWEET_LENGTH - PREFIX_LENGTH;

    public static List<String> tweets(String message) {
        if (message.length() <= MAX_TWEET_LENGTH) {
            return Arrays.asList(message);
        }

        int tweetCount = message.length() / CHARS_PER_TWEET;
        if (message.length() % CHARS_PER_TWEET != 0) {
            tweetCount++;
        }

        String[] words = message.split("\\s");
        
        List<String> tweets = new ArrayList<>();
        int cursor = 0;
        int currentCount = 1;

        while (cursor < words.length) {
            StringBuilder content = new StringBuilder(CHARS_PER_TWEET);
            while (cursor < words.length
                    && content.length() + words[cursor].length() + 1 < CHARS_PER_TWEET) {
                content.append(" ");
                content.append(words[cursor]);
                cursor++;
            }
            tweets.add(
                    String.format("(Tweet %d/%d)%s", currentCount, tweetCount, content));
            currentCount++;
        }
        return tweets;
    }

}

And a simple test method:

    public static void main(String[] args) {
        List<String> tweets = tweets(
                "It was the best of times, it was the worst of times, it was the age of " + 
                "wisdom, it was the age of foolishness, it was the epoch of belief, it was " + 
                "the epoch of incredulity, it was the season of Light, it was the season of " + 
                "Darkness, it was the spring of hope, it was the winter of despair, we had " + 
                "everything before us, we had nothing before us, we were all going direct " + 
                "to Heaven, we were all going direct the other way—in short, the period " + 
                "was so far like the present period, that some of its noisiest authorities " + 
                "insisted on its being received, for good or for evil, in the superlative " + 
                "degree of comparison only.");
        for (String tweet : tweets) {
            System.out.println(tweet);
        }
    }

Output:

(Tweet 1/5) It was the best of times, it was the worst of times, it was the age of wisdom, it was the age of foolishness, it was the epoch
(Tweet 2/5) of belief, it was the epoch of incredulity, it was the season of Light, it was the season of Darkness, it was the spring of
(Tweet 3/5) hope, it was the winter of despair, we had everything before us, we had nothing before us, we were all going direct to Heaven,
(Tweet 4/5) we were all going direct the other way—in short, the period was so far like the present period, that some of its noisiest
(Tweet 5/5) authorities insisted on its being received, for good or for evil, in the superlative degree of comparison only.

(We have been assuming the number of tweets will be less than 10, and less than 100, etc. The problem becomes quite a bit trickier if that's not the case, because the number of characters you can use per tweet depends on the number of characters in the prefix, which depends on the number of total tweets, which depends on the number of characters used per tweet. A circular dependency. So the solution might involve attempting to build with one prefix length and when the prefix length changes, start all over again with the new prefix length, and repeat until you have gone through all the input.

Actually, my code for the bonus problem has a bug related to the same issue, because I'm still using the CHARS_PER_TWEET to calculate the number of tweets, but the tweets are sometimes shorter than that based on the word splitting. To fix that, you would need to calculate the content of all the tweets without the prefix, then go back and prepend the prefix containing the correct count. Then similarly, if the prefix length changes based on the tweet count, do it all over again.)

\$\endgroup\$
1
  • \$\begingroup\$ Wow! Thanks a lot for this answer Jeremy! It is super complete! I will try it now and will see if I am able to fix this bug that you comment on the bonus problem. You rock!!! \$\endgroup\$
    – isanma
    Aug 15 at 18:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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