current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 2 down vote favorite
1

I have written a common class for finding display sizes in pre-honeycomb and for latest versions of android. Could anyone review my code and suggest me possible modifications in this code. I would love to do follow beautiful coding standards. 

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 (Build.VERSION.SDK_INT < Build.VERSION_CODES.HONEYCOMB_MR2) {
            return mDisplay.getHeight();
        } else {
            Point size = new Point();
            mDisplay.getSize(size);
            return size.y;
        }
    }

    @TargetApi(Build.VERSION_CODES.HONEYCOMB_MR2)
    public int getDisplayWidth() {
        if (Build.VERSION.SDK_INT < Build.VERSION_CODES.HONEYCOMB_MR2) {
            return mDisplay.getWidth();
        } else {
            Point size = new Point();
            mDisplay.getSize(size);
            return size.x;
        }
    }
}
share|improve this question

1 Answer 1

up vote 3 down vote

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).

share|improve this answer

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.