Recyclerview Adapter for weather app – Makes API calls for each item

I have a weather app that allows the user to save Cities (with nickname and zip) into a room database. I’m using mvvm and livedata to observe the list of cities and update the recyclerview accordingly. I then perform an API call for each item in the recyclerview, which to me seems wrong. It works but seems wrong.

I’d appreciate any reviews on things I could do to improve.

public class RecyclerAdapter extends ListAdapter<MyCity, RecyclerAdapter.ViewHolder> {

    private static final DiffUtil.ItemCallback<MyCity> DIFF_CALLBACK = new DiffUtil.ItemCallback<MyCity>() {
        @Override
        public boolean areItemsTheSame(@NonNull MyCity oldItem, @NonNull MyCity newItem) {
            return oldItem.getId() == newItem.getId();
        }

        @Override
        public boolean areContentsTheSame(@NonNull MyCity oldItem, @NonNull MyCity newItem) {
            return oldItem.getZipCode() == newItem.getZipCode();
        }
    };

    public RecyclerAdapter() {
        super(DIFF_CALLBACK);
    }

    @NonNull
    @Override
    public ViewHolder onCreateViewHolder(@NonNull ViewGroup parent, int viewType) {
        View view = LayoutInflater.from(parent.getContext()).inflate(R.layout.layout_item_list, parent, false);
        return new ViewHolder(view);
    }

    @Override
    public void onBindViewHolder(@NonNull ViewHolder holder, int position) {

        MyCity city = getItem(position);

        WeatherApi weatherApi = RetrofitService.createService(WeatherApi.class);

        weatherApi.getWeatherWithZip(city.getZipCode(), Constants.API_KEY).enqueue(new Callback<WeatherResponse>() {
            @Override
            public void onResponse(Call<WeatherResponse> call, Response<WeatherResponse> response) {
                if (response.isSuccessful()) {
                    String zip = String.valueOf(city.getZipCode());
                    holder.zip.setText(zip);
                    holder.city.setText(response.body().getName());
                    holder.weather.setText(Conversions.kelvinToFahrenheit(response.body().getMain().getTemp()) + "°C");
                }
            }

            @Override
            public void onFailure(Call<WeatherResponse> call, Throwable t) {

            }
        });
    }

    public MyCity getCityAt(int position) {
        return getItem(position);
    }

    class ViewHolder extends RecyclerView.ViewHolder {

        private TextView zip, city, weather;


        public ViewHolder(@NonNull View itemView) {
            super(itemView);
            zip = itemView.findViewById(R.id.tv_zip);
            city = itemView.findViewById(R.id.tv_city);
            weather = itemView.findViewById(R.id.tv_weather);
        }
    }
}

EDIT:
I just got an email from openweatherapi… I made 11791 calls in a minute. This is definitely not ideal.

First issue

You’re calling the weather API on every OnBindViewHolder(), which means it will get called for each visible row, and on new rows entering, or re-entering the screen. So if you scroll the list a few times you will call the API for each city more than once.

Second issue

Your Adapter shouldn’t make any API calls. In fact, it should only concern itself about displaying cities.

How can we solve this?

Your MyCity model should provide all information necessary to display the weather.

So let’s add city name and temperature to your model.

Next we create a service which iterates over the cities and calls the API once for each city. This can be a regular Android Service for example, or something more advanced like WorkManager.

This logic will be triggered from your ViewModel.

Depending on how frequent you want the weather updates to be, you can call this each time you open the screen, or on certain intervals.

In all cases, once you get the response back, you need to persist the new data to your database.

Since you are observing the data, the adapter will be automatically notified of changes.

Note: when retrieving the cities to call the API you shouldn’t use LiveData because when you update the city it will re-trigger the LiveData and you find yourself in an endless loop.

Your Dao will look something like this:

@Dao
public interface CitiesDao {

    @Query("SELECT * FROM cities")
    List<MyCity> getCities();

    @Query("SELECT * FROM cities")
    LiveData<List<MyCity>> observeCities();

    // etc

}

This is just the starting point.

More things you can consider:

• Use different models for data and UI (e.g. MyCity and MyCityUiModel)
• Use the Repository pattern