Комментарии 23
И читали его программисты ученые и хвалили Ваньку за старания евоные.
Вам в прошлом посте вроде советовали Rx, гляньте на него, ну правда.
А теперь вопросы.
1. Разметка активити, хардкод текста
2. Класс Utils (который больше похож на «Constants»), часть переменных final, часть нет. Создается впечатление что вы не видите разницу.
3. Метод verify(). Во первых, повторяется 2 раза, принцип DRY считай нарушен. Насколько я понимаю метод дергается при каждой новой попытке, и каждый раз создает новый экземпляр SharedPreferences, как-то это не правильно. Да и вообще хранить данные переменные в SharedPreferences не совсем верно.
4. Я даже боюсь думать, что будет если запустить все это, и начать крутить телефон)
Почитайте про Rx и MVP, вы сможете упростить и укоротить свой код в разы)
2. Спасибо, поправил. Невнимательность. Изначально там была пара функций, удаленных за ненадобностью, вот название и осталось.
3. Была мысль вынести его в Utils, сделав статическим, а SharedPreferences брать из службы.
Примерно так:
public class Utils {
public static final String EXTRA_KEY_OUT = "EXTRA_OUT";
public static final String ACTION_MYINTENTSERVICE = "ru.timgor.alerttest.RESPONSE";
public static final String TAG = "AlertTest";
public static final String SUCCESS = "success";
public static final String AUTOMATIC = "chbAutomatic";
public static boolean verify(SharedPreferences settings){
boolean success = settings.getBoolean(Utils.SUCCESS, false);
return success;
}
}
и соответственно, на примере AutomaticService:
import android.content.Intent;
import android.content.SharedPreferences;
import android.util.Log;
import com.google.android.gms.gcm.GcmNetworkManager;
import com.google.android.gms.gcm.GcmTaskService;
import com.google.android.gms.gcm.TaskParams;
public class AutomaticService extends GcmTaskService {
@Override
public int onRunTask(TaskParams taskParams) {
SharedPreferences settings = getSharedPreferences(Utils.TAG, MODE_PRIVATE);
Log.d(Utils.TAG, "Автоматический запуск. Начало работы");
Log.d(Utils.TAG, "Переданное число: "+ taskParams.getExtras().getInt("randomNum"));
if (!Utils.verify(settings)) {
Log.d(Utils.TAG, "AUTO. Задача не отработала");
Intent responseIntent = new Intent();
responseIntent.setAction(Utils.ACTION_MYINTENTSERVICE);
responseIntent.addCategory(Intent.CATEGORY_DEFAULT);
responseIntent.putExtra(Utils.EXTRA_KEY_OUT, false);
Log.d(Utils.TAG, "Загрузка не произошла");
sendBroadcast(responseIntent);
} else {
Log.d(Utils.TAG, "AUTO. Задача отработала успешно");
}
return GcmNetworkManager.RESULT_SUCCESS;
}
}
Как думаете, так лучше?
4. Про повороты-то я не подумал. Спасибо.
C Rx сижу, читаю, уму-разуму набираюсь :)
3. Вам не нужно постоянно лазить в SharedPreferences. Он вам нужен чтобы загрузить/сохранить «статус» элементов, а для работы в коде нужно использовать какую либо переменную. А SharedPreferences лучше вообще один раз инициализировать при запуске приложения.
Наследуетесь от Application, инициализируете SharedPreferences. Данный App нужно также прописать в манифесте, чтобы работало.
public class App extends Application {
public static SharedPreferences sSharedPreferences;
@Override
public void onCreate() {
super.onCreate();
sSharedPreferences = PreferenceManager.getDefaultSharedPreferences(this); // или ваш вариант, с кастомным именем
}
public static SharedPreferences getSharedPreferences() {
return sSharedPreferences;
}
}
После, например создаете синглтон, что-то вида.
public class PreferencesHelper {
private static PreferencesHelper INSTANCE = null;
private SharedPreferences mSharedPreferences;
private SharedPreferences.Editor mEditor;
public static PreferencesHelper getInstance() {
if(INSTANCE == null) {
INSTANCE = new PreferencesHelper();
}
return INSTANCE;
}
public PreferencesHelper() {
this.mSharedPreferences = App.getSharedPreferences();
this.mEditor = this.mSharedPreferences.edit();
}
//для примера сохранение данных в SharedPreferences
public void saveStatusCheckbox(String name, boolean status){
mEditor.putBoolean(name, status);
mEditor.apply();
}
//для примера загрузка из данных из SharedPreferences
public int loadStatusCheckbox(String name){
return mSharedPreferences.getBoolean(name, false);
}
}
и используете его в Activity вот так.
PreferencesHelper helper = PreferencesHelper.getInstance();
Boolean vip_status = helper.loadStatusCheckbox(Constants.CHECK_BOX_VIP_NAME);
А с переменной vip_status прыгать по методам дальше. И при изменении самого чекбокса менять ее, а не значение в Preferences. Его сохранять при выходе из активити, например в том же onDestroy.
Но это чисто работа с Preferences. Важнее продумать работу например при повороте экрана, или вариант того, что пользователь начнет упорно клацать кнопку ручного запуска задачи, тогда ваше приложение скорее всего упадет из-за нехватки памяти)
Для классической реализации синглтон, конструктор private и чтобы избежать проблем добавить synchronized в метод getInstance
private PreferencesHelper() {
this.mSharedPreferences = App.getSharedPreferences();
this.mEditor = this.mSharedPreferences.edit();
}
public static synchronized PreferencesHelper getInstance() {
if(INSTANCE == null) {
INSTANCE = new PreferencesHelper();
}
return INSTANCE;
}
или что мне больше нравится, разделить метод getInstance на два метода initInstance и getInstance:
public class App extends Application {
@Override
public void onCreate() {
super.onCreate();
PreferencesHelper.initInstance(this);
}
}
public class PreferencesHelper {
private static PreferencesHelper INSTANCE = null;
private SharedPreferences mSharedPreferences;
private SharedPreferences.Editor mEditor;
public static void initInstance(Conext context) {
INSTANCE = new PreferencesHelper(context);
}
public static PreferencesHelper getInstance() {
return INSTANCE;
}
private PreferencesHelper(Conext context) {
this.mSharedPreferences = PreferenceManager.getDefaultSharedPreferences(context);
this.mEditor = this.mSharedPreferences.edit();
}
//для примера сохранение данных в SharedPreferences
public void saveStatusCheckbox(String name, boolean status){
mEditor.putBoolean(name, status);
mEditor.apply();
}
//для примера загрузка из данных из SharedPreferences
public int loadStatusCheckbox(String name){
return mSharedPreferences.getBoolean(name, false);
}
}
Какой практический смысл разделения метода getInstance на 2 отдельных? Где может понадобиться инициализировать его, но не вызывать?
Во-вторых.
Вы предлагаете привязывать класс хелпера к контексту каждого нового активити и фрагмента? Так, а зачем вам тогда синглтон в данном случае?
Каждый раз вы будете дергать новый SharedPreferences, я наоборот уводил от этого автора.
«Application.onCreate called when the application is starting, before any activity, service, or receiver objects (excluding content providers) have been created. »
и т.о. метод initInstance вызывается один раз и инициализирует static INSTANCE соответственно тоже один раз при старте процесса приложения.
Метод getInstance просто getter, предоставляющий доступ к INTANCE. Все время используется один и тот же SharedPreferences, который получен при старте приложения.
Целый день за монитором дает о себе знать. Надо идти спать…
- Итак, сразу бросается в глаза странное использование GcmNetworkManager. Фактически он сейчас заменяет простой AlarmManager. Хотелось бы чтобы он всё-таки использовался по назначению.
- Задача явно должна быть не PeriodicTask. И тогда если она завершилась с ошибкой достаточно будет вернуть RESULT_RESCHEDULE. Система всё сделает самостоятельно её перезапустит в соответсвии с back-off policy. Я бы делал, например, 3 запроса подряд и возвращал RESULT_RESCHEDULE если всё плохо.
- Также было бы замечательно добавить к задаче gap (flex или большой setExecutionWindow) для выполнения. А также setUpdateCurrent(true) для того чтобы не плодить несколько задач по ошибке. Как легко можно сделать сейчас, выходя и заходя в приложение снова.
По общей структуре:
- Допустим что серия из 20 вызовов это бизнес требование, тогда, мне кажется, что лучше это делать в одном вызове IntentService. Зачем эта петля с созданием «broadcast <> service <> broadcast» не совсем понятно.
- Дублирование кода в сервисах нужно убрать, это ни к чему хорошему не приводит.
- Вызывать Thread.sleep в MainThread (MyBroadRec.onReceive) не самая лучшая идея.
- Если уж используете broadcast, то желательно использовать LocalBroadcastManager.
Написал несколько сумбурно и урывками, т.к. достаточно неудобно это делать после статьи. Pull-request на порядок удобнее, даже несмотря на отсутствие навигации по коду (конечно исключая upsource).
В общем, все стало немного лучше. Можно даже сказать — идём правильно дорогой… но пока что на руках и забираем немного в сторону.
В рабочем приложении, все завязано на наличие устойчивой связи с интернетом. Если есть связь, то раз в 300 секунд отрабатывает задача синхронизации с сервером. Разве не так надо использовать? За образец я брал эту статью.
Уточните, пожалуйста, а что это gap и flex? Не нашел в сети. А что нашел, то кажется не то.
Дублирование убрал, и переписал через Application и синглтоны, как посоветовали выше.
Завтра выложу, улучшенный код в примечаниях.
Тогда всё в порядке, я не прав. Моя идея была запустить задачу только если нужно что-то синхронизировать и отдать на откуп системе когда всё это выполниться. Т.е. использовать OneoffTask
и возвращать RESULT_RESCHEDULE
пока всё-таки не получиться синхронизироваться. Но если важен вызов каждые N секунд, тогда конечно нужно использовать PeriodicTask
, только нужно добавить условие выхода из цикла обновления, а то вызов каждые 30 секунд даже когда обновлять не нужно — выглядит не очень правильно.
Под "gap" я подразумевал окно в которое может быть вызвана задача, для PeriodicTask
это устанавливается методом setFlex
. Лучше задавать окна, это поможет системе группировать выполнение различных задач и будить устройство немного реже.
А, под условием выхода, не подходит этот кусок:
if(!result && qnt<20){
Log.d(Utils.TAG, "Новая попытка № "+qnt);
qnt++;
try {
Thread.sleep(100);
} catch (InterruptedException e) {
e.printStackTrace();
}
startService(intentRec);
}
else {
qnt=0;
}
}
Опубликовал новый код в примечании.
Ага, тогда понятно. Тогда всё нормально, под условие выхода этот код вполне подходит.
У меня несколько другой опыт — обычно что-то загружаю на сервер, а не скачиваю с него. Отсюда и попытки внести не нужные исправления.
Таймер с ручным запуском (работа над ошибками)