Android MVVM application architecture

Question

Inspired this guide: https://developer.android.com/jetpack/docs/guide I try to build some app with similar architecture.

App interact with remote API via http protocol (json responses from server). For example, GET /bonuses/:id return {status: true, bonus: {id: 1, name: "test bonus"}}

POST /login can return {status: true, token: "dafadafadfadfadf"} or {status: false, message: "Password or phone incorrect"}

For example, I post here my LoginFragment, LoginViewModel, UserRepository and RemoteDataSource.

Does my MVVM approach good? What is very bad in my code and how I can improve bad places? Possible memory leaks, etc...

One bad thing, what I see - repeating code in UserRepository, how I can improve it?

Other issue - naming some instance variables with m and other - without (I fix it later).

What is good in my code?

public class LoginFragment extends Fragment implements View.OnClickListener {

public static String TAG = "LoginFragment";

private NavController mNavController;

private ApiService mApiService;
@BindView(R.id.btn_login)
Button mBtnLogin;
@BindView(R.id.et_phone)
EditText mEtPhone;
@BindView(R.id.et_password)
EditText mEtPassword;

@BindView(R.id.tv_forgot_password)
TextView mTvForgotPassword;

@BindView(R.id.tv_registration)
TextView mTvRegistration;

MainActivity mActivity;

private LoginViewModel mLoginViewModel;


public LoginFragment() {
    // Required empty public constructor
}


public static LoginFragment newInstance(String param1, String param2) {
    LoginFragment fragment = new LoginFragment();
    Bundle args = new Bundle();
    fragment.setArguments(args);

    return fragment;
}

@Override
public void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);

}

@Override
public View onCreateView(LayoutInflater inflater, ViewGroup container,
                         Bundle savedInstanceState) {
    // Inflate the layout for this fragment
    View v = inflater.inflate(R.layout.fragment_login, container, false);
    ButterKnife.bind(this, v);
    mActivity = (MainActivity) getActivity();

    mNavController = mActivity.getNavController();

    mActivity.getSupportActionBar().setTitle(getString(R.string.login));
    mActivity.getSupportActionBar().setDisplayHomeAsUpEnabled(true);
    mActivity.getSupportActionBar().setDisplayUseLogoEnabled(false);

    mApiService = RetrofitClient.getInstance().getApiService();

    mBtnLogin.setOnClickListener(this);
    mTvForgotPassword.setOnClickListener(this);
    mTvRegistration.setOnClickListener(this);


    return v;
}

@Override
public void onActivityCreated(@Nullable Bundle savedInstanceState) {
    super.onActivityCreated(savedInstanceState);
    super.onActivityCreated(savedInstanceState);
    mLoginViewModel = ViewModelProviders.of(this).get(LoginViewModel.class);
}

@Override
public void onClick(View v) {
    switch (v.getId()) {

        case R.id.btn_login: {
            mLoginViewModel.init(mEtPhone.getText().toString().replaceAll("\\D+", ""), mEtPassword.getText().toString());
            mLoginViewModel.getNetworkState().observe(this, new Observer<Integer>() {
                @Override
                public void onChanged(@Nullable Integer state) {
                    if(state == NetworkState.LOADED) {
                        Boolean status = mLoginViewModel.getRemoteDataSource().getStatus();
                        if(status) {
                            SharedPreferences preferences = PreferenceManager.getDefaultSharedPreferences(getActivity());
                            SharedPreferences.Editor editor = preferences.edit();
                            editor.putString(User.TOKEN_NAME, mLoginViewModel.getToken());
                            editor.apply();
                            mNavController.navigate(R.id.action_loginFragment_to_bonusesFragment);

                        } else {
                            AlertDialogHelper.showDialog(mActivity, getString(R.string.warning), mLoginViewModel.getRemoteDataSource().getMessage());
                        }
                    } else if(state == NetworkState.FAILED) {
                        AlertDialogHelper.showDialog(mActivity, getString(R.string.warning), mLoginViewModel.getRemoteDataSource().getMessage());
                    }
                }
            });
            break;

        }
        case R.id.tv_registration: {
            mNavController.navigate(R.id.action_loginFragment_to_registrationFragment);
            break;

        }
        case R.id.tv_forgot_password: {
            mNavController.navigate(R.id.action_loginFragment_to_restorePasswordFragment);
            break;
        }

    }

}
}

