6
\$\begingroup\$

I use this EmailTools helper class inside an Android app to send data as an email attachment, by calling send(context, deviceName, recordedContent), where the parameters are:

  • context: an instance of Context, in order to:

    • create Intent.ACTION_SEND for launching an existing email app to send emails
    • get elements from string.xml to format the subject
    • get the app's version to include in the message body
  • deviceName: a String, the name of the device where the data is coming from.
  • recordedContent: a String, the recorded data, to send as attachment.

Is there a better way? How would you improve this?

public abstract class EmailTools {

    private static final String TAG = EmailTools.class.getSimpleName();

    private static final String MESSAGE_TYPE = "message/rfc822";

    public static void send(Context context, String deviceName, String recordedContent) {
        String subject = String.format(context.getString(R.string.fmt_subject_recorded_data), deviceName);
        String message = String.format(context.getString(R.string.fmt_recorded_from), deviceName);
        message += getPackageInfo(context);

        Intent intent = new Intent(Intent.ACTION_SEND);
        intent.setType(MESSAGE_TYPE);
        intent.putExtra(Intent.EXTRA_SUBJECT, subject);
        intent.putExtra(Intent.EXTRA_TEXT, message);
        addAttachmentToIntent(context, deviceName, recordedContent, intent);
        launchEmailApp(context, intent);
    }

    private static String getPackageInfo(Context context) {
        String packageName = context.getPackageName();
        PackageManager manager = context.getPackageManager();
        try {
            PackageInfo info;
            if (manager != null) {
                info = manager.getPackageInfo(packageName, 0);
                if (info != null) {
                    return String.format("\n\n--\n[App: %s Version: %d/%s]",
                            packageName, info.versionCode, info.versionName);
                }
            }
        } catch (PackageManager.NameNotFoundException e) {
            Log.e(TAG, "Could not get package info", e);
        }
        return "";
    }

    private static void addAttachmentToIntent(Context context, String deviceName, String recordedContent, Intent intent) {
        SimpleDateFormat dateFormat = new SimpleDateFormat("_yyyyMMdd_HHmm");
        String filename = deviceName + dateFormat.format(new Date()) + ".dat";
        try {
            FileOutputStream ostream = context.openFileOutput(filename, Context.MODE_WORLD_READABLE);
            ostream.write(recordedContent.getBytes());
            ostream.close();
            File attachment = context.getFileStreamPath(filename);
            intent.putExtra(Intent.EXTRA_STREAM, Uri.fromFile(attachment));
        } catch (IOException e) {
            Log.e(TAG, "could not create temp file for attachment :(", e);
        }
    }

    private static void launchEmailApp(Context context, Intent intent) {
        try {
            context.startActivity(Intent.createChooser(intent, context.getString(R.string.email_client_chooser)));
        } catch (ActivityNotFoundException ex) {
            Toast.makeText(context, context.getString(R.string.no_email_client), Toast.LENGTH_SHORT).show();
        }
    }
}
\$\endgroup\$
2
\$\begingroup\$

Disregarding the lack of comments and documentation :) No, seriously, add javadoc to every method, also private, and the class.

Your method addAttachmentToIntent silently eats an exception. It is okay to swallow the exception, but a clear improvement would be to return true or false depending on the success of adding the attachment. Hence, return false if an exception occurs, and true otherwise.

Second, you should see if Android doesn't provide the functionality of creating temporary file names for you, for if not, you should try to find a next filename, should the one you try be taken. Simply add "-1", "-2", ..., "-n" until you find a filename which isn't used.

\$\endgroup\$
  • \$\begingroup\$ I usually favour self-documenting code over the javadoc comments. Is there anything not clear in the code @janos worte? I don't think so. If you really want the user to know that the attachment failed I'd propagate the exception (maybe wrapping it in a new, specific, one). Returning boolean does not force the client to check them, which could lead to issues. \$\endgroup\$ – mariosangiorgio Sep 23 '14 at 21:03
  • \$\begingroup\$ Self-documenting code is no good if you don't have access to the source code. In this case, I can imagine only exposing the API, and then it would be good for the user to know that he silently opens the email app without the file attached if there are two attempts in the same minute or if something else goes wrong. Propagating exception isn't something that magically makes a user handle this, the send function should test if addAttachment succeeds or fails, and notify the user correspondingly. \$\endgroup\$ – Pål GD Sep 24 '14 at 7:46
  • 2
    \$\begingroup\$ How would javadoc of private methods help if you don't have access to the code? In that case they are just an implementation detail. The user should know nothing about them \$\endgroup\$ – mariosangiorgio Sep 24 '14 at 11:33
  • 1
    \$\begingroup\$ Sure, but the send method should specify this! And that would be the public interface. \$\endgroup\$ – Pål GD Sep 24 '14 at 15:29
  • \$\begingroup\$ Yes, I agree on that \$\endgroup\$ – mariosangiorgio Sep 24 '14 at 15:37
3
\$\begingroup\$

I'd change a few things to make your code cleaner.

Your send method does three different things: it extract the message from the context, it creates the intent and it finally launches the email app. What about introducing a Message class, a Message decodeMessage(Context context, String deviceName), and an Intent createIntent(Message message) methods?

getPackageInfo is definitely to arrow-shaped. I'd refactor it to separate the logic of creation of the package info string String formatPackageInfo(PackageInfo packageInfo) and the code to retrieve the PackageInfo instance.

