5
\$\begingroup\$

I read effective Java by Joshua Bloch. And it said use enums over int constant. So I was thinking instead of using hard coded strings I could also use enums. The forgotpassword.txt and confirmation.txt are plain text resource files.

When the class is loaded, I get the contents of the text file and store it inside of a map, since enums are only loaded once.

When I want to create an email template I pass key value map to the EmailTemplate.[TemplateName].create () method and it produces an email text. Anytime I want to create a new template I just have to go to one place and add it.

The only down-side I could think of is if you have hundreds of different email templates. Do you think this is a good design?

public enum EmailTemplate
{
    FORGOT_PASSWORD("forgotpassword.txt"),
    CONFIRM_EMAIL("confirmation.txt");

    private final String mFile;

    public String getFile()
    {
        return mFile;
    }

    EmailTemplate(String file)
    {
        mFile = file;
    }

    public String create(Map<String, String> valuesMap)
    {
        //lazy load file content into template
        if (!lookup.containsKey(this))
        {
            lookup.put(this, FileUtil.getFileContents((this.getFile())));
        }
        StrSubstitutor sub = new StrSubstitutor(valuesMap);
        return sub.replace(lookup.get(this));
    }

    private static final Map<EmailTemplate, String> lookup = new HashMap<EmailTemplate, String>();
}

public class EmailGenerator
{
    public static void sendConfirmationEmail(String emailAddress,
            String confirmCode)
    {
        Map<String, String> valuesMap = new HashMap<String, String>();
        valuesMap.put("link", confirmCode);

        String text = EmailTemplate.CONFIRM_EMAIL.create(valuesMap);
        System.out.println(text);
    }

    public static void sendContactUs(String subject, String email, String message, String name)
    {
        Map<String, String> valuesMap = new HashMap<String, String>();
        valuesMap.put("name", name);
        valuesMap.put("text", message);
        valuesMap.put("email", email);

        String text = EmailTemplate.CONTACT_US.create(valuesMap);
        System.out.println(text);
    }
}
\$\endgroup\$
0

3 Answers 3

5
\$\begingroup\$
  1. Don't use plain words as replacement keys since you might accidentally erase a real "name". For example, in your template, use "Hello {{name}}," and you add "{{" and "}}" to the word you want to replace before calling replace. It's easier to do that than to read each template carefully to make sure the keywords do not appear anywhere where they should not be changed.

  2. Hide EmailTemplate inside EmailGenerator and make it private. EmailTemplate is a "dirty" class that should not be exposed publicly: the method create(Map<String, String> valuesMap) must not be accessible since you need exactly the right arguments in the map.

  3. You don't even really need EmailTemplate. If you don't want to reread the files all the time, you can use a "lazy" map, Map<String, String> textFilesCache (file name -> file text). When you fetch a file, you define a method that checks if textFilesCache(filename) is null and, if it is, it loads the file and adds it to textFilesCache. It's even more efficient than the enum since the template files you never use will never be read. An enum seems overkill here and it make the code less readable.
    (If you want to have some fun, you can even take a look at WeakHashMap. It's overkill here since you will only have a few files, but if you had very many and very large files, the JVM can drop some values of a WeakHashMap to free memory.)

\$\endgroup\$
0
1
\$\begingroup\$
  1. I would avoid lazy loading of template files. If one of them is missing you'll get a runtime error and probably a support call. I guess it's not a performance bottleneck to load these files at startup. If you load (and check) them at startup and your app is running you have fewer possible causes for support calls. (Another advantage is that you might loose a contact message if you load the template at runtime.)

  2. It's confusing the link-confirmationCode difference here:

    Map<String, String> valuesMap = new HashMap<String, String>();
    valuesMap.put("link", confirmCode);
    

    Does it contain only a code (like a hashcode) or a complete link? I would call both the same. (The same is true for the text-message pair.)

  3. confirmCode probably kills autocomplete. If you type confirma and press Ctrl+Space for autocomplete in Eclipse it founds nothing which is rather disturbing. I usually try to avoid abbreviations like this. They are also not too readable.

  4. The same is true for mFile. What is m here? I would rename it to templateFilename (both the field and constructor parameter).

  5. Java has EnumMap. I would consider that instead of a HashMap.

\$\endgroup\$
1
  • \$\begingroup\$ The confirm code is just a unique string for email confirmation, the actual link is inside of the text file. I do agree renaming the file to templateFile does make it read better. \$\endgroup\$
    – James
    Jul 4, 2014 at 18:02
0
\$\begingroup\$

Your code looks overly complicated. If the number of templates is low, why don't you initialize them manually, and if it's high why don't you just put them in a collection? Are you sure you really need lots of singletons here?

\$\endgroup\$
1
  • \$\begingroup\$ @mkalkowlv the above code is just a sample. I have about 20 different email templates. If you look closely, i am storing them within a collection. The reason you don't to initialize them manually is because do i really want to keep reading from a file if 20+ users forgotten their password? Or if 100+ users are using the contact form? \$\endgroup\$
    – James
    Jul 4, 2014 at 13:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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