ДЗ_2_Сагадеева Светлана #22

gilgenbergg · 2019-03-02T16:34:13Z

No description provided.

otopba

Визуально приложение работает и выполняет поставленную задачу. Но очень много захардкоженных цифр и строк. Нужно все выносить в константы. Так же, многие лейауты имеют излишнюю вложенность. Большенство замечаний повторяются от файла к файлу, когда будешь править замечания, сразу же проверяй другие файлы на предмет такой же проблемы.

Общие замечания:

Можно сохранить студента без имени и фамилии
Лучше в EditText использовать подсказки, нежели дефолтные значения (Second Name, First Name), иначе их приходится постоянно стирать
Язык во всем приложении должен быть един. Кнопки называются на русском, а активити почему-то на английском

otopba · 2019-03-03T17:48:42Z

app/src/main/java/ru/ok/technopolis/students/AdditionActivity.java

+import java.util.Random;
+
+public class AdditionActivity extends AppCompatActivity implements View.OnClickListener {
+    TextView secondName;


Лучше в явном виде устанавливать видимость полей (public,private...)

otopba · 2019-03-03T17:48:58Z

app/src/main/java/ru/ok/technopolis/students/AdditionActivity.java

+    RadioButton isMale;
+    Button saver;
+
+    private String[] image_names;


Стоит использовать camelCase

otopba · 2019-03-03T17:49:27Z

app/src/main/java/ru/ok/technopolis/students/AdditionActivity.java

+        this.setTitle("Addition");
+        setContentView(R.layout.student_addition);
+        Random random = new Random();
+        Intent data = getIntent();


Зачем здесь интент?

otopba · 2019-03-03T17:50:18Z

app/src/main/java/ru/ok/technopolis/students/AdditionActivity.java

+
+    @Override
+    public void onClick(View v) {
+


Лишняя строка

otopba · 2019-03-03T17:50:44Z

app/src/main/java/ru/ok/technopolis/students/AdditionActivity.java

+
+        boolean male = isMale.isChecked();
+        Intent intent = new Intent();
+        intent.putExtra("secondName", secondName.getText().toString());


secondName.getText() может вернуть null

ошибки ввода пользователем (попытки добавить или изменить студента, оставив его с пустой фамилией или именем) обрабатываю в InfoActivity и AdditionActivity соответственно

otopba · 2019-03-03T17:54:44Z

app/src/main/java/ru/ok/technopolis/students/MainActivity.java

+            int index = data.getIntExtra("index", -1);
+            switch (op) {
+                case 2:
+                    if (students.isEmpty())


Фигурные скобки

otopba · 2019-03-03T17:55:50Z

app/src/main/java/ru/ok/technopolis/students/Student.java

-    private String secondName;
-    private boolean maleGender;
-    private int photo;
+    public String firstName;


Не стоит напрямую обращаться к полям модели. Лучше делать поля приватными и обращаться через гетеры/сеттеры. Что бы защититься от случайного изменения

otopba · 2019-03-03T17:56:49Z

app/src/main/res/layout/activity_main.xml

+<?xml version="1.0" encoding="utf-8"?>
+
+<!--suppress ALL -->


Зачем?

otopba · 2019-03-03T17:59:54Z

app/src/main/res/layout/student_addition.xml

@@ -0,0 +1,105 @@
+<?xml version="1.0" encoding="utf-8"?>


Вложенность имеет 3 уровня. Можно было бы этого избежать использовав более сложные лейауты

otopba · 2019-03-03T18:00:15Z

app/src/main/res/layout/student_info.xml

@@ -0,0 +1,118 @@
+<?xml version="1.0" encoding="utf-8"?>


Тоже большая вложенность

otopba

Не все замечания поправлены.
Что чаще всего встречается:

Неиспользуемые поля класса
Лишние поля класса
Константы не вынесены, а захардкожены
Написание переменных не должно быть в стиле С student_to_change, а стоит использовать camelCase

otopba · 2019-03-09T18:15:54Z

app/src/main/java/ru/ok/technopolis/students/AdditionActivity.java

+    private TextView secondName;
+    private TextView firstName;
+    private ImageView photo;
+    private RadioButton isFemale;


Зачем тебе это радиобаттон, если нигде не используешь? isFemale

photo, saver, imageNames могу быть локальными переменными

otopba · 2019-03-09T18:16:31Z

app/src/main/java/ru/ok/technopolis/students/AdditionActivity.java

-        intent.putExtra("photo", pic_id);
-        setResult(RESULT_OK, intent);
-        finish();
+        if ((secondName.getText().toString().trim().length() != 0) ||


secondName.getText() может вернуть null

otopba · 2019-03-09T18:17:11Z

app/src/main/java/ru/ok/technopolis/students/InfoActivity.java


-    private String[] image_names;
+    private String[] imageNames;


imageNames нигде не используется

photo, isFemale, isMale, saver, deleter могут быть локальными

otopba · 2019-03-09T18:17:48Z

app/src/main/java/ru/ok/technopolis/students/InfoActivity.java

        photo.setImageResource(pic_id);
        boolean male;
-        boolean female;
        male = data.getBooleanExtra("male", false);


Зачем тут инициализация в 2 строки? Можно сделать одной

otopba · 2019-03-09T18:18:00Z

app/src/main/java/ru/ok/technopolis/students/InfoActivity.java

+        female = !male;
+        secondName.setText(data.getStringExtra("second_name"));
+        firstName.setText(data.getStringExtra("first_name"));
+        if (male)


@gilgenbergg Все еще актуально

otopba · 2019-03-09T18:18:10Z

app/src/main/java/ru/ok/technopolis/students/InfoActivity.java

+        photo.setImageResource(pic_id);
+        boolean male;
+        boolean female;
+        male = data.getBooleanExtra("male", false);


@gilgenbergg Все еще актуально

otopba · 2019-03-09T18:18:50Z

app/src/main/java/ru/ok/technopolis/students/MainActivity.java

@@ -16,7 +16,7 @@
 public class MainActivity extends AppCompatActivity {
    private ArrayList<Student> students;
    private StudentAdapter studentAdapter;
-    private String[] image_names;
+    private String[] imageNames;


Зачем image_names?

Random стоит сделать private

otopba · 2019-03-09T18:20:05Z

app/src/main/res/values/strings.xml

@@ -1,5 +1,11 @@
 <resources>
    <string name="app_name">Students</string>
+    <string name="SecondName">SecondName</string>


ключи строк пишут через подчеркивание с маленькой буквы

…n fix

otopba · 2019-03-09T20:35:16Z

app/src/main/java/ru/ok/technopolis/students/AdditionActivity.java


-    private String[] imageNames;
-    int pic_id;
+    int picID;


почему не private?

otopba · 2019-03-09T20:35:56Z

app/src/main/java/ru/ok/technopolis/students/InfoActivity.java


-    private String[] imageNames;
+    private static final String PROCESS_NAME = "Editor";


наверное не процесс, а title?

otopba · 2019-03-09T20:36:30Z

app/src/main/java/ru/ok/technopolis/students/InfoActivity.java

+        boolean male = data.getBooleanExtra(MainActivity.MALE_EXTRA, false);
+        secondName.setText(data.getStringExtra(MainActivity.SECOND_NAME_EXTRA));
+        firstName.setText(data.getStringExtra(MainActivity.FIRST_NAME_EXTRA));
+        if (male) {
            isMale.setChecked(true);


можно прям поставить результат в радиобаттон isMale.setChecked(male);

otopba · 2019-03-09T20:37:03Z

app/src/main/java/ru/ok/technopolis/students/MainActivity.java

+    static final String PHOTO_EXTRA = "photo";
+    static final String STUDENT_INDEX_EXTRA = "studentIndex";
+    static final String BUTTON_CODE_EXTRA = "buttonCode";
+


Лишняя строка

otopba · 2019-03-09T20:37:10Z

app/src/main/java/ru/ok/technopolis/students/MainActivity.java

-    Random random;
+    private Random random;
+
+    static final String SECOND_NAME_EXTRA = "secondName";


otopba · 2019-03-09T20:37:47Z

app/src/main/java/ru/ok/technopolis/students/MainActivity.java

@@ -16,8 +16,15 @@
 public class MainActivity extends AppCompatActivity {
    private ArrayList<Student> students;


А нужно ли тебе здесь указывать тип? Лучше оперировать на уровне интерфейсов List students;

otopba · 2019-03-09T20:39:39Z

app/src/main/java/ru/ok/technopolis/students/InfoActivity.java


            case R.id.student_info__b_delete:
-                intent.putExtra("button_code", 2);
-                intent.putExtra("index", getIntent().getIntExtra("index", -1));
+                intent.putExtra(MainActivity.BUTTON_CODE_EXTRA, 2);


Строчки смотрю вынесла, но вот числа не везде. Что означает цифра 2? Откуда она взялась? Эта же двойка будет использоваться далее зачем каждый раз помнить что именно означает цифра 2 если можно завести константу

и с другими кодами тоже

Да, использую два типа кодов: для разделения InfoActivity и AdditionActivity и для разграничения операции удаление/сохранение при работе с InfoActivity. Перенесла в константы.

otopba · 2019-03-09T20:44:33Z

Работа принята.
9 балов

gilgenbergg added 8 commits February 23, 2019 20:58

initial

18aa368

min sdk version was changed, drawable resource for images was added

a7111e7

in progress

e9591a0

Merge branch 'base'

2e62ca2

Student addition

48c697d

last version

7311696

gitignore fix

09a82ad

final

e9b2d04

otopba self-requested a review March 3, 2019 17:47

otopba suggested changes Mar 3, 2019

View reviewed changes

otopba added the Needs work label Mar 3, 2019

fixes

bf15075

otopba suggested changes Mar 9, 2019

View reviewed changes

extras in constants, styling, bug of saving with one field in additio…

da02c09

…n fix

otopba suggested changes Mar 9, 2019

View reviewed changes

otopba added done 9 points and removed Needs work labels Mar 9, 2019

operations constants were added

f920219

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ДЗ_2_Сагадеева Светлана #22

ДЗ_2_Сагадеева Светлана #22

gilgenbergg commented Mar 2, 2019

otopba left a comment

otopba Mar 3, 2019

otopba Mar 3, 2019

otopba Mar 3, 2019

otopba Mar 3, 2019

otopba Mar 3, 2019

gilgenbergg Mar 9, 2019

otopba Mar 3, 2019

otopba Mar 3, 2019

otopba Mar 3, 2019

otopba Mar 3, 2019

otopba Mar 3, 2019

otopba left a comment

otopba Mar 9, 2019

otopba Mar 9, 2019

otopba Mar 9, 2019

otopba Mar 9, 2019

otopba Mar 9, 2019

otopba Mar 9, 2019

otopba Mar 9, 2019

otopba Mar 9, 2019

otopba Mar 9, 2019

otopba Mar 9, 2019

otopba Mar 9, 2019

otopba Mar 9, 2019

otopba Mar 9, 2019

otopba Mar 9, 2019

otopba Mar 9, 2019

gilgenbergg Mar 9, 2019

otopba commented Mar 9, 2019


		private String[] imageNames;
		private static final String PROCESS_NAME = "Editor";

		@@ -16,8 +16,15 @@
		public class MainActivity extends AppCompatActivity {
		private ArrayList<Student> students;

ДЗ_2_Сагадеева Светлана #22

Are you sure you want to change the base?

ДЗ_2_Сагадеева Светлана #22

Conversation

gilgenbergg commented Mar 2, 2019

otopba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otopba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otopba commented Mar 9, 2019