1
\$\begingroup\$

Inspired this guide: https://developer.android.com/jetpack/docs/guide enter image description here 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.

\$\endgroup\$
1
\$\begingroup\$
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!

\$\endgroup\$
  • \$\begingroup\$ Thanks for great answer! Can you check, how I organize MVVM and repository patterns? \$\endgroup\$ – igor_rb Nov 4 '18 at 8:51
  • \$\begingroup\$ Yes split files into different folders for models, controllers, etc. \$\endgroup\$ – Faraz Nov 4 '18 at 23:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.