oMystique commented 6 years ago

Смотрел в ветке parser-rss

Замечания по коду

InsertRssTask

[x] Вполне можно сразу проверить respBody на null и произвести необходимые манипуляции, вместо присваивания null переменной rss, с последующей проверкой
```
    final Rss rss = respBody == null ? null : Rss.parse(new String(respBody));

    if (rss == null) {
        setError(ErrorState.InvalidFormat);

        return null;
    }

    rss.setUrl(feedUrl);
```
[x] Переменная нигде не используется
ArticleListAdapter
```
private Context mContext;
```

Others

[x] Переменная нигде не используется
```
private static final String ARTICLE_IMG_LENGTH = "length";
```
[x] Вещи, которые не имплементированы, но их реализация планируется, лучше помечать комментарием //TODO

[x] Класс нигде не используется

public class Util {
public static Integer parseInt(String value) {
    try {
        return Integer.parseInt(value);
    } catch (Exception ex) {
        return null;
    }
}
}

Замечания по приложению

[x] Вьюху с лентами следует сделать другого цвета. Сейчас она сливается с контентом самой ленты. Быть может, достаточно будет добавить тень. Или же сделать менее блеклым Navigation Bar. Следует проверить.
[x] Приложение багается при повороте устройства. Воспроизведено на эмуляторе. Следует проверить.
[x] Дата выглядит странно

oMystique commented 6 years ago

[x] Не стоит оставлять пустым блок catch Лучше запилить внутри него логгирование ошибки с определенным тегом
[x] В ячейках новостей лучше явно указывать дату вместе с другим контентом
[x] Должна быть дефолтная картинка у каждой новости http://orehab.ru/wp-content/uploads/2016/08/no-photo.png

genaku commented 6 years ago

В RssTabsAdapter хранить фрагменты RssTabFragment не стоит, потому как управлением фрагментами занимается сам FragmentStatePagerAdapter. Лучше хранить там список с параметрами rss-лент и при добавлении/удалении этих лент оперировать не добавлением/удалением фрагментов, а списком с параметрами этих лент.

Вместо

    public Fragment getItem(int position) {
        return mFragments.get(mRssList.get(position).getFeed());
}

надо использовать стандартный для этого адаптера метод:

    public Fragment getItem(int position) {
        return RssTabFragment.newInstance(params[position]);
    }

где params[position] это какие-то параметры для rss-ленты из списка лент, хранящегося внутри адаптера.

Передавать команды на обновление и т.д. текущего фрагмента можно напрямую. Для этого можно использовать SmartFragmentStatePagerAdapter. У него есть метод: getRegisteredFragment(int position)

oMystique commented 6 years ago

Смотрел в ветке Database

[x] Приложение падает при обновлении RSS-ленты Emulator Nexus 5, 26 API

E/ru.iandreyshev.parserrss.ui.activity.FeedActivity: https://lenta.ru/rss/last24
E/ru.iandreyshev.parserrss.models.rss.Rss: Add 7 new articles
D/AndroidRuntime: Shutting down VM
E/AndroidRuntime: FATAL EXCEPTION: main
              Process: ru.iandreyshev.parserrss, PID: 15231
              java.lang.NullPointerException: Attempt to invoke interface method 'void ru.iandreyshev.parserrss.app.IEvent.doEvent()' on a null object reference
                  at ru.iandreyshev.parserrss.models.async.UpdateRssFromNetTask.onPostExecute(UpdateRssFromNetTask.java:55)
                  at ru.iandreyshev.parserrss.models.async.UpdateRssFromNetTask.onPostExecute(UpdateRssFromNetTask.java:13)
                  at android.os.AsyncTask.finish(AsyncTask.java:695)
                  at android.os.AsyncTask.-wrap1(Unknown Source:0)
                  at android.os.AsyncTask$InternalHandler.handleMessage(AsyncTask.java:712)
                  at android.os.Handler.dispatchMessage(Handler.java:105)
                  at android.os.Looper.loop(Looper.java:164)
                  at android.app.ActivityThread.main(ActivityThread.java:6541)
                  at java.lang.reflect.Method.invoke(Native Method)
                  at com.android.internal.os.Zygote$MethodAndArgsCaller.run(Zygote.java:240)
                  at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:767)

