Виолетта Мамыкина, Адресная книга #5

Violetta-Mamykina · 2018-02-24T17:24:06Z

No description provided.

h31 · 2018-02-25T00:07:02Z

src/main/java/ru/spbstu/kspt/task1/Main.java

-    public static void main(String[] args) {
-        logger.debug("Logging example");
-        System.out.println("Hello World!");
+    public static class Address {


Имя класса и имя файла должны совпадать. Переименуйте, пожалуйста, файл.

h31 · 2018-02-25T00:17:31Z

src/main/java/ru/spbstu/kspt/task1/Main.java

+        private int flat;
+
+        Address(String street, int house, int flat) {
+            this.street = street;


Передаваемые аргументы необходимо проверять на корректность, особенно если речь про публичные методы и конструкторы. Насколько это корректно, если house окажется равным -5?

Если класс public, то и конструктор уместно сделать public.

h31 · 2018-02-25T00:24:00Z

src/main/java/ru/spbstu/kspt/task1/Main.java

+        private static Map<Person, Address> addressBook = new LinkedHashMap<>();
+
+        public addressBook(Map<Person, Address> myMap) {
+            this.addressBook = myMap;


HashMap является изменяемым (mutable) типом, поэтому если кто-то извне случайно изменит myMap, то изменения отразятся и на addressBook. Чтобы избежать подобных сюрпризов, лучше создавать новый объект и заполнять его данными из myMap.

h31 · 2018-02-25T00:25:28Z

src/main/java/ru/spbstu/kspt/task1/Main.java

+        }
+
+
+        public void addAddressBook(Person person, Address address) {


addAddressBook? Не очень подходящее название. Мы добавляем адресную книгу? Или человека с его адресом?

h31 · 2018-02-25T00:26:33Z

src/main/java/ru/spbstu/kspt/task1/Main.java

+        public Address getAddress(Person person) throws IllegalAccessException {
+            if (addressBook.containsKey(person)) {
+                return addressBook.get(person);
+            } else throw new IllegalAccessException();


Если ставите фигурные скобки около одной ветви if, то ставьте их и около другой, для симметричности и удобства чтения.

IllegalAccessException - не самый подходящий тип исключения (по смыслу). Есть варианты получше.

h31 · 2018-02-25T01:06:58Z

src/test/java/ru/spbstu/kspt/task1/MainTest.java

+        Main.addressBook addressBook = createAddressBook();
+        addressBook.changeAddressBook(((Main.Person) MyMap.keySet().toArray()[1]), new Main.Address("Харченко", 16, 540));
+        addressBook.changeAddressBook((Main.Person) MyMap.keySet().toArray()[0], new Main.Address("Шателена", 18, 8));
+        try {


Не нужно ловить исключения в тестах. Если будет выброшено исключение, то система тестирования сама его поймает и сообщит о нем.

h31 · 2018-02-25T10:40:14Z

src/main/java/ru/spbstu/kspt/task1/Main.java

+            return this.street;
+        }
+
+        public Integer getHouse() {


Используйте примитивные типа (int) везде, где можно. Integer используется только там, где нельзя использовать int.

h31 · 2018-02-25T10:43:28Z

src/main/java/ru/spbstu/kspt/task1/Main.java

+            return listOfPerson;
+        }
+
+        public List findOnHouse(String street, Integer house) {


Не забывайте указывать, какого типа данные хранит внутри себя List (с помощью обобщенных типов, generic types).

h31 · 2018-02-25T10:54:45Z

src/main/java/ru/spbstu/kspt/task1/Main.java

+        public List findOnHouse(String street, Integer house) {
+            List<Person> listOfPerson = new ArrayList<>();
+            for (Person person : addressBook.keySet()) {
+                if (addressBook.get(person).street == street && addressBook.get(person).house == house) {


См. findOnStreet

h31 · 2018-02-25T11:06:34Z

src/test/java/ru/spbstu/kspt/task1/MainTest.java

-    private static final Logger logger = LogManager.getLogger(MainTest.class);
+    private static Map<Main.Person, Main.Address> MyMap = new LinkedHashMap<>();
+
+    private static Main.addressBook createAddressBook() {


Сделайте так, чтобы к классам можно было обращаться, не указывая Main.

h31 · 2018-03-13T12:39:37Z

src/main/java/ru/spbstu/kspt/task1/Address.java

+import java.util.Objects;
+
+public class Address {
+    public String street;


Если вы пишете геттеры, то поля делайте private, иначе смысла нет в геттерах.

h31 · 2018-03-13T12:42:30Z

src/main/java/ru/spbstu/kspt/task1/Address.java

+public class Address {
+    public String street;
+    public int house;
+    private int flat;


Поле private, но для него нет геттера. Что-то записали в объект, а прочитать это не можем.

h31 · 2018-03-13T12:49:03Z

src/main/java/ru/spbstu/kspt/task1/Address.java

+
+    @Override
+    public String toString() {
+        return street + " " + house + " " + flat;


Адреса обычно пишут через запятую. указывая, где квартира, а где номер дома. Поищите, как пишут адреса.

h31 · 2018-03-13T12:49:43Z

src/main/java/ru/spbstu/kspt/task1/AddressBook.java

+public class AddressBook implements AddressBookInterface{
+    public Map<Person, Address> addressBook = new LinkedHashMap<>();
+
+    public AddressBook() {


Если у вас только один конструктор и он пустой, то его можно не писать.

h31 · 2018-03-13T12:54:09Z

src/main/java/ru/spbstu/kspt/task1/AddressBook.java

+    }
+
+    public void addPersonWithAddress(Person person, Address address) {
+        addressBook.put(person, address);


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

h31 · 2018-03-13T12:58:07Z

src/main/java/ru/spbstu/kspt/task1/Person.java

+import java.util.Objects;
+
+public class Person {
+    public String name;


Напишите геттер, а поле сделайте private.

h31 · 2018-03-13T13:04:17Z

src/main/java/ru/spbstu/kspt/task1/AddressBook.java

+    }
+
+    public void changeAddress(Person person, Address address) {
+        if (addressBook.containsKey(person)) {


Уместно сообщить об ошибке, если ничего не заменили.

h31 · 2018-03-13T13:06:20Z

src/main/java/ru/spbstu/kspt/task1/Address.java

+    private int flat;
+
+    public Address(String street, int house, int flat) {
+        this.street = street;


Адрес тоже наверное стоит проверять, как минимум пустым он быть не может.

h31 · 2018-03-13T13:08:57Z

src/test/java/ru/spbstu/kspt/task1/MainTest.java

-    private static final Logger logger = LogManager.getLogger(MainTest.class);
+    private static AddressBook createAddressBook() {
+        AddressBook addressBook = new AddressBook();
+        addressBook.addPersonWithAddress(new Person("Дженжер"),


С тестами всё хорошо, только не вижу тестов с некорректными данными. И вообще проверки некорректных ситуаций.

h31 · 2018-03-13T13:14:11Z

src/main/java/ru/spbstu/kspt/task1/Address.java

+        if (house <= 0 || flat <= 0) throw new IllegalArgumentException();
+        this.house = house;
+        this.flat = flat;
+    }


Задание несложное, так что давайте сделаем что-нибудь дополнительное, чтобы я со спокойной душой поставил вам 5. Как насчет того, чтобы сделать конструктор для Address, который бы считывал адрес из строки? Строку брать в каком-нибудь готовом формате, например, почтовом.

Я давно вас не проверял, поэтому могу несколько дней подождать.

h31

Исправьте замечание к toString и к тестам. Если всё будет в порядке с исправлениями, я готов поставить 5.

h31 · 2018-03-15T18:41:08Z

src/test/java/ru/spbstu/kspt/task1/MainTest.java

+        }
+        try {
+            addressBook.addPersonWithAddress(new Person("Виноградный"),
+                    new Address("наб.Фонтанки", -8, 359));


Как раз для случаев, когда нужно проверить, что при указанных входных данных метод выбрасывает исключение, создан тестовый метод assertThrows. Там, где исключения быть не должно, try лучше убрать.

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

h31 · 2018-03-15T18:45:23Z

src/main/java/ru/spbstu/kspt/task1/AddressBook.java

+
+    public List<Person> findOnStreet(String street) {
+        List<Person> listOfPerson = new ArrayList<>();
+        for (Map.Entry<Person, Address> mapSet : addressBook.entrySet()) {


Речь была про mapSet, я бы назвал его addressBookEntry или addressBookRecord. Хотя имя метода тоже стало лучше, правильное изменение.

h31 · 2018-03-15T18:46:41Z

src/main/java/ru/spbstu/kspt/task1/Address.java

    @Override
    public String toString() {
-        return street + " " + house + " " + flat;
+        return " ул. " + street + ", д. " + house + ", кв. " + flat;


Пробел в начале строки, двоеточие в Person.toString - это всё нужно перенести в более подходящие места.

h31 · 2018-03-15T18:49:25Z

src/main/java/ru/spbstu/kspt/task1/Address.java

    }

+    public Address(String address) {
+        if (address.matches("ул\\.\\s+.+,\\s+д\\.\\s+\\d+,\\s+кв\\.\\s+\\d+")) {


Раз уж вы используете регулярку, есть смысл использовать группы для извлечения конкретных полей из строки.

h31 · 2018-03-15T22:37:02Z

Ставлю 5.

Violetta-Mamykina added 3 commits February 18, 2018 15:31

first

62ad8ca

added tests

b48052f

corrected findOnHouse and tests

5de074f

h31 requested changes Feb 25, 2018

View reviewed changes

Violetta-Mamykina added 5 commits March 5, 2018 23:36

Completed change for comments; Add auto-testing

08828de

All completed

685b2a7

Completed change for comments; Add auto-testing

656018a

Added tests

620138e

Added interface

a2162e3

h31 requested changes Mar 13, 2018

View reviewed changes

Violetta-Mamykina added 2 commits March 15, 2018 01:11

Correction based on comments

fcf24aa

Corrected toString

05ed524

h31 requested changes Mar 15, 2018

View reviewed changes

Corrected

86e5d0b

h31 added the excellent Отлично label Mar 15, 2018

		}


		public void addAddressBook(Person person, Address address) {

Виолетта Мамыкина, Адресная книга #5

Are you sure you want to change the base?

Виолетта Мамыкина, Адресная книга #5

Uh oh!

Conversation

Violetta-Mamykina commented Feb 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

h31 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

h31 commented Mar 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants