Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Using:

ToastUtils.LONG.show(context, R.string.greeting);

Code:

public class ToastUtils {
    public static ToastUtils LONG = new ToastUtils(Toast.LENGTH_LONG);
    public static ToastUtils SHORT = new ToastUtils(Toast.LENGTH_SHORT);

    private final int duration;

    private ToastUtils(int duration) {
        this.duration = duration;
    }

    public Toast makeText(Context context, String text) {
        return Toast.makeText(context, text, duration);
    }

    public void show(Context context, String text) {
        makeText(context, text).show();
    }

    public Toast makeText(Context context, int resId) {
        return Toast.makeText(context, resId, duration);
    }

    public void show(Context context, int resId) {
        makeText(context, resId).show();
    }

}

In my opinion, there are some disadvantages of using Toast class directly:

  • In Toast class I often miss the call to show()
  • Parameter length has type int. So a compiler couldn't detect invalid argument value

What do you think? Maybe this class is useless?

Can you suggest any improvements?

share|improve this question

2 Answers 2

up vote 3 down vote accepted

There is one important thing missing on your LONG and SHORT fields.

What if I do ToastUtils.LONG = null; ?

That's right, I would break it.

You must specify them as final

public class ToastUtils {
    public static final ToastUtils LONG = new ToastUtils(Toast.LENGTH_LONG);
    public static final ToastUtils SHORT = new ToastUtils(Toast.LENGTH_SHORT);

However, you might just as well change this one and make it an enum

public enum ToastUtils {
    LONG(Toast.LENGTH_LONG), SHORT(Toast.LENGTH_SHORT);

    ... // the rest of your code can look the same

But I have to agree with @Kao that your class does not seem to be very useful.

It is better to learn the existing API than creating your own API to avoid having to learn the existing API (Yes, I just made that up. It makes sense though, doesn't it?)

share|improve this answer

IMO this class isn't very useful (at least in a current shape).

In Toast class I often miss the call to show()

Well, it's not a bug, it's a feature. There are many situations when you only want to prepare your Toast using makeText() and show it later with show(). Moreover, you don't fix this "bug" with your class - there is still a makeText() method which does exactly what Toast used to do. And you still can forget to call show().

Parameter length has type int. So a compiler couldn't detect invalid argument value

I wouldn't say that this is a bug, too. And you're fixing it in a bad way, again:

   public static ToastUtils LONG = new ToastUtils(Toast.LENGTH_LONG);
   public static ToastUtils SHORT = new ToastUtils(Toast.LENGTH_SHORT);

So there can only be single instances of LONG and SHORT at the time. This will make many things complicated.


To sum up, this is usage example of your class:

ToastUtils.LONG.show(context, R.string.greeting);

And this is the same without using your class:

Toast.makeText(context, R.string.greeting, Toast.LENGTH_LONG).show();

It's not a big improvement, is it?

share|improve this answer
    
That there can only be one instance of LONG and SHORT ToastUtils should not matter as they are simply factories for creating the real toast. –  Simon André Forsberg Aug 29 at 9:49
    
Yes, you're right. But this still will make things complicated. –  Kao Aug 29 at 9:55
    
Yes, overall the class is quite useless –  Simon André Forsberg Aug 29 at 10:16

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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