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

Students homework android #9

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

Students homework android #9

wants to merge 6 commits into from

Conversation

BrainLUX
Copy link

No description provided.

@otopba otopba self-requested a review March 3, 2019 19:37
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.

Хороший и аккуратный код.
Совсем небольшие замечания


public class MainActivity extends AppCompatActivity {

private List<Student> students;
private EditText surName, name;
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);
Objects.requireNonNull(getSupportActionBar()).hide();
Copy link

Choose a reason for hiding this comment

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

Зачем requireNonNull?

}

private int getRandomPhoto(boolean maleGender) {
if (maleGender)
Copy link

Choose a reason for hiding this comment

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

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


private void onStudentClick(View view, int i) {
if (index != -1) {
if (checkFields())
Copy link

Choose a reason for hiding this comment

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

И здесь без скобок тяжко смотреть

if (index != -1) {
if (checkFields())
if (isDataSaved())
Toast.makeText(MainActivity.this, "Вам следует сохранить изменения", Toast.LENGTH_SHORT).show();
Copy link

Choose a reason for hiding this comment

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

Если есть желание, то перенеси все строки в ресурсы


private void setBorderVisibility(View view, int visible) {
if (view != null) {
view.findViewById(R.id.student_item__top).setVisibility(visible);
Copy link

Choose a reason for hiding this comment

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

Не очень хорошо каждый раз искать вью по айди. Лучше один раз найти и запомнить

Toast.makeText(MainActivity.this, "Студент сохранён", Toast.LENGTH_SHORT).show();
studentAdapter.notifyDataSetChanged();
}

Copy link

Choose a reason for hiding this comment

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

Лишние пустые строки

nameTextView.setText(String.valueOf(student.getSecondName() + " " + student.getFirstName()));
avatarImageView.setImageResource(student.getPhoto());
}

Copy link

Choose a reason for hiding this comment

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

Пустые строки

android:layout_height="match_parent"
android:fillViewport="true"
tools:ignore="ExtraText,UselessParent">
>
Copy link

Choose a reason for hiding this comment

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

Лишний символ

@@ -1,9 +1,201 @@
<?xml version="1.0" encoding="utf-8"?>
<android.support.constraint.ConstraintLayout xmlns:android="http://schemas.android.com/apk/res/android"
<RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android"
Copy link

Choose a reason for hiding this comment

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

Зачем тут релатив, если у тебя скрол вью во весь экран?

@BrainLUX
Copy link
Author

BrainLUX commented Mar 4, 2019

@otopba fixed

else
if (checkFields()) {
if (isDataSaved()) {
Toast.makeText(MainActivity.this, getResources().getString(R.string.userShouldSave_warning), Toast.LENGTH_SHORT).show();
Copy link

Choose a reason for hiding this comment

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

1)Можно писать просто this
2) Toast.makeText может принимать вторым аргументом прямо id строки: R.string.userShouldSaveCreated_warning

}

private boolean isDataSaved() {
return students.get(index).getFirstName().trim().length() == 0 || students.get(index).getSecondName().trim().length() == 0;
}

private boolean checkFields() {
if (name.getText().toString().trim().length() > 0 && surName.getText().toString().trim().length() > 0)
Copy link

Choose a reason for hiding this comment

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

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

app:layout_constraintBottom_toBottomOf="@id/guideline_horizontal"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintStart_toStartOf="parent" />

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 4, 2019

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

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