Czy to dobry fragment kodu z wykorzystaniem Android MVP?

Uczę się właśnie MVP dla Androida. Zastanawiam się czy to co napłodziłem jest poprawne w kontekście tego wzorca czy jeszcze coś można udoskonalić, a może jest całkowicie do bani:

public interface CredentialsContract {

    interface Model {
        interface OnFinishedLoginListener {
            void onFinishedLogin(String body);
            void onFailureLogin(Throwable t);
        }
        void checkLogin(OnFinishedLoginListener onFinishedListener, String login, String pass);
    }


    interface View {
        void showBigProgressDialog();
        void hideBigProgressDialog();
        void setPropertiesWhenLoginIsTrue();
        void setPropertiesWhenLoginIsFalse();
        void setLoginTextViewText(String response);
    }


    interface Presenter {
        void saveSomeCredentials(String nameLogin, String passLogin);
        void saveEmptyCredentials();
        void checkLoginPresenter(String login, String pass);
    }
}

public class RestData implements CredentialsContract.Model {

    @Override
    public void checkLogin(final OnFinishedLoginListener onFinishedListener, String login, String passWhenLogin) {
        Call<String> result = Api.getClient().checkLogin(login, passWhenLogin);
        result.enqueue(new Callback<String>() {
            @Override
            public void onResponse(Call<String> call, Response<String> response) {
                    onFinishedListener.onFinished(response.body());      
            }

            @Override
            public void onFailure(Call<String> call, Throwable t) {
                Log.e("tag", t.toString());
                onFinishedListener.onFailure(t);
            }
        });
    }
    
}

public class CredentialsPresenter implements CredentialsContract.Presenter, CredentialsContract.Model.OnFinishedLoginListener {

    CredentialsContract.Model model;
    CredentialsContract.View view;
    SharedPreferences sharedPreferences;
    SharedPreferences.Editor editor;

    public CredentialsPresenter(CredentialsContract.View view, SharedPreferences sharedPreferences) {
        model = new RestData();
        this.view = view;
        this.sharedPreferences = sharedPreferences;
        editor = sharedPreferences.edit();
    }

    @Override
    public void saveSomeCredentials(String nameLogin, String passLogin) {
        editor.putString("userLogin", nameLogin);
        editor.putString("userPassword", passLogin);
        editor.putBoolean("cbSaveCredentials", true);
        editor.apply();
    }

    @Override
    public void saveEmptyCredentials() {
        editor.putString("userLogin", "");
        editor.putString("userPassword", "");
        editor.putBoolean("cbSaveCredentials", false);
        editor.apply();
    }

    @Override
    public void checkLoginPresenter(String login, String passWhenLogin) {
        view.showBigProgressDialog();
        model.checkLogin(this, login, passWhenLogin);
    }


    @Override
    public void onFinishedLogin(String body) {
        view.setLoginTextViewText(body);
        if(body.contains("true"))
            view.setPropertiesWhenLoginIsTrue();
        if(body.contains("false"))
            view.setPropertiesWhenLoginIsFalse();
        view.hideBigProgressDialog();
    }

    @Override
    public void onFailureLogin(Throwable t) {
        view.hideBigProgressDialog();
    }

public class Credentials extends AppCompatActivity implements CredentialsContract.View {

    private EditText etNameLogin, etPassLogin;
    private CheckBox cbSaveCredentials, cbTerms;
    private TextView tvLoginStatus, tvRegistrationStatus;
    private ProgressBar pbBig;
    private CredentialsPresenter credentialsPresenter;
    private Button bOk;

    @Override
    protected void onCreate(@Nullable Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.credentials_layout);

        SharedPreferences pref = getApplicationContext().getSharedPreferences("Preferences", 0); // 0 - for private mode
        credentialsPresenter = new CredentialsPresenter(this, pref);

        initUIFields(pref);

        findViewById(R.id.tvTerms).setOnClickListener(v -> initRegulationsDialog());

