Spartan programming Java example
Our input consists of the following, 49 lines private function, taken from a class which is designed to send by email automatic reports on the daily runs of a test suite to a number of subscribers. The CCC metric of this function is 1351, while the NOT metric is 452.
private static int sendAnEmail(String from, List<String> recepients, String subject, String body, List<File> attachements, boolean actuallySend)
throws Throwable {
List<MimeBodyPart> parts = new ArrayList<MimeBodyPart>();
// ''Build a sub-part for each attachment''
for (Iterator<File> i = attachements.iterator(); i.hasNext();) {
MimeBodyPart currPart = new MimeBodyPart();
FileDataSource fds = new FileDataSource(i.next());
currPart.setDataHandler(new DataHandler(fds));
currPart.setFileName(fds.getName());
parts.add(currPart);
}
// ''Build the message. add sub-parts if exist''
Session ses = Session.getInstance(new Properties());
Message msg = new MimeMessage(ses);
if (parts.size() == 0)
msg.setText(body); // ''A single part message''
else {
// ''A Multi part message''
MimeBodyPart textPart = new MimeBodyPart();
textPart.setText(body);
Multipart mp = new MimeMultipart();
mp.addBodyPart(textPart);
for (Iterator<MimeBodyPart> i = parts.iterator(); i.hasNext();)
mp.addBodyPart(i.next());
msg.setContent(mp);
}
msg.setFrom(new InternetAddress(from));
msg.setSubject(subject);
msg.setSentDate(new Date());
Transport transp = ses.getTransport("smtp");
transp.connect(smtpServer, port, userName, password);
List<InternetAddress> toAddresses = new ArrayList<InternetAddress>();
for (Iterator<String> i = recepients.iterator(); i.hasNext();) {
String curr = i.next();
InternetAddress[] temp = InternetAddress.parse(curr, false);
toAddresses.addAll(Arrays.asList(temp));
}
InternetAddress[] arr = new InternetAddress[toAddresses.size()];
int j = 0;
for (Iterator<InternetAddress> i = toAddresses.iterator(); i.hasNext(); ++j) {
InternetAddress curr = i.next();
arr[j] = curr;
}
msg.addRecipients(Message.RecipientType.TO, arr);
if (actuallySend)
transp.sendMessage(msg, arr);
transp.close();
return arr.length;
}We can start by replacing the first iteration
for (Iterator<File> i = attachements.iterator(); i.hasNext();) {
MimeBodyPart currPart = new MimeBodyPart();
FileDataSource fds = new FileDataSource(i.next());
currPart.setDataHandler(new DataHandler(fds));
currPart.setFileName(fds.getName());
parts.add(currPart);
}by the newer style, for each iteration
for (File f: attachements) {
MimeBodyPart currPart = new MimeBodyPart();
FileDataSource fds = new FileDataSource(f);
currPart.setDataHandler(new DataHandler(fds));
currPart.setFileName(fds.getName());
parts.add(currPart);
}removes a number of tokens and eliminates one reference to the iteration variable. (In the original version, variable i is referenced twice, in the second, variable f referenced only once.) This loop can be simplified further by using a generic name instead of currPart
for (File f: attachements) {
MimeBodyPart part = new MimeBodyPart();
FileDataSource fds = new FileDataSource(f);
part.setDataHandler(fds);
part.setFileName(fds.getName());
parts.add(part);
}Doing the same for the second loop, i.e., replacing
for (Iterator<MimeBodyPart> i = parts.iterator(); i.hasNext();)
mp.addBodyPart(i.next());by
for (MimeBodyPart p: parts)
mp.addBodyPart(p);also contributes to a reduction in the number of tokens.
for (Iterator<String> i = recepients.iterator(); i.hasNext();) {
String curr = i.next();
InternetAddress[] temp = InternetAddress.parse(curr, false);
toAddresses.addAll(Arrays.asList(temp));
}benefits even more by the change to the newer iteration syntax
for (String r: recepients)
toAddresses.addAll(Arrays.asList(InternetAddress.parse(r, false)));Two temporary variables are thus eliminated, just as the curly brackets around the loop body.
The fourth and last loop
int j = 0;
for (Iterator<InternetAddress> i = toAddresses.iterator(); i.hasNext(); ++j) {
InternetAddress curr = i.next();
arr[j] = curr;
}is similarly simplified by the transition to the new syntax.
int j = 0;
for (InternetAddress a: toAddresses)
arr[j++] = a;Looking at the entire function
private static intsendAnEmail(String from, List<String> recepients, String subject, String body, List<File> attachements, boolean actuallySend)
throws Throwable {
List<MimeBodyPart> parts = new ArrayList<MimeBodyPart>();
// ''Build a sub-part for each attachment''
for (File f: attachements) {
MimeBodyPart part = new MimeBodyPart();
part.setDataHandler(new DataHandler(new FileDataSource(f);));
part.setFileName(fds.getName());
parts.add(part);
}
// ''Build the message. add sub-parts if exist''
Session ses = Session.getInstance(new Properties());
Message msg = new MimeMessage(ses);
if (parts.size() == 0)
msg.setText(body); // ''A single part message''
else {
// ''A Multi part message''
MimeBodyPart textPart = new MimeBodyPart();
textPart.setText(body);
Multipart mp = new MimeMultipart();
mp.addBodyPart(textPart);
for (MimeBodyPart p: parts)
mp.addBodyPart(p);
msg.setContent(mp);
}
msg.setFrom(new InternetAddress(from));
msg.setSubject(subject);
msg.setSentDate(new Date());
Transport transp = ses.getTransport("smtp");
transp.connect(smtpServer, port, userName, password);
List<InternetAddress> toAddresses = new ArrayList<InternetAddress>();
for (String r: recepients)
toAddresses.addAll(Arrays.asList(InternetAddress.parse(r, false)));
InternetAddress[] arr = new InternetAddress[toAddresses.size()];
int j = 0;
for (InternetAddress a: toAddresses)
arr[j++] = a;
msg.addRecipients(Message.RecipientType.TO, arr);
if (actuallySend)
transp.sendMessage(msg, arr);
transp.close();
return arr.length;
}we see that the total number of lines is reduced by about 15% to 42.
This is still not good enough, but even the little simplification done so far clarifies the code's intent, which makes it possible to simplify it even further.
Examine for example this code portion.
List<InternetAddress> toAddresses = new ArrayList<InternetAddress>();
for (String r: recepients)
toAddresses.addAll(Arrays.asList(InternetAddress.parse(r, false)));
InternetAddress[] arr = new InternetAddress[toAddresses.size()];
int j = 0;
for (InternetAddress a: toAddresses)
arr[j++] = a;
msg.addRecipients(Message.RecipientType.TO, arr);The purpose of this code is to add to the prepared email message the list of recipients. But, this is done in a cumbersome fashion. First each element of the recepients list is parsed. The parsed result is converted into a list of addresses. All these addresses then collected into another list toAddresses, which is then converted to an array and placed into the message. By observing that the interface of List has a function which converts it into an array, we can greatly simplify this code fragment.
for (String r: recepients)
msg.addRecipients(Message.RecipientType.TO, InternetAddress.parse(r, false));eliminating one loop and three variables.
This transformation introduced a problem though: The function returns the total number of recipients. This count is lost now. The function however does not document its return value, so it is a good reason to suspect that this value is not used. Luckily, the function is private, so, we can quickly check where it is used. As it turns out, the function is invoked only in a single place, and this value is not used there. We therefore can convert the function's return type to void.
Another problem with this transformation is that the array arr, which was used to store the list of recipients, does not longer exist. This array is subsequently used to actually send the message.
if (actuallySend)
transp.sendMessage(msg, arr);Luckily, this array is not needed, and we can retrieve the list of recipients from the message itself. Thus, we can write instead
if (actuallySend)
transp.sendMessage(msg, msg.getAllRecipients());The above code is even more correct, since a message should be sent not only to the recipients in the To: list, but also to the Cc: and Bcc: lists.
The entire function now looks as follows
private static void sendAnEmail(String from, List<String> recepients, String subject, String body, List<File> attachements, boolean actuallySend)
throws Throwable {
List<MimeBodyPart> parts = new ArrayList<MimeBodyPart>();
// ''Build a sub-part for each attachment''
for (File f: attachements) {
MimeBodyPart part = new MimeBodyPart();
part.setDataHandler(new DataHandler(new FileDataSource(f);));
part.setFileName(fds.getName());
parts.add(part);
}
// ''Build the message. add sub-parts if exist''
Session ses = Session.getInstance(new Properties());
Message msg = new MimeMessage(ses);
if (parts.size() == 0)
msg.setText(body); // A single part message
else {
// A Multi part message
MimeBodyPart textPart = new MimeBodyPart();
textPart.setText(body);
Multipart mp = new MimeMultipart();
mp.addBodyPart(textPart);
for (MimeBodyPart p: parts)
mp.addBodyPart(p);
msg.setContent(mp);
}
msg.setFrom(new InternetAddress(from));
msg.setSubject(subject);
msg.setSentDate(new Date());
Transport transp = ses.getTransport("smtp");
transp.connect(smtpServer, port, userName, password);
for (String r: recepients)
msg.addRecipients(Message.RecipientType.TO, InternetAddress.parse(r, false));
if (actuallySend)
transp.sendMessage(msg, msg.getAllRecipients());
transp.close();
}The line count is reduced by another 15% to 36.
Now that the function is simplified this much, it is time to reorder its statements so that the simpler code is executed first. Clearly, the creation of the message to sent, and the addition of the mundane, scalar fields is simpler than doing the more complex stuff of adding MIME attachments.
Here is what we obtain:
private static void sendAnEmail(String from, List<String> recepients, String subject, String body, List<File> attachements, boolean actuallySend)
throws Throwable {
Session ses = Session.getInstance(new Properties());
Message msg = new MimeMessage(ses);
msg.setFrom(new InternetAddress(from));
msg.setSubject(subject);
msg.setSentDate(new Date());
List<MimeBodyPart> parts = new ArrayList<MimeBodyPart>();
// ''Build a sub-part for each attachment''
for (File f: attachements) {
MimeBodyPart currPart = new MimeBodyPart();
FileDataSource fds = new FileDataSource(f);
currPart.setDataHandler(new DataHandler(fds));
currPart.setFileName(fds.getName());
parts.add(currPart);
}
// ''Build the message. add sub-parts if exist''
if (parts.size() == 0)
msg.setText(body); // A single part message
else {
// A Multi part message
MimeBodyPart textPart = new MimeBodyPart();
textPart.setText(body);
Multipart mp = new MimeMultipart();
mp.addBodyPart(textPart);
for (MimeBodyPart p: parts)
mp.addBodyPart(p);
msg.setContent(mp);
}
Transport transp = ses.getTransport("smtp");
transp.connect(smtpServer, port, userName, password);
for (String r: recepients)
msg.addRecipients(Message.RecipientType.TO, InternetAddress.parse(r, false));
if (actuallySend)
transp.sendMessage(msg, msg.getAllRecipients());
transp.close();
}Examining the first few lines of the function
Session ses = Session.getInstance(new Properties());
Message msg = new MimeMessage(ses);
msg.setFrom(new InternetAddress(from));
msg.setSubject(subject);
msg.setSentDate(new Date());we find that variables ses and msg are generic. No particular assumptions are made about the session, nor about the message object. Rewriting the code with generic names technique gives
Session s = Session.getInstance(new Properties()
Message m = new MimeMessage(s);
m.setFrom(new InternetAddress(from));
m.setSubject(subject);
m.setSentDate(new Date());Examining the subsequent lines,
// ''Build a sub-part for each attachment''
for (File f: attachements) {
MimeBodyPart currPart = new MimeBodyPart();
FileDataSource fds = new FileDataSource(f);
currPart.setDataHandler(new DataHandler(fds));
currPart.setFileName(fds.getName());
parts.add(currPart);
}
// ''Add sub-parts if exist''
if (parts.size() == 0)
m.setText(body); // A single part message
else {
// ''A Multi part message''
MimeBodyPart textPart = new MimeBodyPart();
textPart.setText(body);
Multipart mp = new MimeMultipart();
mp.addBodyPart(textPart);
for (MimeBodyPart p: parts)
mp.addBodyPart(p);
m.setContent(mp);
}we see that these iterate over the attachments twice. First collecting these into the list parts and then iterating over this list to insert the attachments into the message. A moment's thought shows that this list can be eliminated.
if (attachements.size() == 0) // ''A single part message''
m.setText(body);
else { // ''A Multi part message''
MimeBodyPart textPart = new MimeBodyPart();
textPart.setText(body);
Multipart mp = new MimeMultipart();
mp.addBodyPart(textPart);
for (File f: attachements) {
MimeBodyPart part = new MimeBodyPart();
FileDataSource fds = new FileDataSource(f);
part.setDataHandler(new DataHandler(fds));
part.setFileName(fds.getName());
mp.addBodyPart(part);
}
m.setContent(mp);
}Our function is now reduced further to 31 lines.
private static void sendAnEmail(String from, List<String> recepients, String subject, String body, List<File> attachements, boolean actuallySend)
throws Throwable {
Session s = Session.getInstance(new Properties());
Message m = new MimeMessage(s);
m.setFrom(new InternetAddress(from));
m.setSubject(subject);
m.setSentDate(new Date());
if (attachements.size() == 0) // ''A single part message''
m.setText(body);
else { // ''A Multi part message''
MimeBodyPart textPart = new MimeBodyPart();
textPart.setText(body);
Multipart mp = new MimeMultipart();
mp.addBodyPart(textPart);
for (File f: attachements) {
MimeBodyPart part = new MimeBodyPart();
FileDataSource fds = new FileDataSource(f);
part.setDataHandler(new DataHandler(fds));
part.setFileName(fds.getName());
mp.addBodyPart(part);
}
m.setContent(mp);
}
Transport transp = s.getTransport("smtp");
transp.connect(smtpServer, port, userName, password);
for (String r: recepients)
m.addRecipients(Message.RecipientType.TO, InternetAddress.parse(r, false));
if (actuallySend)
transp.sendMessage(m, m.getAllRecipients());
transp.close();
}Now that about 35% of the code is eliminated, while functionality is the same, we can see even clearer the intent, and more importantly, a design flaw in this function. The function control flow of the function is coupled with the caller. Specifically, the function defines a boolean argument which tells it whether it should actually send the electronic mail message it worked so hard on making. If this boolean flag is false, then the message is silently discarded.
It is not clear why the function was designed this way. Since it is private we can quickly inspect its invocation sites. Doing so, reveals that it is only called at one code location, and that this flag is false. We are therefore safe to remove the actuallySend flag parameter. This is also a good time to use a generic, terse name for the variable representing the transport.
The result is a 29 lines long function,
private static void sendAnEmail(String from, List<String> recepients, String subject, String body, List<File> attachements) throws Throwable {
Session s = Session.getInstance(new Properties());
Message m = new MimeMessage(s);
m.setFrom(new InternetAddress(from));
m.setSubject(subject);
m.setSentDate(new Date());
if (attachements.size() == 0) // ''A single part message''
m.setText(body);
else { // ''A Multi part message''
MimeBodyPart textPart = new MimeBodyPart();
textPart.setText(body);
Multipart mp = new MimeMultipart();
mp.addBodyPart(textPart);
for (File f: attachements) {
MimeBodyPart part = new MimeBodyPart();
FileDataSource fds = new FileDataSource(f);
part.setDataHandler(new DataHandler(fds));
part.setFileName(fds.getName());
mp.addBodyPart(part);
}
m.setContent(mp);
}
Transport t = s.getTransport("smtp");
t.connect(smtpServer, port, userName, password);
for (String r: recepients)
m.addRecipients(Message.RecipientType.TO, InternetAddress.parse(r, false));
t.sendMessage(m, m.getAllRecipients());
t.close();
}Finally, time has come to apply a number of cleanup operations:
- Correct the spelling error in the argument named attachements.
- Correct the spelling error in the argument named recepients; better yet, rename this parameter to the shorter and arguably clearer name to
- Correct the grammar of comments
- Move the loop iterating over the recipients further up, so as to obey the shorter first rule.
- Simplify the function name
- Reorder the statements so the order of setting the fields of the mail message matches the the order of arguments
private static void sendEmail(String from, List<String> to, String subject, String body, List<File> attachments) throws Throwable {
Session s = Session.getInstance(new Properties());
Message m = new MimeMessage(s);
m.setSentDate(new Date());
m.setFrom(new InternetAddress(from));
for (String t: to)
m.addRecipients(Message.RecipientType.TO, InternetAddress.parse(t, false));
m.setSubject(subject);
if (attachments.size() == 0) // ''No attachments to send''
m.setText(body);
else { // ''Generate a multi-part message''
MimeBodyPart textPart = new MimeBodyPart();
textPart.setText(body);
Multipart mp = new MimeMultipart();
mp.addBodyPart(textPart);
for (File f: attachments) {
MimeBodyPart part = new MimeBodyPart();
FileDataSource fds = new FileDataSource(f);
part.setDataHandler(new DataHandler(fds));
part.setFileName(fds.getName());
mp.addBodyPart(part);
}
m.setContent(mp);
}
Transport t = s.getTransport("smtp");
t.connect(smtpServer, port, userName, password);
t.sendMessage(m, m.getAllRecipients());
t.close();
}The CCC of this final version is 862, which is about 46% of the CCC of the input. Similarly, the NOT is 260, which is 57% of the original version.
The resulting routine may still look to many as long and complex. On top of the five parameters, function sendEmail defines many more variables:
Three variables in its outer most scope: Session s;, Message m;, and Transport t; (which, admittedly, is visible only in the last four lines of the code).
Another variable is defined in the tight loop examining the recipients list
for (String t: to)
m.addRecipients(Message.RecipientType.TO, InternetAddress.parse(t, false));Two variables in the outer scope of the else branch in charge of dealing with the attachments:
MimeBodyPart textPart = ...
Multipart mp = ...Another loop variable for iterating over the attachments, in
for (File f : attachments)
Two more variables in the scope of this inner loop:
MimeBodyPart part = ...
FileDataSource fds = ...In total, the function has 9 variables. All variables are defined in the smallest possible scope. Spartan programming dictates also that the variability of these should be minimized. Indeed, in this case, we can, and should, declare all of them as final
Using terse names for a total of 14 variables, especially with a function spanning almost thirty lines of code may be cryptic to some readers. The purpose of variable s, declared at the beginning of the routine, may not be so obvious to someone reading its end. The fix is not in changing to verbose and descriptive names, but rather in eliciting the obvious submodules.
Spartan programming suggests using short routines. There is a limit to what can be achieved using the above described, semi-automatic techniques. To make this routine clearer and more concise, we will now decompose it into cohesive, loosely coupled routines. Submodules.
The fact that variable Transport t = is only used in the last four executable lines, suggests that this code portion can be factored out. This is easy to do with Eclipse: Mark these lines, and then choose Extract Method... in the "Refactor" menu, or simply hit Alt+Shift+M. Eclipse will then prompt you for a name for the method you are about to create. A good descriptive name is not hard to come up with. Here is the resulting code
private static void sendEmail(final String from, final List<String> to, final String subject, final String body, final List<File> attachments) throws Throwable {
final Session s = Session.getInstance(new Properties());
final Message m = new MimeMessage(s);
m.setSentDate(new Date());
m.setFrom(new InternetAddress(from));
for (final String t : to)
m.addRecipients(Message.RecipientType.TO, InternetAddress.parse(t, false));
m.setSubject(subject);
if (attachments.size() == 0) // ''No attachments to send''
m.setText(body);
else { // ''Generate a multi-part message''
final MimeBodyPart textPart = new MimeBodyPart();
textPart.setText(body);
final Multipart mp = new MimeMultipart();
mp.addBodyPart(textPart);
for (final File f : attachments) {
final MimeBodyPart part = new MimeBodyPart();
final FileDataSource fds = new FileDataSource(f);
part.setDataHandler(new DataHandler(fds));
part.setFileName(fds.getName());
mp.addBodyPart(part);
}
m.setContent(mp);
}
sendMessage(m, s);
}
private static void sendMessage(final Message m, final Session s) {
final Transport t = s.getTransport("smtp");
t.connect(smtpServer, port, userName, password);
t.sendMessage(m, m.getAllRecipients());
t.close();
}The result is a bit longer (also because we just added the final modifiers), but the gains in clarity are worth it. Routine sendMessage is particularly nice: only four lines, and very clear semantics: given an email message and a session, send the message over this session.
Further application of the same Extract Method... refactoring tool to the main conditional used in the code, yiels another sub-routine
private static void setContent(final Message m, final String body, final List<File> attachments) throws MessagingException {
if (attachments.size() == 0) // ''No attachments to send''
m.setText(body);
else { // ''Generate a multi-part message''
final MimeBodyPart textPart = new MimeBodyPart();
textPart.setText(body);
final Multipart mp = new MimeMultipart();
mp.addBodyPart(textPart);
for (final File f : attachments) {
final MimeBodyPart part = new MimeBodyPart();
final FileDataSource fds = new FileDataSource(f);
part.setDataHandler(new DataHandler(fds));
part.setFileName(fds.getName());
mp.addBodyPart(part);
}
m.setContent(mp);
}
}while the initial routine is as simple as
private static void sendEmail(final String from, final List<String> to, final String subject, final String body,
final List<File> attachments) throws Throwable {
final Session s = Session.getInstance(new Properties());
final Message m = new MimeMessage(s);
m.setSentDate(new Date());
m.setFrom(new InternetAddress(from));
for (final String t : to)
m.addRecipients(Message.RecipientType.TO, InternetAddress.parse(t, false));
m.setSubject(subject);
setContent(m, body, attachments);
sendMessage(m, s);
}Examining the routine we have just elicited, we notice that it nothing more than a complex conditional. We can simplify the routine further by making an early return from the true branch.
private static void setContent(final String body, final List<File> attachments, final Message m) throws MessagingException {
if (attachments.size() == 0) { // ''No attachments to send''
m.setText(body);
return;
}
// ''Generate a multi-part message''
final MimeBodyPart textPart = new MimeBodyPart();
textPart.setText(body);
final Multipart mp = new MimeMultipart();
mp.addBodyPart(textPart);
for (final File f : attachments) {
final MimeBodyPart part = new MimeBodyPart();
final FileDataSource fds = new FileDataSource(f);
part.setDataHandler(new DataHandler(fds));
part.setFileName(fds.getName());
mp.addBodyPart(part);
}
m.setContent(mp);
}Simplifying the code this much, makes it easy to identify two other sub-tasks: the generation of message headers, and the making of a file attachment into a body part, which can be included in the MIME. The resulting code should be pretty self explanatory
private static void sendEmail(final String from, final List<String> to, final String subject, final String body,
final List<File> attachments) throws Throwable {
final Session s = Session.getInstance(new Properties());
final Message m = new MimeMessage(s);
setHeaders(m, from, to, subject);
setContent(m, body, attachments);
sendMessage(m, s);
}
private static void setHeaders(final Message m, final String from, final List<String> to, final String subject) {
m.setSentDate(new Date());
m.setFrom(new InternetAddress(from));
for (final String t : to)
m.addRecipients(Message.RecipientType.TO, InternetAddress.parse(t, false));
m.setSubject(subject);
}
private static void setContent(final Message m, final String body, final List<File> attachments) throws MessagingException {
if (attachments.size() == 0) { // ''No attachments to send''
m.setText(body);
return;
}
// ''Generate a multi-part message''
final MimeBodyPart textPart = new MimeBodyPart();
textPart.setText(body);
final Multipart mp = new MimeMultipart();
mp.addBodyPart(textPart);
for (final File f : attachments)
mp.addBodyPart(makeBodyPart(f));
m.setContent(mp);
}
private static MimeBodyPart makeBodyPart(final File f) throws MessagingException {
final MimeBodyPart $ = new MimeBodyPart();
final FileDataSource fds = new FileDataSource(f);
$.setDataHandler(new DataHandler(fds));
$.setFileName(fds.getName());
return $;
}
private static void sendMessage(final Message m, final Session s) {
final Transport t = s.getTransport("smtp");
t.connect(smtpServer, port, userName, password);
t.sendMessage(m, m.getAllRecipients());
t.close();
}