Implementation #1
Description
1. Использование дефолтного пакета для размещения кода is considered as a bad practice;
2. Что дает модификатор final для класса AddressBook? Чем мотивировано такое решение?
3. Каков модификатор области видимости класса Address? Почему выбран именно такой? Аналогично про область видимости конструктора (пока смущает, в частности, соотношение областей видимости конструктора и методов)
4. Используется ли поле name класса AddressBook?
5. Если у Вас есть структурированный класс Address, почему в membersList ArrayList в качестве значений? С точки зрения как ООП, так и читаемости кода, логичнее, кажется, туда положить объекты класса Address
6. Для обоих классов, как для описывающих данные, целесообразно, помимо toString() переопределить также equals и hashCode (до этого стоит почитать в документации про контракты этих методов - каким условиям должна удовлетворять их реализации, и в дальнейшем быть готовым обсудить, где и в каких условиях их применение может быть полезным)
7. У Вас точно может быть только одна адресная книга в программе? (кажется, что-то пошло не так с модификаторами membersList)
8. Возвращать коллекции принято через использование интерфейсов, а не тех реализаций, которые используются внутри класса/ метода (используя интерфейсы, Вы "подписываете обязательство" на то, что возвращаемая коллекция должна реализовывать только часть методов, которыми обладает коллекция, на данный момент реально используемая в реализации - и внешние "клиенты", которым Вы её вернёте, могут рассчитывать только на эти методы. Таким образом, внутри своего метода Вы потом можете менять коллекцию, например, на другие виды, удовлетворяющие интерфейсу - а в использующем коде при этом менять ничего не придётся). Поля также обычно объявляются с применением интерфейсов. Например, HashMap<String, ArrayList> membersList -> Map<String, List> membersList, то же самое актуально для сигнатур методов
9. Для методов типа addMember/ removeMember и т.д. принято возвращать булевы величины (или, например, предыдущее значение), чтобы программист-клиент мог понять, всё пошло так, как он того ожидал (можно ориентироваться на документацию к стандартным коллекциям/ Map, например, и делать по аналогии).“throw new IllegalArgumentException("This name isn't member”);” - в принципе, так тоже можно, но точно нужно такое делать в методе поиска (геттере?) Кажется, что нет, т.к. если мы кого-то ищем, то в справочнике его может и не быть, это всё-таки скорее не исключительная ситуация, а нормальная.
10. getAddress AddressBook-а в норме всё, что должен делать - возвращать объект Address, у которого есть свой toString()
11. getMemberByStreet - если у нас есть структурированная информация (например, набор фамилий людей) - то в большинстве ситуаций стремимся возвращать её в структурированном виде, превратить в строку программисту-клиенту куда проще, чем парсить. А может, он их посчитать захочет, например?
12. Навскидку кажется, что getMemberByStreetAndHouse должен бы внутри себя вызывать как служебный getMemberByStreet (сокращение объёмов кода, увеличение читаемости)
13. "/n" - не единственный вариант завершения строки. Для того, чтобы быть устойчивым к возможным переездам между разными версиями ОС, имеет смысл пользоваться, например, [методом System.lineSeparator()]https://www.dotnetperls.com/newline-java);
14. Константы, вроде той, с помощью которой Вы описываете исключения, лучше выносить в переменную и использовать ее (объем кода + проще изменять в случае необходимости)