        bOk = findViewById(R.id.bOk);
        bOk.setOnClickListener(v -> {
            if (cbSaveCredentials.isChecked())
                credentialsPresenter.saveSomeCredentials(etNameLogin.getText().toString(), etPassLogin.getText().toString());
            else
                credentialsPresenter.saveEmptyCredentials();

            Intent i = new Intent(getApplicationContext(), Gra.class);
            i.putExtra("userLogin", etNameLogin.getText().toString());
            i.putExtra("onlineGame", true);
            startActivity(i);
        });

        findViewById(R.id.bLogin).setOnClickListener(v -> credentialsPresenter.checkLoginPresenter(etNameLogin.getText().toString(), etPassLogin.getText().toString()));

    }

    public void initUIFields(SharedPreferences pref) {
        etNameLogin = findViewById(R.id.etNameLogin);
        etNameLogin.setText(pref.getString("userLogin", ""));
        etPassLogin = findViewById(R.id.etPassLogin);
        etPassLogin.setText(pref.getString("userPassword", ""));
        cbSaveCredentials = findViewById(R.id.cbSaveCredentials);
        cbSaveCredentials.setChecked(pref.getBoolean("cbSaveCredentials", false));
        etNameRegistration = findViewById(R.id.etName);
        etEmailRegistration = findViewById(R.id.etEmailRegistration);
        etPassRegistration = findViewById(R.id.etPassRegistration);
        etPassConfirmationRegistration = findViewById(R.id.etPassConfirmationRegistration);
        cbTerms = findViewById(R.id.cbTerms);
        tvLoginStatus = findViewById(R.id.tvLoginStatus);
        tvRegistrationStatus = findViewById(R.id.tvRegistrationStatus);
        pbBig = findViewById(R.id.bigProgressBar);
    }

    public void initRegulationsDialog() {
        AlertDialog.Builder dialogRegulations = new AlertDialog.Builder(Credentials.this);
        dialogRegulations.setTitle("Terms and conditions");
        dialogRegulations.setMessage("Content");
        dialogRegulations.setCancelable(false);
        dialogRegulations.setNeutralButton("OK", (dialog1, which) -> dialog1.dismiss());
        dialogRegulations.show();
    }

    @Override
    public void showBigProgressDialog() {
        pbBig.setVisibility(View.VISIBLE);
    }

    @Override
    public void hideBigProgressDialog() {
        pbBig.setVisibility(View.INVISIBLE);
    }

    @Override
    public void setPropertiesWhenLoginIsTrue() {
        bOk.setEnabled(true);
        tvLoginStatus.setTextColor(ContextCompat.getColor(this, R.color.green));
    }

    @Override
    public void setPropertiesWhenLoginIsFalse() {
        bOk.setEnabled(false);
        tvLoginStatus.setTextColor(ContextCompat.getColor(this, R.color.red));
    }

    @Override
    public void setLoginTextViewText(String response) {
        tvLoginStatus.setText(response);
    }
}

• Ustaw kolokowanie składni dla Javy bo nic nie widać.
• Jeśli to rzeczywiście kod dla Androida to zła kategoria. Powinna być Mobilne. Nie każdy programista Javy zna Androida.

Pierwszy artykuł wypluty przez Googla jasno stawia sprawę, że grzebanie w SharedPreferences powinno być w modelu, a u Ciebie prezenter to robi.

Dlaczego nie mogę edytować treści tematu?

"Dzwonią dzwony, ale nie wiadomo w którym kościele" - będę mieć temat na oku. Jeśli ktoś wcześniej nie wrzuci dobrych podpowiedzi podzielę się nimi z Tobą bez zbędnej zwłoki - w tym tygodniu.

lubie_programowac napisał(a):

"Dzwonią dzwony, ale nie wiadomo w którym kościele" - będę mieć temat na oku. Jeśli ktoś wcześniej nie wrzuci dobrych podpowiedzi podzielę się nimi z Tobą bez zbędnej zwłoki - w tym tygodniu.

Dużo jest źle?

1. W MVP Model, View oraz Presenter powinny być agnostyczne. Nie powinieneś mieć tam referencji do androida. U Ciebie jest to zrealizowane połowicznie: nie masz referencji do klas androidowych ale w nazwach metod już tak: (dodatkowe pytanie dlaczego jest big w nazwie? Moze byc jakis small? Jesli nawet tak to te metody powinny miec odzwierciedlenie w domenie a nie w wielkosci czyli np full screen progress albo header progress)

void showBigProgressDialog(); -> void showProgress()
void hideBigProgressDialog(); -> void hideProgress()

1. Polecam zapoznać się z architekturą VIPER która jest używana na iOSie ale robi dobrą robotę na androidzie VIPER ~= mVP + clean architecture + routing. m z małem po model wtedy składa się z usecasów. Pownieneś mieć ~LoginUseCase który w konstruktorze przyjmowałby ~LoginService oraz TokenService. W LoginService miałbyś zawarty LoginRepository który pobierałby dane z internetu oraz je parsował. W prezenterze nie powinno być referencji do androida.

2. Dodaj do projektu ViewBinding - pozbędziesz się masy kodu z findviewbyid.

3. W androidzie nie stosuje się notacji węgierskiej -> private EditText etNameLogin, etPassLogin; -> loginName, loginPassword, private ProgressBar pbBig; -> progress

4. Widok powinien być pasywny, najlepiej gdybyś kolor przesyłał już w prezentera a nie decydował w View czy error to red albo brown a loginSuccess to green a może magenta.

5. 0 for private mode == Context.MODE_PRIVATE

6. W widoku nie powinieneś sprawdzać co się dzieje w sharedpreferences, jest to odpowiedzialność prezentera ( a w tym use casów) do obsługi logiki biznesowej i poinformowaniu widoku jak ma się zachować. Tak samo dla cbSaveCredentials.isChecked()

7. Dodaj statyczne finalne klasy do stringów bo teraz np "userLogin" masz w dwóch miejscach.

8. Obsługa "userLogin" i rodziny pewnie powinna być w klasie ~ UserRepository z implementacją lokalną która byłaby używana przez wspomniany LoginService oraz wyżej LoginUsecase

9. Wprowadzenie daggera / innego frameworku do DI (toothpick, Hilt teraz popularny jest) jest dobrym pomysłem

10. Mógłbyś dodać prostą imeplementację dla rxjavy, ona dobrze się spina z Single<T> z retrofitem oraz ~VIPERem. Korzystałem polecam.

11. Oczywiście kotlin oraz korutyny też są warte rozważenia.

Tutaj masz dobry widok jak wygląda VIPER https://cdn.ttgtmedia.com/rms/onlineimages/whatis-viper_desktop.png

Ogólnie kod nie jest zły. Kontrakt MVP mi się podoba bo jest angostyczny - bez referencji androida. Reszta to de facto kilka spraw + jakiś cukier składniowy do ogarnięcia w kilka h.

Jakbyś miał jakieś pytania pisz, odpowiem w wolnej chwili.

@lubie_programowac: dzięki za sugestie. Strasznie to wszystko skompliwane i dużo tego ;/ odnośnie 1, tak, występuje progressbar niemal na pół ekranu :) odnośnie punktu 7, Kolega wyżej pisał, że jest to odpowiedzialność modelu, tak sam na tej stronie którą załączył Ty piszesz że jednak prezentera.

Kubaz napisał(a):

@lubie_programowac: dzięki za sugestie. Strasznie to wszystko skompliwane i dużo tego ;/ odnośnie 1, tak, występuje progressbar niemal na pół ekranu :) odnośnie punktu 7, Kolega wyżej pisał, że jest to odpowiedzialność modelu, tak sam na tej stronie którą załączył Ty piszesz że jednak prezentera.

My z @cs mówimy o tym samym. Według @cs shared preferences powinno być w modelu - ja się z tym zgadzam. Dla mnie model to Interactors / Usecases + services + repositories + mappers + entities. W punkcie 7 napisałem: jest to odpowiedzialność prezentera ( a w tym use casów) do obsługi logiki biznesowej i poinformowaniu widoku jak ma się zachować . Prezenter powinien gadać z usecasami które w środku gdzieś tam powinny mieć sharedpreferences. Po pobraniu danych z usecasow powinien on przekazac je do widoku.