LoginViewModel:

public class LoginViewModel extends ViewModel {

private UserRepository mUserRepository;

private RemoteDataSource<String> mLogin;
private String mPhone;
private String mPassword;

public LoginViewModel() {
}

public void init(String phone, String password) {
    mUserRepository = new UserRepository();
    mPassword = password;
    mPhone = phone;
    mLogin = mUserRepository.login(phone, password);
}

public LiveData<Integer> getNetworkState() {
    return mLogin.getNetworkState();
}
public Boolean getStatus() {
    return mLogin.getStatus();
}
public String getToken() {
    return mLogin.getData();
}

public RemoteDataSource<String> getRemoteDataSource() {
    return mLogin;
}

}

UserRepository:

public class UserRepository {

    public static String TAG = "UserRepository";

    ApiService mApiService;

    SharedPreferences mPrefs;
    Context mContext;

    RemoteDataSource<User> mUserInfo;
    RemoteDataSource<String> mUserLogin;
    RemoteDataSource<String> mUserSendCode;

    public UserRepository() {

        mApiService = new RetrofitClient().getApiService();
        mContext = App.getAppContext();
        mPrefs = PreferenceManager.getDefaultSharedPreferences(mContext);
        mUserInfo = new RemoteDataSource<>();
        mUserLogin = new RemoteDataSource<>();
        mUserSendCode = new RemoteDataSource<>();
    }

    public RemoteDataSource getUserInfo() {
        mUserInfo.setIsLoading();
        Call<ApiResponse> userCall = mApiService.getUserInfo(mPrefs.getString(User.TOKEN_NAME, null));
        userCall.enqueue(new Callback<ApiResponse>() {
            @Override
            public void onResponse(Call<ApiResponse> call, Response<ApiResponse> response) {
                mUserInfo.setIsLoaded(response.body().getUser());
                mUserInfo.setStatus(response.body().getStatus());
                mUserInfo.setMessage(response.body().getMessage());
            }

            @Override
            public void onFailure(Call<ApiResponse> call, Throwable t) {
                Log.e(TAG, t.getMessage());
                mUserInfo.setFailed(t.getMessage());
            }
        });

    return mUserInfo;

    }


    public RemoteDataSource login(String phone, String password) {
        mUserLogin.setIsLoading();
        Call<ApiResponse> loginCall = mApiService.login(phone, password);
        loginCall.enqueue(new Callback<ApiResponse>() {
            @Override
            public void onResponse(Call<ApiResponse> call, Response<ApiResponse> response) {
                mUserLogin.setIsLoaded(response.body().getToken());
                mUserLogin.setStatus(response.body().getStatus());
                mUserLogin.setMessage(response.body().getMessage());
            }

            @Override
            public void onFailure(Call<ApiResponse> call, Throwable t) {
                Log.e(TAG, t.getMessage());
                mUserLogin.setFailed(t.getMessage());
            }
        });
        return mUserLogin;
    }

    public RemoteDataSource sendCode(String phone) {
        mUserSendCode.setIsLoading();
        Call<ApiResponse> sendCodeCall = mApiService.sendCode(phone);
        sendCodeCall.enqueue(new Callback<ApiResponse>() {
            @Override
            public void onResponse(Call<ApiResponse> call, Response<ApiResponse> response) {
                mUserSendCode.setIsLoaded(response.body().getToken());
                mUserSendCode.setStatus(response.body().getStatus());
                mUserSendCode.setMessage(response.body().getMessage());
            }

            @Override
            public void onFailure(Call<ApiResponse> call, Throwable t) {
                Log.e(TAG, t.getMessage());
                mUserLogin.setFailed(t.getMessage());
            }
        });

        return mUserSendCode;
    }
}

and RemoteDataSource:

public class RemoteDataSource<T> {
private final MutableLiveData<Integer> networkState = new MutableLiveData<>();
private T  data;
private String message;
private Boolean status;
private String code;


public RemoteDataSource() {
    networkState.postValue(NetworkState.DEFAULT);
}

public MutableLiveData<Integer> getNetworkState() {
    return networkState;
}

public void setIsLoading() {
    networkState.postValue(NetworkState.LOADING);
}

public void setDefault() {
    networkState.postValue(NetworkState.DEFAULT);
}

public void setIsLoaded(T data) {
    networkState.postValue(NetworkState.LOADED);
    this.data = data;
}

public void setFailed(@NonNull String errorMessage) {
    this.message = errorMessage;
    networkState.postValue(NetworkState.FAILED);
}

public void setMessage(String message) {
    this.message = message;
}

public String getMessage() {
    return message;
}

public Boolean getStatus() {
    return status;
}

public void setStatus(Boolean status) {
    this.status = status;
}

public T getData() {
    return data;
}

public String getCode() { return code;}
public void setCode(String code) {this.code = code;}
}

User - is simple pojo model:

public class User {
public static String TOKEN_NAME="token";

private String token;
private Integer bonus;
private String name;
private String email;

private String phone;

public Integer getBonus() { return bonus;}
public void setBonus(Integer bonus) {this.bonus = bonus;}

public String getToken() {
    return token;
}

public void setToken(String token) {
    this.token = token;
}

public String getName() {
    return name;
}
public String getPhone() {
    return phone;
}
public String getEmail() {
    return email;
}

}

BonusRepository, BonusVieModel, Bonus model and BonusFragment looks like the very similar on upper code.

UPD: it will be great, if you answer cover, how I organize repository/mvvm patterns.

Answer 1

LoginFragment: Should only have the responsibility of a View to show the data or navigation to other screen by action received by viewmodel.

So any data storing or network related access shouldn't be here,

mApiService = RetrofitClient.getInstance().getApiService();

UserRepository: The following storing data task in LoginFragment can be done in UserRepository itself as this responsibility lies within that class. On success of the api call you can directly store that in SharedPreferences.

SharedPreferences preferences = PreferenceManager.getDefaultSharedPreferences(getActivity());
SharedPreferences.Editor editor = preferences.edit();
editor.putString(User.TOKEN_NAME, <token>);
editor.apply();

LoginViewModel: Rather than observing network state after doing login, just observe the ui state from the viewmodel in onViewCreated(). Why does view needs to bother about network state or other state? view just need to do the things which viewmodel asks it to. Doing navigation or showing error based on network or any logic is to be determined by the viewmodel.

Viewmodel can create ui states like,

abstract class UIState

ErrorState(String Message) extends UIState
NavigationState(String screenName) extends UIState

View can just subscribe to viewmodel like,:

viewmodel.subscribe().observe(this, new Observer<UIState>() {
                @Override
                public void onChanged(@Nullable UIState state) {
                   if(state instanceOf ErrorState)
                   {
                     showError(state.message);
                   }
                   else if(state instanceOf NavigationState)
                   {
                     navigateTo(state.screenName); //Dont send R.id values from viewmodel as it shouldn't have android related classes which will make testing(junit) hard. Map your R.id based on screenName using switch/if-else and pass it to mNavController 
                   }
                }
            });

Answer 2

if(state == NetworkState.LOADED) {
                        Boolean status = mLoginViewModel.getRemoteDataSource().getStatus();

Instead of having two separate if statement checks, this can be combined to check the NetworkState as well as call and check getStatus(), checking both conditions with an AND (&&) operator.

Be careful regarding white-space. Sometimes I see random newlines in your code. For the sake of readability -- it is beneficial to add newlines to separate parts of your code. But, use them sparingly, don't put newlines in random places where they don't belong, as that breaks the structural uniformity of your code.

Stick to one style for single line method bodies. In some places, you are doing this:

public String getToken() {
    return token;
}

and in others, doing this:

public Integer getBonus() { return bonus;}

personally, I and majority of Java developers would recommend the first style. But whatever you decide on going with, sticking to one particular style is the most important thing.

When you are writing a larger codebase, it's immensely important to keep structure in your code. Otherwise, reading over parts of the code become more difficult.

Please let me know if you have any other questions, or would like more help understanding any part of my answer. Thank you & welcome to the CoderReview community!