Skip to content
This repository has been archived by the owner on Mar 1, 2021. It is now read-only.

not full functionality #18

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

not full functionality #18

wants to merge 8 commits into from

Conversation

EgoriusE
Copy link

No description provided.

@otopba otopba self-requested a review March 3, 2019 20:39
@otopba
Copy link

otopba commented Mar 3, 2019

@EgoriusE Код не компилируется

Пожалуйста, удали из репозитория папку .idea. Это моя изначальная ошибка. Можно забрать изменения из моего репозитория, там исправлена эта проблема

Copy link

@otopba otopba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Нужно поправить замечания что оставлены к коду.
Общие замечания:

  1. Нет выделение активного элемента в RecyclerView
  2. Очень длинное имя разламывает верстку
  3. При смене пола не меняется аватарка
  4. Если пытаться сохранить юзера с пустыми полями нет никакого предупреждения
  5. По клику “удалить” студент не удаляется

import android.support.v7.app.AppCompatActivity;
import android.support.v7.widget.LinearLayoutManager;
import android.support.v7.widget.RecyclerView;
import android.view.KeyEvent;
Copy link

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 {
private Random random = new Random(2);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Зачем это поле?

private Student currentStudent;

List<Student> students;
TextView text;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

где-то область видимости указывается явно, а где-то нет. Лучше всегда указывать явно


@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
setContentView(R.layout.activity_main);

sex = findViewById(R.id.activity_main_sex);
male = findViewById(R.id.activity_main_sex_male);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Названия переменных стоит давать более полные. Что бы можно было отличить кнопку от текста

add.hide();
bottomView.setVisibility(View.VISIBLE);

if (student.isMaleGender())
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Лучше всегда расставлять фигурные скобки

RadioButton male;
RadioButton female;
Button delete;
Button save;

@Override
protected void onCreate(Bundle savedInstanceState) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не стоит делать такие длинные методы. Раздели на несколько

@@ -7,14 +7,14 @@
private boolean maleGender;
private int photo;

public Student(String firstName, String secondName, boolean maleGender, int photo) {
Student(String firstName, String secondName, boolean maleGender, int photo) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Зачем убрал public?

@@ -1,9 +1,154 @@
<?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"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Констрейнт вложенный в констрейнт можно избежать. Уменьшай вложенность

xmlns:tools="http://schemas.android.com/tools"
android:layout_width="match_parent"
android:layout_height="wrap_content">

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Следи за лишними строками

@otopba
Copy link

otopba commented Mar 9, 2019

@EgoriusE
Проект не собирается. Нет ресурса recycler_view_item_circle_icon

@EgoriusE
Copy link
Author

EgoriusE commented Mar 9, 2019

К сожалению не нашел бескостыльного способа выделения активного элемента RecyclerView

@@ -5,136 +5,188 @@
import android.support.v7.app.AppCompatActivity;
import android.support.v7.widget.LinearLayoutManager;
import android.support.v7.widget.RecyclerView;
import android.view.KeyEvent;
import android.view.View;
import android.widget.Button;
import android.widget.EditText;
import android.widget.ImageView;
import android.widget.RadioButton;
import android.widget.RadioGroup;
import android.widget.TextView;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Лишний импорт

import java.util.LinkedList;
import java.util.List;
import java.util.Random;

public class MainActivity extends AppCompatActivity {
private Random random = new Random(2);
private Random random = new Random();
private int r;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Название r ни о чем не говорит

bottomView.setVisibility(View.GONE);
add.show();

if (!firstNameText.getText().toString().equals("") &&
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

firstNameText.getText() иногда может вернуть null

@@ -12,7 +12,8 @@

public class StudentAdapter extends RecyclerView.Adapter<StudentAdapter.StudentViewHolder> {
private final Listener onStudentClickListener;
private final List<Student> students;
private List<Student> students;
private View lastV;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Лучше так не сокращать, не понятно что такое V

@@ -27,7 +28,8 @@ public StudentViewHolder onCreateViewHolder(@NonNull ViewGroup viewGroup, int i)
view.setOnClickListener(new View.OnClickListener() {
@Override
public void onClick(View v) {
onStudentClickListener.onStudentClick((Student) v.getTag());
onStudentClickListener.onStudentClick((Student) v.getTag(), v , lastV);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Передавать куда-то вью из списка плохая практика. Лучше делать все внутри адаптера

Посмотри как сделано тут, например, #10

@otopba
Copy link

otopba commented Mar 9, 2019

При длинном имени разламывается верстка
image

@otopba
Copy link

otopba commented Mar 12, 2019

Верстка все еще разъезжается
image

@otopba
Copy link

otopba commented Mar 12, 2019

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants