Add main functionality with out dagger2 nor data manager #1

AFGhazy · 2018-07-28T11:19:33Z

No description provided.

IslamSalah

@ghazy The sample is actually impressive. Most of the comments I left are minor.
Here are some points I believe you need to consider IMHO:

Consider having a good commit history with descriptive name [The same comment was left on Quiz1]
The Project structure needs to be improved a little to follow MVP pattern:
= No need for adapter package, we can move the adapter to the concerned feature package.
= No need to activity package, check how this sample handled it.
= No need for viewHolders package, we should rather move it the feature package.
= All feature packages can be enclosed in anther package. check how this sample handled it.
= apiInterface and networks packages should be nested into data package. Check the above sample.
= All package names should be lowercase without underscores. Check (this)[https://docs.oracle.com/javase/tutorial/java/package/namingpkgs.html]
CC @ayman

IslamSalah · 2018-08-01T10:15:58Z

app/src/main/java/com/blink22/android/mvpandroid/activities/BaseActivity.java

+ */
+
+public abstract class BaseActivity extends AppCompatActivity implements NavigationView.OnNavigationItemSelectedListener {
+    protected abstract Fragment createFragment();


@AFGhazy Kindly check this for best practices for class member ordering.

IslamSalah · 2018-08-01T10:45:01Z

app/src/main/java/com/blink22/android/mvpandroid/activities/BaseActivity.java

+    @BindView(R.id.nav_view) NavigationView navigationView;
+
+    @Override
+    protected void onCreate(@Nullable Bundle savedInstanceState) {


@AFGhazy I suggest breaking this method into smaller ones. [Check similar comment on Quiz1 PR].

IslamSalah · 2018-08-01T10:48:04Z

app/src/main/java/com/blink22/android/mvpandroid/activities/BaseActivity.java

+
+        for(NavigationItemEnum item: NavigationItemEnum.values()) {
+            if(item.getId() == menuItem.getItemId()) {
+                Intent i = new Intent(getApplicationContext(), item.getClassToLaunch());


@AFGhazy Intent iI vote for using descriptive good names rather than very short ones. What do you think? [minor comment]

also why getApplicationContext(), this or getActivity in case of fragment should be enough

IslamSalah · 2018-08-01T11:29:28Z

app/src/main/java/com/blink22/android/mvpandroid/newTodo/NewTodoContract.java

+    interface View extends BaseView<Presenter> {
+
+        String getTitle();
+        String getDescription();


@AFGhazy I suggest leaving blank lines between different methods.

IslamSalah · 2018-08-01T11:32:02Z

db.json

-      "title": "Buy grocery",
      "details": "Go to the market to buy carrots",
-      "done": false
+      "done": true,


@AFGhazy +1 For making a virtual back-end server 👍

IslamSalah · 2018-08-01T12:01:45Z

app/build.gradle


-    implementation 'io.reactivex:rxandroid:1.2.1'
-    implementation 'io.reactivex:rxjava:1.3.0'
+    implementation 'io.reactivex.rxjava2:rxandroid:2.0.2'


@AFGhazy +1 For using RxJava

IslamSalah · 2018-08-01T12:02:19Z

app/src/main/java/com/blink22/android/mvpandroid/BasePresenter.java

+ * Created by ahmedghazy on 7/25/18.
+ */
+
+public interface BasePresenter extends LifecycleObserver{


@AFGhazy +1 for using LifecycleObserver

IslamSalah · 2018-08-01T12:10:24Z

app/src/main/java/com/blink22/android/mvpandroid/todos/TodosPresenter.java

+    }
+
+    @Override
+    @OnLifecycleEvent(Lifecycle.Event.ON_DESTROY) public void stop() {


@AFGhazy I suggest putting the annotations in a separate line abovew the method signature as you did in NewTodoPresenter Check this

IslamSalah · 2018-08-01T12:13:30Z

app/src/main/res/layout/item_todos.xml

+    android:padding="20dp"
+    >
+
+


@AFGhazy I vote for removing these many blank lines.

IslamSalah · 2018-08-01T12:26:19Z

app/src/main/java/com/blink22/android/mvpandroid/todos/TodosContract.java

+
+    interface Presenter extends BasePresenter {
+
+        void getTodos();


@AFGhazy +1 for nice contract method names.

TarekAshrafAhmed · 2018-08-01T17:12:34Z

app/src/main/java/com/blink22/android/mvpandroid/todos/TodosPresenter.java

+    public void getTodos() {
+
+        mView.showWait();
+


@AFGhazy avoid too many blank lines

TarekAshrafAhmed · 2018-08-01T17:14:41Z

app/src/main/java/com/blink22/android/mvpandroid/todos/TodosPresenter.java

+    @Override
+    public void getTodos() {
+
+        mView.showWait();


@AFGhazy Always check that views are available before calling them, the callbacks may be received after the view is closed or killed by the OS

TarekAshrafAhmed · 2018-08-01T17:16:47Z

app/src/main/java/com/blink22/android/mvpandroid/viewHolders/TodosVH.java

+        mTitle.setText(todo.getTitle());
+        mDescription.setText(todo.getDetails());
+        mDone.setChecked(todo.isDone());
+        mDone.setOnClickListener(new View.OnClickListener() {


@AFGhazy Butterknife it

TarekAshrafAhmed · 2018-08-01T17:29:41Z

app/src/main/java/com/blink22/android/mvpandroid/todos/TodosPresenter.java

+
+        DisposableObserver<Todo> disposableObserver = mTodosSubscriber.updateTodo(new TodosSubscriber.Callback<Todo>() {
+            @Override
+            public void onSuccess(Todo todo) {


@AFGhazy It is good to give the user feedback of success/failure of updating requests

TarekAshrafAhmed · 2018-08-01T17:39:32Z

app/src/main/java/com/blink22/android/mvpandroid/activities/BaseActivity.java

+
+        for(NavigationItemEnum item: NavigationItemEnum.values()) {
+            if(item.getId() == menuItem.getItemId()) {
+                Intent i = new Intent(getApplicationContext(), item.getClassToLaunch());


also why getApplicationContext(), this or getActivity in case of fragment should be enough

IslamSalah · 2018-08-04T14:48:22Z

app/src/main/java/com/blink22/android/mvpandroid/newTodo/NewTodoPresenter.java

+    private CompositeDisposable mCompositeDisposable;
+
+    public static NewTodoPresenter getInstance(TodosSubscriber todosSubscriber, NewTodoContract.View view) {
+        return sNewTodoPresenter == null ? new NewTodoPresenter(todosSubscriber, view) : sNewTodoPresenter;


@AFGhazy I believe this line needs a little modification, as sNewTodoPresenter will be null forever. Please let me know if I missed something.

It could be fixed as shown below. but I vote for implementing it as here.

return sNewTodoPresenter == null ? sNewTodoPresenter = new NewTodoPresenter(todosSubscriber, view) : sNewTodoPresenter;

Kindly let me know what you think.

AFGhazy changed the title ~~No dagger2 nor data manager~~ Add main functionality with out dagger2 nor data manager Jul 28, 2018

Add main functionality without dagger2 nor data manager

0b4824b

AFGhazy force-pushed the develop branch from 181bc99 to 0b4824b Compare July 28, 2018 14:19

Support lifecycle aware presenter

a82d7bd

IslamSalah reviewed Aug 1, 2018

View reviewed changes

TarekAshrafAhmed reviewed Aug 1, 2018

View reviewed changes

IslamSalah reviewed Aug 4, 2018

View reviewed changes

Add main functionality with out dagger2 nor data manager #1

Are you sure you want to change the base?

Add main functionality with out dagger2 nor data manager #1

Uh oh!

Conversation

AFGhazy commented Jul 28, 2018

Uh oh!

IslamSalah left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

IslamSalah left a comment •

edited

Loading