[x] При повторном открытии приложения все ленты пусты. При их повторном обновлении - приложение вновь валится

oMystique commented 6 years ago

[x] При открытии полной новости - краш Emulator Nexus 5, 26 API


E/JavaBinder: !!! FAILED BINDER TRANSACTION !!!  (parcel size = 819400)
D/AndroidRuntime: Shutting down VM
E/AndroidRuntime: FATAL EXCEPTION: main
              Process: ru.iandreyshev.parserrss, PID: 15844
              java.lang.RuntimeException: android.os.TransactionTooLargeException: data parcel size 819400 bytes
                  at android.app.ActivityThread$StopInfo.run(ActivityThread.java:4006)
                  at android.os.Handler.handleCallback(Handler.java:789)
                  at android.os.Handler.dispatchMessage(Handler.java:98)
                  at android.os.Looper.loop(Looper.java:164)
                  at android.app.ActivityThread.main(ActivityThread.java:6541)
                  at java.lang.reflect.Method.invoke(Native Method)
                  at com.android.internal.os.Zygote$MethodAndArgsCaller.run(Zygote.java:240)
                  at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:767)
               Caused by: android.os.TransactionTooLargeException: data parcel size 819400 bytes
                  at android.os.BinderProxy.transactNative(Native Method)
                  at android.os.BinderProxy.transact(Binder.java:748)
                  at android.app.IActivityManager$Stub$Proxy.activityStopped(IActivityManager.java:4636)
                  at android.app.ActivityThread$StopInfo.run(ActivityThread.java:3998)
                  at android.os.Handler.handleCallback(Handler.java:789) 
                  at android.os.Handler.dispatchMessage(Handler.java:98) 
                  at android.os.Looper.loop(Looper.java:164) 
                  at android.app.ActivityThread.main(ActivityThread.java:6541) 
                  at java.lang.reflect.Method.invoke(Native Method) 
                  at com.android.internal.os.Zygote$MethodAndArgsCaller.run(Zygote.java:240) 
                  at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:767)

oMystique commented 6 years ago

[x] Все еще актуально

Вьюху с лентами следует сделать другого цвета. Сейчас она сливается с контентом самой ленты. Быть может, достаточно будет добавить тень. Или же сделать менее блеклым Navigation Bar. Следует проверить.

oMystique commented 6 years ago

[x] При сворачивании приложения происходит краш

E/AndroidRuntime: FATAL EXCEPTION: main
              Process: ru.iandreyshev.parserrss, PID: 16300
              java.lang.RuntimeException: android.os.TransactionTooLargeException: data parcel size 819304 bytes
                  at android.app.ActivityThread$StopInfo.run(ActivityThread.java:4006)
                  at android.os.Handler.handleCallback(Handler.java:789)
                  at android.os.Handler.dispatchMessage(Handler.java:98)
                  at android.os.Looper.loop(Looper.java:164)
                  at android.app.ActivityThread.main(ActivityThread.java:6541)
                  at java.lang.reflect.Method.invoke(Native Method)
                  at com.android.internal.os.Zygote$MethodAndArgsCaller.run(Zygote.java:240)
                  at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:767)
               Caused by: android.os.TransactionTooLargeException: data parcel size 819304 bytes
                  at android.os.BinderProxy.transactNative(Native Method)
                  at android.os.BinderProxy.transact(Binder.java:748)
                  at android.app.IActivityManager$Stub$Proxy.activityStopped(IActivityManager.java:4636)
                  at android.app.ActivityThread$StopInfo.run(ActivityThread.java:3998)

oMystique commented 6 years ago

[x] Публичные объявления лучше располагать в начале класса, затем должны идти protected и private

private boolean saveRssToDatabase() {
    try {

        if (mDatabase.putRssIfSameUrlNotExist(mRss)) {
            return true;
        }

        mResultEvent = () -> mListener.onDuplicateRss();

    } catch (Exception ex) {
        Log.e(TAG, Log.getStackTraceString(ex));
        mResultEvent = () -> mListener.onDbError();
    }

    return false;
}

public interface IEventListener extends ITaskListener<ViewRss> {
    void onInvalidUrl();

