Code review #5
Code review #5
Conversation
|
|
||
| // TODO handle this exceptions sensibly! | ||
| try { | ||
| Controller.createDatabases(this); |
There was a problem hiding this comment.
Вот тут начинаются почти все проблемы. Статические поля повсюду. В какой то момент поддерживать это станет невозможно, а читать крайне сложно уже сейчас. Попробую обьяснить на данном конкретном примере.
Ваше приложение не будет работать, если оно в какой то момент будет сериализовано и убито андроидом из за недостатка ресурсов и потом восстановлено (но не в мейн активити). Тогда коннекшен к базе не будет запрашиваться вообще.
Второй момент - создание активити из стороннего приложения. Допустим гугл решит интегрироваться с вашим приложением и сделает кнопку добавления домашек из почты. В коде это будет выглядеть как startIntention("AddHomeworkActivity", {"Subject" -> "Maths, "Deadline" -> "10.10.2010", ... }). Тут опять получиться путь, который минует MainActivity вообще.
Это только андроид специфичные случаи. Помимо этого способов выстрелить себе в ногу через глобальные константы миллион.
В целом рекомендация такая - удалить все статические поля, кроме final констант.
|
|
||
| outputCurrentDate(); | ||
| outputDeadlines(); | ||
| outputTodaySchedule(); |
There was a problem hiding this comment.
Вообще хорошо, что вы выделили отдельные шаги в методы, но тут это вольно сильно путает. Например:
db = initDb()
deadlines = db.readDeadlines()
console.print(deadline)
Очень понятный вокфлоу. В вашем случае - без захода в функцию крайне сложно судить - какие данные берутся, откуда и куда выводятся. А сами функции делают сразу три вещи - получение данных, формирование всяких красивых строчек и биндинг с ui.
| try { | ||
| Controller.createDatabases(this); | ||
| } catch(Exception exception) { | ||
|
|
There was a problem hiding this comment.
Бог с ней с обработкой ошибок. Но даже в процессе разработки - просто залогируйте ошибку. Сильно упростит дебаг.
| } | ||
|
|
||
| @Override | ||
| protected void onActivityResult(int requestCode, int resultCode, Intent data) { |
There was a problem hiding this comment.
Вам вообще не нужно делать startActivityForResult. Вы результат не используете.
outputDeadlines();
outputTodaySchedule();
Явно должны переехать в onResume.
| public class AddHomeworkActivity extends AppCompatActivity { | ||
|
|
||
| private static final String TAG = "AddHomeworkActivity"; | ||
| public static final HomeworkFieldsAccumulator HFA = new HomeworkFieldsAccumulator(); |
There was a problem hiding this comment.
Статический класс в статическое поле.
| private int day; | ||
| private int hour; | ||
| private int minutes; | ||
| private double expectedScore = Double.MIN_VALUE; |
There was a problem hiding this comment.
year, month и day - должны быть представлены одной структурой данный, они выставляются за раз.
hour, minutes - та же история.
При передаче дальше можно сгрупировать и их во что то вроде DateTime
| if (howToSend == null) { | ||
| howToSend = " "; | ||
| } | ||
|
|
There was a problem hiding this comment.
Инкапсулировать сюда базу данных точно очень плохая идея.
Получается методы set* - читают поля и пишут в структурку, а add - пишет в базу данных. Это невозможно предугадать по названию
| subject, regularity, description, | ||
| howToSend, expectedScore, files); | ||
| } | ||
|
|
There was a problem hiding this comment.
Те же самые проблемы. Непонятно какие данные и откуда берутся, куда пишутся. Распилить на два. get и write. Может даже три (что то вроде updateMissing). Совершенно непонятно, почему на isSet проверяются только дата и время? А что остальные?
| return subject.trim().length() > 0; | ||
| } | ||
|
|
||
| public void clear() { |
There was a problem hiding this comment.
После рефакторинга выше этот метод уходит совсем.
|
|
||
| public void editHomework( int id ) { | ||
| if (description == null) { | ||
| description = " "; |
There was a problem hiding this comment.
Это какое то спорное решение - присваивать пробел. Null - понятно, пустая строка в целом тоже. Они обычно участвуют в проверках вида isNullOrEmpty. Почему пробел? Это как с цифрами. Присвоение 0 и 1 обычно не вызывает вопросов. А вот, скажем, 3 - подозрительно.
No description provided.