private PackageInfo getPackageInfo(){
    String packageName = context.getPackageName();
    PackageManager manager = context.getPackageManager();
    if (manager == null)
        return null;
    try
    {
        return manager.getPackageInfo(packageName, 0);
    }
    catch (PackageManager.NameNotFoundException e)
    {
        Log.e(TAG, "Could not get package info", e);
    }
    return null;
}

private static String formatPackageInfo(PackageInfo packageInfo) {
        if(packageInfo == null)
            return "";
        return String.format("\n\n--\n[App: %s Version: %d/%s]",
                        packageName, info.versionCode, info.versionName);
}

In addAttachmentToIntent I'd probably move the declaration of dateFormat and of filename in the try block. You're not going to use them outside that scope so it could be a good idea to give them the smallest scope possible.

\$\endgroup\$
  • \$\begingroup\$ I wonder if returning null in getPackageInfo is a good idea. It's introducing null checked in the caller code (and a risk of NullPointerException. \$\endgroup\$ – Marc-Andre Sep 24 '14 at 13:42
  • \$\begingroup\$ I think it's acceptable as they are both private methods. I won't expose it to a client but my rule of thumb is that nulls are not excessively bad in the internal implementation of a class. Generally speaking I totally agree that nulls should be avoided as much as possible \$\endgroup\$ – mariosangiorgio Sep 24 '14 at 13:45
1
\$\begingroup\$

Both other answers helped me in some way:

  • As @Pal GD pointed out, swallowing the exception if attaching fails, stinks
  • As @mariosangiorgio pointed out, the getPackageInfo method stinks

I also found some other improvement ideas:

  • The formatting of the package info string "\n\n--\n[App: %s Version: %d/%s]" was strange. In fact the \n\n-- is for visual separation, and lumping it together with the version formatting is not pretty. It would be better to isolate the app version formatting to its own method, separate from other elements in the message.

  • A related thing, if I could not get the PackageInfo, I did not add an app version string. But I can do better: I could at least include the package name, which can be useful additional info.

  • A much better option than swallowing the Exception when trying to attach the content is to include the content in the message body itself. It's better than nothing.

  • Last but not least, this utility class should not be an abstract class. The idea of an abstract class is to be extended by other classes, which conflicts with the concept of a utility class. It's better to make this a regular class and make its constructor private.

Putting it all together:

public class EmailTools {

    private static final String TAG = EmailTools.class.getSimpleName();

    private static final String MESSAGE_TYPE = "message/rfc822";

    private static final String HORIZONTAL_RULE = "\n\n---\n\n";

    private EmailTools() {
        // prevent creating utility class
    }

    public static void sendDeviceRecording(Context context, String defaultEmail, String deviceName, String recordedContent) {
        String subject = String.format(context.getString(R.string.fmt_subject_recorded_data), deviceName);
        String messageHeader = String.format(context.getString(R.string.fmt_recorded_from), deviceName);

        StringBuilder builder = new StringBuilder();
        builder.append(messageHeader).append(HORIZONTAL_RULE);

        Intent intent = new Intent(Intent.ACTION_SEND);
        intent.setType(MESSAGE_TYPE);
        intent.putExtra(Intent.EXTRA_EMAIL, new String[]{defaultEmail});
        intent.putExtra(Intent.EXTRA_SUBJECT, subject);
        if (!addAttachmentToIntent(context, deviceName, recordedContent, intent)) {
            builder.append(recordedContent).append(HORIZONTAL_RULE);
        }
        builder.append(getPackageInfoString(context));

        intent.putExtra(Intent.EXTRA_TEXT, builder.toString());
        launchEmailApp(context, intent);
    }

    private static String getPackageInfoString(Context context) {
        PackageInfo info = getPackageInfo(context);
        return String.format("[App: %s Version: %d/%s]",
                context.getPackageName(), info.versionCode, info.versionName);
    }

    private static PackageInfo getPackageInfo(Context context) {
        String packageName = context.getPackageName();
        PackageManager manager = context.getPackageManager();
        if (manager != null) {
            try {
                return manager.getPackageInfo(packageName, 0);
            } catch (PackageManager.NameNotFoundException e) {
                Log.e(TAG, "Could not get package info", e);
            }
        }
        return new PackageInfo();
    }

    private static boolean addAttachmentToIntent(Context context, String deviceName, String recordedContent, Intent intent) {
        SimpleDateFormat dateFormat = new SimpleDateFormat("_yyyyMMdd_HHmm");
        String filename = deviceName + dateFormat.format(new Date()) + ".dat";
        try {
            FileOutputStream ostream = context.openFileOutput(filename, Context.MODE_WORLD_READABLE);
            ostream.write(recordedContent.getBytes());
            ostream.close();
            File attachment = context.getFileStreamPath(filename);
            intent.putExtra(Intent.EXTRA_STREAM, Uri.fromFile(attachment));
            return true;
        } catch (IOException e) {
            Log.e(TAG, "could not create temp file for attachment :(", e);
            return false;
        }
    }

    private static void launchEmailApp(Context context, Intent intent) {
        try {
            context.startActivity(Intent.createChooser(intent, context.getString(R.string.email_client_chooser)));
        } catch (ActivityNotFoundException ex) {
            Toast.makeText(context, context.getString(R.string.no_email_client), Toast.LENGTH_SHORT).show();
        }
    }
}
\$\endgroup\$

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.