4
\$\begingroup\$

I have written an EmailSession class to connect and get (unread) mail messages. It is all working, but I would like to see your opinions on any enhancements or improvements, as I understood there are lots of bad examples on the Internet and I copy-pasted most of it together.

One issue I already noticed:

  • If there is no internet connection it throws a java.net.UnknownHostException instead of handling the issue gracefully.

import java.util.*;
import java.util.logging.*;
import javax.mail.*;

public class EmailSession {

    private Session session;
    private Store store;
    private Folder inbox;
    private int messageCount;
    private int unreadMessageCount;
    private List<Message> unreadMessages;

    public EmailSession() {
        Properties props = new Properties();
        props.setProperty("mail.store.protocol", MyProps.getProperty("mail.store.protocol"));
        try {
            session = Session.getInstance(props, null);
            store = session.getStore();
            store.connect(MyProps.getProperty("mail.host"), MyProps.getProperty("mail.user"), MyProps.getProperty("mail.password"));
            inbox = store.getFolder(MyProps.getProperty("mail.foldername"));
            inbox.open(Folder.READ_ONLY);
            messageCount = inbox.getMessageCount();
            List<Message> messages;
            if (messageCount > 100) {
                Logger.getLogger(EmailSession.class.getName()).log(Level.WARNING, 
                        "More than 100 messages");
                messages = Arrays.asList(inbox.getMessages(1, 100));
            } else {
                messages = Arrays.asList(inbox.getMessages());
            }
            unreadMessages = new ArrayList<>();
            unreadMessageCount = 0;
            for (Message message : messages) {
                boolean isUnreadMessage = true;
                for (Flags.Flag flag : message.getFlags().getSystemFlags()) {
                    if (flag == Flags.Flag.SEEN) {
                        isUnreadMessage = false;
                        break;
                    }
                }
                if (isUnreadMessage) {
                    unreadMessages.add(message);
                    unreadMessageCount++;
                }
            }
            inbox.close(true);
        } catch (NoSuchProviderException ex) {
            Logger.getLogger(EmailSession.class.getName()).log(Level.SEVERE, null, ex);
        } catch (MessagingException ex) {
            Logger.getLogger(EmailSession.class.getName()).log(Level.SEVERE, null, ex);
        }
    }

    public Folder getInbox() {
        return inbox;
    }

    public int getMessageCount() {
        return messageCount;
    }

    public int getUnreadMessageCount() {
        return unreadMessageCount;
    }

    public List<Message> getUnreadMessages() {
        return unreadMessages;
    }

    public boolean close() {
        try {
            store.close();
            return true;
        } catch (MessagingException ex) {
            Logger.getLogger(EmailSession.class.getName()).log(Level.SEVERE, null, ex);

            return false;
        }
    }

}
\$\endgroup\$
5
\$\begingroup\$

Pointless member variables

session could be a local variable in the constructor. You use it once to get store and you don't reference it again.

unreadMessageCount is pointless, because you could replace it with unreadMessages.size() instead.

In the constructor, you assign inbox, but you also close it. I'm not really familiar with how javax.mail works, but usually, after you close something it tends to become unusable. If that's the case, then the inbox member is pointless.

Pointless member variables make your class design confusing, hard to read, and potentially buggy. Try to be minimalistic, and eliminate anything from your class that it doesn't really need.

Extract logger

The logging in your code can be simplified if you extract the Logger to a constant:

private static final Logger LOGGER = Logger.getLogger(EmailSession.class.getName());

// ...
LOGGER.warning("More than 100 messages");
// ...
LOGGER.log(Level.SEVERE, null, ex);

Extract methods

The constructor is too long. It's hard to read large chunks of code. You could extract some of the functionality there to private methods to make it more readable:

private List<Message> getMessages(Folder inbox, int messageCount) throws MessagingException {
    if (messageCount > 100) {
        LOGGER.warning("More than 100 messages");
        return Arrays.asList(inbox.getMessages(1, 100));
    }
    return Arrays.asList(inbox.getMessages());
}

private List<Message> getUnreadMessages(List<Message> messages) throws MessagingException {
    List<Message> unreadMessages = new ArrayList<>();
    for (Message message : messages) {
        boolean isUnreadMessage = true;
        for (Flags.Flag flag : message.getFlags().getSystemFlags()) {
            if (flag == Flags.Flag.SEEN) {
                isUnreadMessage = false;
                break;
            }
        }
        if (isUnreadMessage) {
            unreadMessages.add(message);
        }
    }
    return unreadMessages;
}

With these helper methods and the other suggestions above, you could simplify the constructor:

public EmailSession() {
    Properties props = new Properties();
    props.setProperty("mail.store.protocol", MyProps.getProperty("mail.store.protocol"));
    Session session = Session.getInstance(props, null);
    try {
        store = session.getStore();
        store.connect(MyProps.getProperty("mail.host"), MyProps.getProperty("mail.user"), MyProps.getProperty("mail.password"));
        inbox = store.getFolder(MyProps.getProperty("mail.foldername"));
        inbox.open(Folder.READ_ONLY);
        messageCount = inbox.getMessageCount();
        List<Message> messages = getMessages(inbox, messageCount);
        inbox.close(true);
        unreadMessages = getUnreadMessages(messages);
    } catch (NoSuchProviderException ex) {
        LOGGER.log(Level.SEVERE, null, ex);
    } catch (MessagingException ex) {
        LOGGER.log(Level.SEVERE, null, ex);
    }
}

I also moved inbox.close higher to close it immediately when you don't need it anymore.

Avoid wildcard imports

Wildcard imports like import java.util.* are considered bad practice. Try to avoid it, it's better to clean up all of these:

import java.util.*;
import java.util.logging.*;
import javax.mail.*;
\$\endgroup\$
4
\$\begingroup\$

I'd expect a WhateverSession to implement AutoCloseable, to be usable in a "try-with-resources" statement.

For that you need: Java 7, a void method close() to override the AutoCloseable

Your class then would look minimally different:

public class EmailSession implements AutoCloseable() {

    //all the other stuff.

    @Override
    public void close() {
        store.close();
        //Just bubble exceptions to caller
    }
}

Additionally your EmailSession just handles a single user. Assuming one user has multiple e-mail accounts, you can't create a EmailSession nicely. Instead you'd have to expose an different constructor where you pass in all required data to build it.

This means you could then use your EmailSession in a try-with-resources statement as follows:

try (EmailSession mailsession = new EmailSession()) {
    //do stuff with mailsession
} /* Exception handling here */
\$\endgroup\$
3
\$\begingroup\$

I'm the only one who thinks the constructor is too big? Generally, i avoid to let the constructor do many things (just.. create the object and leave!) and prefer static factory and methods to it.

Field unreadMessageCount is already 0 by default when you create the object, you don't need unreadMessageCount = 0;.

What about one line for save messages?

List<Message> messages = Arrays.asList(inbox.getMessages(1, messageCount > 99 ? 100 : messageCount));

private Session session;

Is a field, but used only in constructor. It could be a local variable, if Session.getInstance(props, null) is an heavy operation you should mark it static (and do some other things if it's used in multithread applications).

store is used only in constructor, you need it to read data? You do everything in the constructor why you can't close it in the constructor too? After all, when the constructor ends everything is readed from class fields and it's not used anymore.

\$\endgroup\$
  • \$\begingroup\$ Great feedback! \$\endgroup\$ – Martijn Burger Jul 31 '14 at 16:18
  • \$\begingroup\$ "I'm the only one who thinks the constructor is too big?" Oh no you are not! That is untestable. \$\endgroup\$ – Kolargol00 Aug 1 '14 at 13:56

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.