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.)