-
Notifications
You must be signed in to change notification settings - Fork 21
Homework2 #17
base: master
Are you sure you want to change the base?
Homework2 #17
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Нужно поправить замечания что оставлены к коду.
Общие замечания:
- Нет выделение активного элемента в RecyclerView
- В спиcке имя человека залезает на аватарку
- Если начать вводить имя вся верстка разламывается
- Можно сохранить пустое имя/фамилию
- Кликаю 2 раза “удалить” получаю креш
|
||
public class MainActivity extends AppCompatActivity implements OnClickListener{ | ||
private List<Student> students; | ||
private StudentsAdapter studentsAdapter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Можно не делать полем
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не поняла про активный элемент, что именно имеете ввиду? про имя и верстку, сохранение и удаление исправила. А что именно тут поле?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
studentsAdapter Используется только в одном методе, не обязательно его выносить в поле класса
switch (v.getId()) { | ||
case R.id.button6: | ||
onAddClick(); | ||
setupRecyclerView(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не нужно каждый раз заново вызывать setupRecyclerView и создавать новый адаптер
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Благодаря этому я отрисовываю новый список
} | ||
|
||
private void onStudentClick(Student student, String id) { | ||
TextInputEditText firstname = findViewById(R.id.texInputEdit4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
camelCase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Исправила
btnDelStudent.setContentDescription("1000"); | ||
int id = Integer.parseInt(str); | ||
|
||
if (students.get(id).getSecondName().length()>0) students.remove(id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Лучше расставлять фигурные скобки
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Исправила
} | ||
|
||
private void onSaveStudent() { | ||
int id = Integer.parseInt((String) btnDelStudent.getContentDescription()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не очень хорошая практика передавать что-то через ContentDescription а потом еще и кстатить и пытаться перевести в int
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Благодаря этому я присваиваю идентификаторы записям в листе, description это текстовое поле, а идентификаторы int
Student student = students.get(i); | ||
viewHolder.bind(student); | ||
viewHolder.itemView.setTag(student); | ||
viewHolder.itemView.setContentDescription(Integer.toString(i)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А зачем это нужно? Почему просто тег не подходит?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Чтобы добавить идентификаторы и обращаться к ним впоследствии, изначально их нет, поэтому приходится здесь описать
|
||
private void bind(@NonNull Student student) { | ||
nameTextView.setText(student.getFirstName() + " " + student.getSecondName()); | ||
if (student.isMaleGender()) posterImageView.setImageResource(student.getPhoto()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Лучше ставить фигурные скобки
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Исправила
nameTextView.setText(student.getFirstName() + " " + student.getSecondName()); | ||
if (student.isMaleGender()) posterImageView.setImageResource(student.getPhoto()); | ||
else posterImageView.setImageResource(student.getPhoto()); | ||
; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Лишний символ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Исправила
@@ -1,9 +1,173 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<android.support.constraint.ConstraintLayout xmlns:android="http://schemas.android.com/apk/res/android" | |||
xmlns:app="http://schemas.android.com/apk/res-auto" | |||
xmlns:tools="http://schemas.android.com/tools" | |||
android:layout_width="match_parent" | |||
android:layout_height="match_parent" | |||
tools:context=".MainActivity"> | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Лишняя строка
<TextView |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Все id элементов совсем не отражают предназначения View
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Исправила
|
@@ -3,6 +3,9 @@ | |||
<component name="GradleSettings"> | |||
<option name="linkedExternalProjectsSettings"> | |||
<GradleProjectSettings> | |||
<compositeConfiguration> | |||
<compositeBuild compositeDefinitionSource="SCRIPT" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А зачем это добавлено?
|
||
public class MainActivity extends AppCompatActivity implements OnClickListener{ | ||
private List<Student> students; | ||
private StudentsAdapter studentsAdapter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
studentsAdapter Используется только в одном методе, не обязательно его выносить в поле класса
} | ||
|
||
private void resetBorder(View view) { | ||
if (borderView != null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Лучше всегда ставить фигурные скобки
Работа принята |
No description provided.