    void onNetError(final IHttpRequestResult requestResult);

    void onParsingError();

    void onDbError();

    void onDuplicateRss();

    void onSuccess(final ViewRss rss);
}

===================================

    private GetRssFromNetTask() {
    }

    public static void execute(final IEventListener listener, final String url) {
        final GetRssFromNetTask task = new GetRssFromNetTask();
        task.setTaskListener(listener);
        task.mRequestHandler = new HttpRequestHandler(url);
        task.mListener = listener;
        task.execute();
    }

oMystique commented 6 years ago

[x] Очень странно, лучше пометить комментариями вида //TODO, если это не доделано. Либо же, зарефакторить

@Override
protected ViewRss doInBackground(final String... strings) {
    if (!validateUrl()) {
        return null;
    } else if (!validateExistence()) {
        return null;
    } else if (!getRssFromNet()) {
        return null;
    } else if (!parseRss()) {
        return null;
    } else if (!saveRssToDatabase()) {
        return null;
    }

    mResultEvent = () -> mListener.onSuccess(mRss);

    return mRss;
}

oMystique commented 6 years ago

[x] Вероятно, в финале данной функции должно возвращаться true.

private boolean parseRss() {
    if ((mRssFromNet = RssParser.parse(mRequestHandler.getResponseBody())) == null) {
        mResultEvent = () -> mListener.onParsingError(mUpdatingRss);

        return false;
    }

    mRssFromNet.setUrl(mRequestHandler.getUrlStr());

    return false;
}

Иначе, проверка вида

} else if (!parseRss()) {
            return null;

всегда будет проходить, что нам не нужно

oMystique commented 6 years ago

[x] У классов GetRssFromNetTaskи UpdateRssFromNetTask однотипный функционал методов методов doInBackground, getRssFromNet, parseRss... следует вынести в отдельный класс, дабы не плодить дублирующиеся фрагменты кода

oMystique commented 6 years ago

[x] doInBackgroundи подобные методы, следует пометить как @Nullable, дабы присутствовала сигнализация того, что метод может вернуть null

oMystique commented 6 years ago

[x] Нигде не используемая переменная

private static final String TAG = HttpRequestHandler.class.getName();

oMystique commented 6 years ago

[x] Возвращаемое значение нигде не используется. Его точно стоит возвращать?
```
public IHttpRequestResult sendGet() {
```

oMystique commented 6 years ago

[x] Падают тесты RSSParserV2Tests


java.lang.AssertionError
at org.junit.Assert.fail(Assert.java:86)
at org.junit.Assert.assertTrue(Assert.java:41)
at org.junit.Assert.assertNotNull(Assert.java:712)
at org.junit.Assert.assertNotNull(Assert.java:722)
at ru.iandreyshev.parserrss.models.rss.RssParserV2Test.parseFileAndCheck(RssParserV2Test.java:140)
at ru.iandreyshev.parserrss.models.rss.RssParserV2Test.not_parse_articles_in_items_node(RssParserV2Test.java:57)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:51)
at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at com.intellij.rt.execution.application.AppMainV2.main(AppMainV2.java:131)

java.lang.AssertionError at org.junit.Assert.fail(Assert.java:86) at org.junit.Assert.assertTrue(Assert.java:41) at org.junit.Assert.assertNotNull(Assert.java:712) at org.junit.Assert.assertNotNull(Assert.java:722) at ru.iandreyshev.parserrss.models.rss.RssParserV2Test.parseFileAndCheck(RssParserV2Test.java:140) at ru.iandreyshev.parserrss.models.rss.RssParserV2Test.parse_rss_with_feed_title_and_description_only(RssParserV2Test.java:35) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26) at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57) at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) at org.junit.runners.ParentRunner.run(ParentRunner.java:363) at org.junit.runner.JUnitCore.run(JUnitCore.java:137) at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68) at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:51) at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242) at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at com.intellij.rt.execution.application.AppMainV2.main(AppMainV2.java:131)

...

oMystique commented 6 years ago

[x] Падают тесты HttpRequestHandlerTest


