Conversation
| import java.util.List; | ||
|
|
||
| public interface ListWeatherView { | ||
| void showData(List<ListWeather> listWeathers, MainWeather mainWeather); |
There was a problem hiding this comment.
Я бы разделил этот метод на два. Это тебе даст больше контроля над View. Если тебе придется обновить что-то одно, то ты, собственно, вызовешь конкретный метод
| public void glide(MainWeather mainWeather) { | ||
| Glide | ||
| .with(MainActivity.this) | ||
| .load(String.format(imageUri, mainWeather.getListWeathers().get(0).getWeather().get(0).getIcon()).toLowerCase()) |
There was a problem hiding this comment.
Url для картинки должен быть уже подготовлен к тому моменту как MainWeather попадет в активити. То есть отформатировать и подготовить этот URL надо заранее
| super.onDestroy(); | ||
| } | ||
|
|
||
| public void glide(MainWeather mainWeather) { |
There was a problem hiding this comment.
метод имеет неинформативное имя. Надо дать более осмысленное. Также не думаю, что метод должен публичным, потому что он используется только внутри конкретной активити
| @Override | ||
| public void showData(List<ListWeather> listWeathers, MainWeather mainWeather) { | ||
| if (!temperatureCheck) { | ||
| T = " C"; |
There was a problem hiding this comment.
подготовка контента должна просиходить на уровне презентера, а не в самой активити. Активити только показывает то, чт оей дают
| if (!temperatureCheck) { | ||
| T = " C"; | ||
| temperature = mainWeather.getListWeathers().get(0).getMain().getTemp(); | ||
| temperatureTextView.setText(String.valueOf(temperature) + " C"); |
There was a problem hiding this comment.
подготовка контента должна просиходить на уровне презентера, а не в самой активити. Активити только показывает то, чт оей дают
| adapter.setListWeathers(mainWeather.getListWeathers(), T); | ||
| glide(mainWeather); | ||
| } else { | ||
| T = " F"; |
There was a problem hiding this comment.
подготовка контента должна просиходить на уровне презентера, а не в самой активити. Активити только показывает то, чт оей дают
| } else { | ||
| T = " F"; | ||
| temperature = mainWeather.getListWeathers().get(0).getMain().getTemp(); | ||
| temperatureTextView.setText(String.valueOf(temperature) + " F"); |
There was a problem hiding this comment.
подготовка контента должна просиходить на уровне презентера, а не в самой активити. Активити только показывает то, чт оей дают
|
|
||
| @Override | ||
| public void showError() { | ||
| Toast.makeText(this, "Ошибка", Toast.LENGTH_SHORT).show(); |
There was a problem hiding this comment.
Лучше используй Snackbar. Тост уже довольно устаревший компонент
|
|
||
| public class MainActivityPresenter { | ||
| private CompositeDisposable compositeDisposable; | ||
| ListWeatherView view; |
| public void showInfoCelsium() { | ||
| ApiFactory apiFactory = ApiFactory.getInstance(); | ||
| ApiService apiService = apiFactory.getApiService(); | ||
| compositeDisposable = new CompositeDisposable(); |
There was a problem hiding this comment.
CompositeDisposable не нужно постоянно пересоздавать. Он создается только один раз. Инициализируй его при создании презентера
| }); | ||
| compositeDisposable.add(disposable); | ||
| } | ||
|
|
There was a problem hiding this comment.
У тебя есть два метода: showInfoCelsium и showInfoFarenhate. В них есть одинаковые части кода. Их можно выделить в отдельные методы. Также ты можешь сделать конзюмеры для подписки и ошибки полями класса и просто передавать их в сабскрайб, чтобы постоянно не копипастить этот код
| }); | ||
| } | ||
|
|
||
| void saveBoolean(boolean temperatureCheck) { |
There was a problem hiding this comment.
дай более информативное имя методу
| ed.apply(); | ||
| } | ||
|
|
||
| public boolean loadBoolean() { |
There was a problem hiding this comment.
дай более информативное имя методу
| public void onCheckedChanged(CompoundButton buttonView, boolean isChecked) { | ||
| if (isChecked) { | ||
| temperatureCheck = true; | ||
| Toast.makeText(SettingsActivity.this, "Включен режим Фаренгейт", Toast.LENGTH_SHORT).show(); |
| saveBoolean(temperatureCheck); | ||
| } else { | ||
| temperatureCheck = false; | ||
| Toast.makeText(SettingsActivity.this, "Включен режим Цельсий", Toast.LENGTH_SHORT).show(); |
| public class WeatherAdapter extends RecyclerView.Adapter<WeatherAdapter.WeatherViewHolder> { | ||
|
|
||
| List<ListWeather> listWeathers; | ||
| MainActivity mainActivity; |
There was a problem hiding this comment.
Ссфлки на активити в адаптере быть не должно. Контекст ты можешь получить из itemView твоего ViewHolder
| weatherViewHolder.textViewTime.setText(listWeather.getDtTxt()); | ||
| Glide | ||
| .with(mainActivity) | ||
| .load(String.format(imageUri, listWeather.getWeather().get(0).getIcon()).toLowerCase()) |
There was a problem hiding this comment.
Url для картинки должен быть уже подготовлен к тому моменту как MainWeather попадет в активити. То есть отформатировать и подготовить этот URL надо заранее. Адаптер просто учавствутет в процессе отображения контента, по сути он тоже часть view. Поэтому он должен просто показывать данные
|
|
||
| public class WeatherAdapter extends RecyclerView.Adapter<WeatherAdapter.WeatherViewHolder> { | ||
|
|
||
| List<ListWeather> listWeathers; |
| weatherViewHolder.temperatureTextView.setText(String.valueOf(listWeather.getMain().getTemp()) + T); | ||
| weatherViewHolder.textViewDescription.setText(String.valueOf(listWeather.getWeather().get(0).getDescription())); |
There was a problem hiding this comment.
подготовка контента должна просиходить на уровне презентера, а не в самой активити. Активити только показывает то, чт оей дают
| public interface ApiService { | ||
|
|
||
| @GET("forecast?q=Minsk&units=metric&cnt=25&appid=60ac937a95b7355dbf856446fad8af84") | ||
| Observable<MainWeather> getMainWeatherCels(); |
There was a problem hiding this comment.
Я бы тут использовал Single. Он больше подходит. По сути ты каждый раз делаешь делаешь новый запрос и генеришь новый Observable, поэтому он тут бесполезен
| Observable<MainWeather> getMainWeatherCels(); | ||
|
|
||
| @GET("forecast?q=Minsk&units=imperial&cnt=25&appid=60ac937a95b7355dbf856446fad8af84") | ||
| Observable<MainWeather> getMainWeatherFarenhate(); |
There was a problem hiding this comment.
Я бы тут использовал Single. Он больше подходит. По сути ты каждый раз делаешь делаешь новый запрос и генеришь новый Observable, поэтому он тут бесполезен
|
|
||
| public interface ApiService { | ||
|
|
||
| @GET("forecast?q=Minsk&units=metric&cnt=25&appid=60ac937a95b7355dbf856446fad8af84") |
There was a problem hiding this comment.
appid можно вынести в константу прямо в этом интерфейсе
| @GET("forecast?q=Minsk&units=metric&cnt=25&appid=60ac937a95b7355dbf856446fad8af84") | ||
| Observable<MainWeather> getMainWeatherCels(); | ||
|
|
||
| @GET("forecast?q=Minsk&units=imperial&cnt=25&appid=60ac937a95b7355dbf856446fad8af84") |
There was a problem hiding this comment.
appid можно вынести в константу прямо в этом интерфейсе
|
|
||
|
|
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class MainActivity extends AppCompatActivity { |
There was a problem hiding this comment.
Те же замечания, что и для mvp-реализации
2 participants
No description provided.