4
\$\begingroup\$

I have written a common class for finding display sizes in pre-honeycomb and for the latest versions of Android. Could anyone review my code and suggest possible modifications for this code?

import android.annotation.TargetApi;
import android.content.Context;
import android.graphics.Point;
import android.os.Build;
import android.view.Display;
import android.view.WindowManager;

public class DisplayManager {

    private Display mDisplay;
    private WindowManager mWindowManager;

    public DisplayManager(Context mContext) {
        mWindowManager = (WindowManager) mContext.getSystemService(Context.WINDOW_SERVICE);
        mDisplay = mWindowManager.getDefaultDisplay();
    }

    @TargetApi(Build.VERSION_CODES.HONEYCOMB_MR2)
    public int getDisplayHeight() {
        if (Integer.valueOf(android.os.Build.VERSION.SDK_INT) < 13) {
            return mDisplay.getHeight();
        } else {
            Point size = new Point();
            mDisplay.getSize(size);
            return size.y;
        }
    }

    @TargetApi(Build.VERSION_CODES.HONEYCOMB_MR2)
    public int getDisplayWidth() {
        if (Integer.valueOf(android.os.Build.VERSION.SDK_INT) < 13) {
            return mDisplay.getWidth();
        } else {
            Point size = new Point();
            mDisplay.getSize(size);
            return size.x;
        }
    }
}
\$\endgroup\$
4
\$\begingroup\$

First of all, this line:

if (Integer.valueOf(android.os.Build.VERSION.SDK_INT) < 13) {

Should be replaced with this:

if (Build.VERSION.SDK_INT < Build.VERSION_CODES.HONEYCOMB_MR2) {

Because a) There's no need to use Integer.valueOf for something that is already an int. b) Build.VERSION_CODES.HONEYCOMB_MR2 is a constant (or as we call it in Java, public static final) with the value 13. Avoid using "magic numbers" in your code like that, instead refer to existing constants or create new constants when necessary.

Secondly, I really doubt that the size of your screen will change while your object exists. Which means that this code need only to be run once and can therefore be placed in the constructor:

Point size = new Point();
mDisplay.getSize(size);

Now to the last point I want to make. Your mWindowManager variable is only used within the constructor. I see no need at all for that to be a field in your class. Change that to a local variable instead.

It is also a good practice to make your class fields final when possible (which is both possible for the mDisplay and also your newly created Point size field). The final keyword makes sure that you get a compiler error if you try to change the value of the variable after it has been initialized. Note that mDisplay.getSize(size) doesn't change the variable itself, it changes the values within the variable, i.e. the object size remains the same.

Besides this, I have to say that your code looks excellent. You have good variable names. I don't even see any indentation problems (except those that StackExchange create, but they're not your fault).

\$\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.