java.lang.NullPointerException
at ru.iandreyshev.parserrss.models.web.HttpRequestHandlerTest.return_not_send_state_after_create(HttpRequestHandlerTest.java:22)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:51)
at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at com.intellij.rt.execution.application.AppMainV2.main(AppMainV2.java:131)

java.lang.NullPointerException at ru.iandreyshev.parserrss.models.web.HttpRequestHandler.sendGet(HttpRequestHandler.java:35) at ru.iandreyshev.parserrss.models.web.HttpRequestHandlerTest.return_invalid_url_state_after_send_with_null_url(HttpRequestHandlerTest.java:15) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57) at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) at org.junit.runners.ParentRunner.run(ParentRunner.java:363) at org.junit.runner.JUnitCore.run(JUnitCore.java:137) at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68) at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:51) at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242) at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at com.intellij.rt.execution.application.AppMainV2.main(AppMainV2.java:131)

oMystique commented 6 years ago

[ ] В тестах Урл-Парсера следует добавить кейсы с невалидными урлами. Нет схемы, кривое доменное имя, наличие порта и т.п

public class UrlTest {
    private static final String VALID_URL = "http://domain.com/";

    @Test
    public void return_null_if_parse_null_string() {
        assertNull(Url.parse(null));
    }

    @Test
    public void return_null_if_parse_empty_string() {
        assertNull(Url.parse(""));
    }

    @Test
    public void return_url_string_in_toString_method() {
        final Url url = Url.parse(VALID_URL);

        assertNotNull(url);
        assertEquals(VALID_URL, url.toString());
    }
}

oMystique commented 6 years ago

[x] Нужно эти вещи отправить в values

    private static final String BAD_CONNECTION = "Connection error";
    private static final String BAD_URL = "Invalid url";
    private static final String NET_PERMISSION_DENIED = "Internet permission denied";
    private static final String PARSER_ERROR = "Invalid rss format";
    private static final String DATABASE_ERROR = "Saving error";
    private static final String RSS_WAS_DELETED = "Rss not exist";

oMystique commented 6 years ago

[x] А нужен ли этот фрагмент кода? Он не используется

// Returns the fragment for the position (if instantiated)
Fragment getRegisteredFragment(int position) {
    return registeredFragments.get(position);
}

oMystique commented 6 years ago

[x] Лучше пометить методы комментарием типа "Имплементируй в наследниках, если нужно"
```
protected void onInvalidUrl() {
}

protected void onNetError(final IHttpRequestResult requestResult) {
}

protected void onParserError() {
}
```

oMystique commented 6 years ago

[x] Нужно добавить scrollbar в активити просмотра списка новостей и активити просмотра полной новости, если там текст можно проскроллить

oMystique commented 6 years ago

[ ] Полагаю, можно избавиться от статической надписи "Feed", уменьшить высоту Navigation Bar'a. Кнопку ADD отправить в контекстное меню, а табы расположить до контекстного меню. Важно, чтобы контекстное меню располагалось статично и при скролле табов не меняло свое положение, а так же отделялось бы графически. Например, тенью.

oMystique commented 6 years ago

[ ] Примерно вот так =)

oMystique commented 6 years ago

[ ] Класс UrlTest лучше снабдить тестами, в виду того, что мы тестируем обертку собственного класса над какой-то библиотекой

oMystique commented 6 years ago

[x] Лучше обозвать константы в зависимости от их семантического предназначения

        if (right.getDate() == null && left.getDate() == null) {
            return 0;
        } else if (right.getDate() == null) {
            return -1;
        } else if (left.getDate() == null) {
            return 1;
        }

genaku commented 6 years ago

По Clean Architecture. Это хорошо, что ты сущности базы данных отделил от модели. Но на данный момент у тебя классическая MVP, вся логика приложения зашита в презентерах. Изучи статью https://habrahabr.ru/company/mobileup/blog/335382/

genaku commented 6 years ago

По Kotlin: Конструкции вида: if (entityProperty == null) { return null } можно упростить: entityProperty ?: return null

oMystique commented 6 years ago

Здесь работу закончили.

iandreyshev / android_parser_rss

Замечания по программе ParserRss #2

Замечания по коду

InsertRssTask

ArticleListAdapter

Others

Замечания по приложению