Как стать автором
Обновить

Комментарии 23

И попало письмо по проводам оптоволоконным и медным к администраторам веселым.
И читали его программисты ученые и хвалили Ваньку за старания евоные.
А Ванька-то, слушал да радовался тихо. Похвала ж, она и кошке приятна, даром, что скотина она бессловесная, а уж начинающему разработчику, как бальзам на душу. :)
Хороший вы ресурс нашли для оттачивания кода)
Вам в прошлом посте вроде советовали Rx, гляньте на него, ну правда.
А теперь вопросы.
1. Разметка активити, хардкод текста
2. Класс Utils (который больше похож на «Constants»), часть переменных final, часть нет. Создается впечатление что вы не видите разницу.
3. Метод verify(). Во первых, повторяется 2 раза, принцип DRY считай нарушен. Насколько я понимаю метод дергается при каждой новой попытке, и каждый раз создает новый экземпляр SharedPreferences, как-то это не правильно. Да и вообще хранить данные переменные в SharedPreferences не совсем верно.
4. Я даже боюсь думать, что будет если запустить все это, и начать крутить телефон)
Почитайте про Rx и MVP, вы сможете упростить и укоротить свой код в разы)
1. Уточните пожалуйста, что вы имеете в виду? Вроде от хардкода (если я правильно понимаю — жесткое «вшивание» в программный код различных данных, касающихся окружения программы), избавился. Разметка активности — самая простая.
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 сижу, читаю, уму-разуму набираюсь :)
1. Я имел ввиду текст на кнопках и чекбоксах, не вынесенный в strings, хотя это скорее мои придирки учитывая что это тест проект на коленке.
3. Вам не нужно постоянно лазить в SharedPreferences. Он вам нужен чтобы загрузить/сохранить «статус» элементов, а для работы в коде нужно использовать какую либо переменную. А 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, я наоборот уводил от этого автора.
Отвечу сначала на второй вопрос: Как видно из кода в моем посте PreferencesHelper инициализируется с использованием контекста Application в методе onCreate наследника класса Application, а не Activity или фрагмента.

«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, который получен при старте приложения.
Извиняюсь за «наезд», прочитал ваш код еще раз и все таки увидел переписанный App и все стало на свои места)
Целый день за монитором дает о себе знать. Надо идти спать…
Взаимно :). Я написал свой ответ, а потом только увидел, Ваш второй ответ.
Оффтопик, но как шикарно читать, когда спорят два вежливых человека :)
Раз уж меня упомянули то предётся зайти. Итак:
По GcmNetworkManager:
  • Итак, сразу бросается в глаза странное использование 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. Лучше задавать окна, это поможет системе группировать выполнение различных задач и будить устройство немного реже.

Там такое требование, синхронизироваться каждые 5 минут и проверять, появилось ли что-то новенькое?
А, под условием выхода, не подходит этот кусок:

 if(!result && qnt<20){
                Log.d(Utils.TAG, "Новая попытка № "+qnt);
                qnt++;
                try {
                    Thread.sleep(100);
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
                startService(intentRec);
            }
            else {
                qnt=0;
            }
        }


Опубликовал новый код в примечании.

Ага, тогда понятно. Тогда всё нормально, под условие выхода этот код вполне подходит.
У меня несколько другой опыт — обычно что-то загружаю на сервер, а не скачиваю с него. Отсюда и попытки внести не нужные исправления.

Но ваши замечания очень ценные.
И мне еще предстоит вторая часть задачи, как раз отправлять на сервер результаты деятельности пользователя. :)
Почему вы решили не использовать систему контроля версий? Если кто-то ещё будет вносить предложения, появятся «P.S.S.»? :)
Буду использовать, обязательно. :)

P.S.: А ведь шикарный заголовок мог бы получиться «P.S. Хабр, я люблю тебя» :)
Зарегистрируйтесь на Хабре , чтобы оставить комментарий

Публикации

Истории