https://dev.to/wahibhaq/a-brief-summary-of-thoughts-on-clean-architecture-and-mvp-48h9
https://res.cloudinary.com/practicaldev/image/fetch/s--ahOh6vkO--/c_limit%2Cf_auto%2Cfl_progressive%2Cq_auto%2Cw_880/http://wahibhaq.github.io/img/blog/posts/summary-thoughts-clean-architecture-mvp/srp-clean-architecture-diagram.png

@lubie_programowac: Skoro SharedPrefs powinno być w modelu, wtedy będę potrzebował context w modelu, a skoro tak, to przy tworzeniu nowego obiektu modelu w konstruktorze presentera też będę potrzebował context, bo ten presenter korzysta z modelu. A przecież presenter nie powinien być platform dependent jak więc ten problem rozwiązać?

Zastosowałem taki myk jak sugeruje gościu w odpowiedzi: https://stackoverflow.com/questions/39100105/need-context-in-model-in-mvp.

W dużym uproszczeniu, prezenter wie co należy zrobić, ale nie wie jak i gdzie. Od tego ma właśnie widok i model. Przy tworzeniu modelu możesz przekazać do niego context w konstruktorze, tak jak i przy tworzeniu prezentera powinieneś podać w konstruktorze z jakiego modelu on korzysta, a u Ciebie prezenter sam sobie tworzy model i dlatego musisz w prezenterze dostać się do context'u. Tworzenie modelu powinno być w onCreate, potem utworzenie prezentera:

  Model model = new RestModel(prefs);
  Presenter presenter = new CredentialsPresenter(this, model);

Oczywiście to wymaga przemeblowania interfejsu modelu, żeby zawierał metody do zapis, odczytu itd.

lubie_programowac napisał(a):

1. Widok powinien być pasywny, najlepiej gdybyś kolor przesyłał już w prezentera a nie decydował w View czy error to red albo brown a loginSuccess to green a może magenta.

Chyba jedyny punkt, z którym się nie zgodzę. O takich detalach lepiej kiedy decyduje widok. Jeśli zmienię sposób reprezentacji danych, to będę musiał orać w prezenterze. Osobiście wolę takie rzeczy załatwiać zwykłą flagą, interfejsami, enumami albo sealed class. Chociaż ciężko jest czasem znaleźć balans.

Jestem jeszcze sceptycznie nastawiony do use-case-manii, ale to grubszy temat, bo nie mam nic przeciwko use-casom jako tako, tylko często robi się przez to degeneracja projektu z wstrzykiwaniem 20 use-casów do klas. Ale jakiś podstawowy klocek realizujący logikę biznesową jest potrzebny.

To jak jesteśmy już przy clean codzie w Presenterach, to ja nie przepadam za tym patternem. Powoduje konieczność tworzenia interfejsów, które najczęściej mają tylko 1 implementację. Jak dla mnie to zbędny boiltercode. Nie widziałem by ktoś unitowo testował zmianę tekstu czy kolorów w UI. Dodatkowo Presenter ma niepotrzebnie referencję do widoku. Dlatego wolę ViewModel i przekazywanie do widoku danych przez LiveData, gdzie ViewModel nigdy nie ma referencji do widoku. Widok ma referencję do ViewModelu, ale stara się być tak bardzo "głupi" jak tylko się da.

Mój prezenter zazwyczaj wygląda tak. Najczęściej raczej implementuje jakiś interfejs, ale to dla polimorfizmu a nie dla ukrycia implementacji.

class Presenter {
  fun emitUiModels(events: Flow<Event>): Flow<UiModel> = TODO()
}

ViewModel to raczej kwestia nomenklatury, którą Google zaczął promować wraz z pracą nad Jetpackiem. Yigit z resztą gdzieś na Reddicie pisał, że niefortunnie dobrali nazwę. Już na długo przed ich bibliotekami robiło się obserwowanie bez referencji do widoku za pomocą callbacków, event busów czy RxJavy. https://cashapp.github.io/2020-06-09/android-presenters