Как можно улучшить абстрактный класс?

Рейтинг: 2Ответов: 2Опубликовано: 08.09.2014

Есть класс для работы с звонками на андроиде:

import android.content.BroadcastReceiver;
import android.content.Context;
import android.content.Intent;
import android.telephony.TelephonyManager;

import java.util.Date;

public abstract class PhonecallReceiver extends BroadcastReceiver {

//The receiver will be recreated whenever android feels like it.  We need a static variable to remember data between instantiations

private static int lastState = TelephonyManager.CALL_STATE_IDLE;
private static Date callStartTime;
private static boolean isIncoming;
private static String savedNumber;  //because the passed incoming is only valid in ringing

@Override
public void onReceive(Context context, Intent intent) {
    //We listen to two intents.  The new outgoing call only tells us of an outgoing call.  We use it to get the number.
    if (intent.getAction().equals("android.intent.action.NEW_OUTGOING_CALL")) {
        savedNumber = intent.getExtras().getString("android.intent.extra.PHONE_NUMBER");
    } else {
        String stateStr = intent.getExtras().getString(TelephonyManager.EXTRA_STATE);
        String number = intent.getExtras().getString(TelephonyManager.EXTRA_INCOMING_NUMBER);
        int state = 0;
        if (stateStr.equals(TelephonyManager.EXTRA_STATE_IDLE)) {
            state = TelephonyManager.CALL_STATE_IDLE;
        } else if (stateStr.equals(TelephonyManager.EXTRA_STATE_OFFHOOK)) {
            state = TelephonyManager.CALL_STATE_OFFHOOK;
        } else if (stateStr.equals(TelephonyManager.EXTRA_STATE_RINGING)) {
            state = TelephonyManager.CALL_STATE_RINGING;
        }

        onCallStateChanged(context, state, number);
    }
}

//Derived classes should override these to respond to specific events of interest
protected void onIncomingCallStarted(Context ctx, String number, Date start) {
}

protected void onOutgoingCallStarted(Context ctx, String number, Date start) {
}

protected void onIncomingCallEnded(Context ctx, String number, Date start, Date end) {
}

protected void onOutgoingCallEnded(Context ctx, String number, Date start, Date end) {
}

protected void onMissedCall(Context ctx, String number, Date start) {
}

//Deals with actual events

//Incoming call-  goes from IDLE to RINGING when it rings, to OFFHOOK when it's answered, to IDLE when its hung up
//Outgoing call-  goes from IDLE to OFFHOOK when it dials out, to IDLE when hung up
public void onCallStateChanged(Context context, int state, String number) {
    if (lastState == state) {
        //No change, debounce extras
        return;
    }
    switch (state) {
        case TelephonyManager.CALL_STATE_RINGING:
            isIncoming = true;
            callStartTime = new Date();
            savedNumber = number;
            onIncomingCallStarted(context, number, callStartTime);
            break;
        case TelephonyManager.CALL_STATE_OFFHOOK:
            //Transition of ringing->offhook are pickups of incoming calls.  Nothing done on them
            if (lastState != TelephonyManager.CALL_STATE_RINGING) {
                isIncoming = false;
                callStartTime = new Date();
                onOutgoingCallStarted(context, savedNumber, callStartTime);
            }
            break;
        case TelephonyManager.CALL_STATE_IDLE:
            //Went to idle-  this is the end of a call.  What type depends on previous state(s)
            if (lastState == TelephonyManager.CALL_STATE_RINGING) {
                //Ring but no pickup-  a miss
                onMissedCall(context, savedNumber, callStartTime);
            } else if (isIncoming) {
                onIncomingCallEnded(context, savedNumber, callStartTime, new Date());
            } else {
                onOutgoingCallEnded(context, savedNumber, callStartTime, new Date());
            }
            break;
    }
    lastState = state;
}

}

Вроде все отлично, теперь можно сделать так:

public class CallReceiver extends PhonecallReceiver {

@Override
protected void onIncomingCallStarted(String number, Date start) {
}

@Override
protected void onOutgoingCallStarted(String number, Date start) {
}

@Override
protected void onIncomingCallEnded(String number, Date start, Date end) {
}

@Override
protected void onOutgoingCallEnded(String number, Date start, Date end) {
}

@Override
protected void onMissedCall(String number, Date start) {
}

}

И все прекрасно, в любой момент времени понятно, что происходит. Но техлид сказал, что это какое-то мракобесие, что много лишнего. Что я делаю не так?

Ответы

▲ 1Принят

Исключите статик переменные:

Если Вы создадите 2 и более экземпляра класса, то логика работы Вашего метода, использующего эти переменные, будет нарушена.

Упростите работу с ресивером:

Например,

String action = intent.getAction();
String extra_state = intent.getExtras().getString(TelephonyManager.EXTRA_STATE);
if(action.equals("...NEW_OUTGOING_CALL")){
    internalNewOutgoingCall(intent);
}
else if(extra_state.equals("RINGING")){
    internalRinging(intent);
}

Ещё бы избавиться от if'ов, но это уже так... если ресурсы девать некуда.

По Вашему коду непонятно, зачем и что вы делаете. Старайтесь делать изолированные куски.

▲ 1
  1. Эм-м..., а почему ваш класс, собтвесно говоря, декларируется как абстрактный? Это тоже может вызвать спазмы мракобесия (ну, кроме статических переменных, естественно).
  2. Зачем onCallStateChanged() объявлен как публичный? Это некий интимный, вроде, метод, и хорошо бы объявить его приватным.
  3. Неплохо бы создать протектед конструктор (раз уж класс абстрактный), где провести инициализацию переменных.
  4. Опять же, раз класс абстрактный, знаменитые методы типа protected void onIncomingCallEnded объявить бы публичными абстрактными (все равно они пустые, так пусть будут абстрактные).