I-O Schedule 2014: плохой пример для обучения

Мы уже привыкли, что приложение для Google I/O дефакто стандарт архитектуры приложения, написания кода и дизайна.

Вот и в этот раз, я решил посмотреть, что же нового появилось в приложении. С дизайном все понятно, точнее понятно, что людям нужно снова учится делать его «правильно». Но меня больше интересовал код — что же нового там есть?

Но ничего нового я не увидел, но осознал, что приложение абсолютно не годится как наглядное пособие для обучения начинающих разработчиков.

После быстрого осмотра кода получился вот такой список замечаний.

Disclaimer: не претендую на истину в последней инстанции; большинство замечаний вокруг MySchedule

1. Явно не хватает структурированности в пакете UI

Почему бы не выделить в отдельные пакеты каждую часть?
Можно считать это замечание «притянутым за уши», но мне кажется, что лишние пакеты тут бы не помешали

2. Загрузка данных в MyScheduleActivity.java

Находим метод updateData который загребает данные для всех дней сразу, через AsyncTask

почему это странно и неправильно? тут несколько факторов

1. судя по тому что они перезагружают данные на onresume им бы подошел Loader который делал бы это автоматически.
   Не подходит CursorLoader, можно юзать AsyncTaskLoader. Тем более он бы кенселил загрузку если юзер ушел с экрана. AsyncTask этого сам сделать не может.
2. фактически они сторят данные в адаптере. в это ситуации, логичнее сторить данные моделькой в активити(хотя мне это не нравится тоже), создавать адаптеры по необходимости. и только тогда скармливать им модельку.

3. Ужасный MyScheduleAdapter.java

По факту это обычный ArrayAdapter, но нет, надо все самим написать и наследоваться от ListAdapter. метод getView размеров в 5 экранов! Серьезно, google? как это полотно читать?

Его явно писали несколько человек в лучших традициях — «не хочу рефакторить», «не хочу разбираться». иначе для меня загадка почему стоит класть в мемберы эти 3 цвета, которые используются в getView

mDefaultSessionColor = mContext.getResources().getColor(R.color.default_session_color);
mDefaultStartTimeColor = mContext.getResources().getColor(R.color.body_text_2);
mDefaultEndTimeColor = mContext.getResources().getColor(R.color.body_text_3);

а все остальные, которые используются там же, нет.

4. Странное использование фрагмента — MyScheduleFragment

Идем в MyScheduleActivity.java и смотрим как юзается фрагмент.

@Override
    public void onFragmentViewCreated(ListFragment fragment) {
        fragment.getListView().addHeaderView(
                getLayoutInflater().inflate(R.layout.reserve_action_bar_space_header_view, null));
        int dayIndex = fragment.getArguments().getInt(ARG_CONFERENCE_DAY_INDEX, 0);
        fragment.setListAdapter(mScheduleAdapters[dayIndex]);
        fragment.getListView().setRecyclerListener(mScheduleAdapters[dayIndex]);
    }

Оказывается, ребятам нужен был ViewPager с ListView. Играться с PagerAdapter и ListView они не захотели т.к. FragmentPagerAdapter уже есть и менеджит фрагменты отлично. да еще и методы getListView и setListAdapter у фрагмента публичные, почему бы не юзать. (с точно такими аргументами ко мне приходил и практикант, когда увидел в задаче пункт «использовать фрагменты»).

Ошибка в чем? часть фрагмента напрямую менеджется из активити. хотя по задумке фрагмент должен быть автономным.

Решение: если фрагмнет не может самостоятельно получить данные(у них они формируются одним скопом в активити), то надо задефайнить интерфес Provider во фрагменте, этот активити реализует интерфейс и провайдит данные через него во фрагмент. фрагмнет получит свои данные. и покладет их в адаптер.
Но это ведь почти тоже самое. что делают они, возразит читатель.
Почти! это как котлеты кушать вместе с борщем, а не после. в результате они попадут в желудок, но вот процесс.

В чем разница. Фрагмет автономная едицница со списком и адаптерво внутри. он умеет отобразить данные. проблема только в самих данных — их нет. вот через этот интерфейс мы их и получим, а не нам их засунут.

вот еще решение, которое лучше первого — фрагмент умеет грузить данные для себя! т.е. лоадер в самом фрагменте. и грузит данных столько сколько надо.

5. В адаптерха не используются ViewHolder

во всех адаптерах просто дергают findViewById.
хотя все мы знаем как надо ViewHolder

6. Куча не нужных обсерверов

каждый класс считает своим долгом повесить обсервер

доходит до обсурда — перезапускают лоадер из обсервера. хотя лоадер то и сам все должен узнавать об изменении данных.
Сам-то сам, если пронотифаить нужные url. опять «если». SessionsFragment.java

        mSessionsObserver = new ThrottledContentObserver(new ThrottledContentObserver.Callbacks() {
            @Override
            public void onThrottledContentObserverFired() {
                onSessionsContentChanged();
            }
        });
        activity.getContentResolver().registerContentObserver(
                ScheduleContract.Sessions.CONTENT_URI, true, mSessionsObserver);

7. Неправильное использование лоадеров

очень часто ребята закрывают cursor в лоадере, что противопоказано — Android Devs

один из примеров

mTagMetadata = new TagMetadata(cursor);
cursor.close();

1. они обошли курсор полностью в UI
2. закрыли курсор
3. потом будут рендерить теги(будут инфлейтить вьющки и напихать в LinearLayout)

что бы освободить курсор можно задестроить лоайдер. но если он им нужен что бы перегружать данные вовремя — то нужно делать кастомный AsyncTaskLoader + обсервер(тут он будет уместен).
Лоадер загрузит крусор, обойдет его, закроет и выдаст уже модельку.

8. Использование inline string вместо констант

можете поискать по коду "_uri"

Возможно я и придираюсь, возможно код пережил несколько поколений разработчиков. но можно еще заглянуть в сервисы — там тоже по 10-20 акшенов через switch разбираются.

ИМХО, начинающему разработчику учится по этому приложению не стоит.

Автор: hamsterksu

Источник