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

Students | Vadim Dyachkov #10

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

Students | Vadim Dyachkov #10

wants to merge 8 commits into from

Conversation

vaddya
Copy link

@vaddya vaddya commented Feb 27, 2019

No description provided.

@otopba otopba self-requested a review March 3, 2019 19:51
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. Можно сохранить студента без имени и фамилии


private void init(Context context) {
LayoutInflater inflater = (LayoutInflater) context.getSystemService(Context.LAYOUT_INFLATER_SERVICE);
View view = inflater.inflate(R.layout.student_view, this, false);
Copy link

Choose a reason for hiding this comment

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

Что бы не писать addView(view); можно третим аргументов поставить true

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@Override
public void onClick(View v) {
if (student != null) {
student.setFirstName(firstNameTv.getText().toString());
Copy link

Choose a reason for hiding this comment

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

firstNameTv.getText() иногда возвращает null

Copy link
Author

Choose a reason for hiding this comment

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

fixed

hideBtn.setOnClickListener(new OnClickListener() {
@Override
public void onClick(View v) {
onHide.onClick(v);
Copy link

Choose a reason for hiding this comment

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

Во всех лисенерах нет проверки на нул. Если не был установлен лисенер

Copy link
Author

Choose a reason for hiding this comment

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

fixed

import java.util.ArrayList;
import java.util.List;

public class StudentsRepository {
Copy link

Choose a reason for hiding this comment

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

Для данного задания такое решение подходит. Но в перспективе если с репозиторием будет работать не один поток будут проблемы

android:layout_width="match_parent"
android:layout_height="match_parent">

<View
Copy link

Choose a reason for hiding this comment

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

Зачем нужна эта полоса?

Copy link
Author

Choose a reason for hiding this comment

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

"Разделитель" между списком и формой студента

Copy link

Choose a reason for hiding this comment

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

Ок

@vaddya
Copy link
Author

vaddya commented Mar 3, 2019

@otopba fixed

@@ -60,33 +59,39 @@ private void init(Context context) {
saveBtn.setOnClickListener(new OnClickListener() {
@Override
public void onClick(View v) {
CharSequence firstName = firstNameTv.getText();
CharSequence secondName = secondNameTv.getText();
if (firstName == null || firstName.length() == 0 || secondName == null || secondName.length() == 0) {
Copy link

Choose a reason for hiding this comment

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

Можно проверять с помощью TextUtils.isEmpty()

android:layout_width="match_parent"
android:layout_height="match_parent">

<View
Copy link

Choose a reason for hiding this comment

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

Ок

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.

Работа принята.
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