Android application architecture am i doing this correctly?

Question

I'm following this guide here https://developer.android.com/jetpack/docs/guide?hl=en

In an attempt to learn separation of concern I created a simple application that gets some data from an API and displays it on screen in an activity.

My activity is the following its responsibility is to display information to the user.

public class MainActivity extends AppCompatActivity implements View.OnClickListener {
    private Button myButt;
    private MainViewModel mvw;
    private TextView myView;
    private MutableLiveData<Orders> mld;


    @Override
    protected void onCreate(Bundle savedInstanceState) {
        mvw = new ViewModelProvider(this).get(MainViewModel.class);

        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);
        myButt = findViewById(R.id.button);
        myView = findViewById(R.id.textview);
        myButt.setOnClickListener(this);

        mvw.getMld().observe(this, new Observer<Orders>() {
            @Override
            public void onChanged(Orders orders) {
                Log.i("livedata","got an updt");
                myView.setText(mvw.extractDate(orders));
            }
        });
    }

    @Override
    public void onClick(View view) {
        switch(view.getId()){
            case R.id.button:
                Log.i("tag","hello");
                mvw.updateData();
        }
    }
}

Below follow the ViewModel its concern is getting data from a repository class and do some computation on that data before displaying it in the activity.

public class MainViewModel extends ViewModel {
    private GetOrder getOrderRepo;
    private MutableLiveData<Orders> mld;

    public MainViewModel(){
        getOrderRepo = new GetOrder();
        mld = getOrderRepo.getAllOrders();
    }

    public MutableLiveData<Orders> getMld() {
        return mld;
    }

    public void setMld(MutableLiveData<Orders> mld) {
        this.mld = mld;
    }





    public void updateData(){
        getOrderRepo.getAllOrders(); //discard the return value
    }

    public String extractDate(Orders orders){
        ArrayList<Order> listOfOrders = orders.getOrders();
        String date = listOfOrders.get(0).getOrderTime();
        return date;
    }

}

Next is the repository it handles the GET request from an API and puts it into the MutableLiveData container "allOrders"

public class GetOrder {
    private ApiService mAPIService;
    MutableLiveData<Orders> allOrders;
    private Orders orders;
    public GetOrder(){
        mAPIService = ApiUtils.getAPIService();
    }
    public MutableLiveData<Orders> getAllOrders(){
        Log.i("func","starting func");
        allOrders = new MutableLiveData<Orders>();
        mAPIService.getOrders().subscribeOn(Schedulers.io()).observeOn(AndroidSchedulers.mainThread())
                .subscribe(new Subscriber<Orders>() {
                    @Override
                    public void onCompleted() {
                        Log.i("func","onComplete");

                    }

                    @Override
                    public void onError(Throwable e) {
                        Log.i("onError",e.toString());

                    }

                    @Override
                    public void onNext(Orders orders) {
                        Log.i("Repo",orders.toString());
                        allOrders.setValue(orders);
                    }
                });
        return allOrders;
    }
}

Is this a correct implementation? Or have I misunderstood something?

One concern I have is with the button having the mvw.orderData().observe(this, new Observer() since this creates a new observer every time. Or does it die after each onChanged?

Updated after feedback

Answer 1

What you are implementing here is MVVM architecture. Model(repository)-View (Activity)-VM(Viewmodel). The basic structure looks alright but I'm seeing a few things you can improve on to better follow best practices.

1. As you said, you are creating a new Observer for every Button click. You should observe the LiveData in onCreate instead. Add a function in your ViewModel that's telling the repository to update data and call that in onClick method.
2. You should implement MutableLiveData as private and wrap it in a LiveData that you then observe in the Activity. So it's safe from mutation. Also the ViewModel should store the fetched MutableLiveData.

Update (2): Check MainActivityViewModel under this link. The MutableLiveData, which you set in your ViewModel shoud be private.

 private MutableLiveData<List<String>> fruitList;

That MutableLiveData is then returned via normal LiveData. This LiveData is not mutable and can not be null and therefor safe to expose to other classes like your MainActivity where you can observe it.

LiveData<List<String>> getFruitList() {
        if (fruitList == null) {
            fruitList = new MutableLiveData<>();
            loadFruits();
        }
        return fruitList;
    } 

loadFruits() function would be the api call from the repository in your case.

3. You should instantiate the ViewModel using ViewModelProvider and/or ViewModelFactory. Here is a more detailed explanation. It should look something like this:

   private val viewModel = ViewModelProvider(this).get(MainViewModel::class.java)

It is now Lifecycle aware. You can inject that same instance in a fragment later if you like.

4. Just for fun: I like how your button variable is called myButt :D

Answer 2

ViewModel should be in charge for storing data.

So, the better approach would be having MutableLiveData<Orders> allOrders; in ViewModel instead of Repository. That way Repository is only responsible for getting data. It should not store it.

Then, in your Activity you should observe allOrders live data like so: mvw.allOrders.observe(...) in onCreate().

And on button click you just call Repository method to get/update allOrders LiveData using your ViewModel's method orderData().