В общем, хотел получить работу джуниора в одной компании. Для проверки мне прислали пару заданий:




За пару дней я выполнил эти задания (смотрите вложенный архив), но мне отказали в работе. Конечно, мне не указали на ошибки. Поэтому прошу более опытных программистов покритиковать мой код. Уж очень хочется разобраться - возможно, я действительно еще не готов для этой работы. Заранее благодарю.

Это сообщение отредактировал(а) Iktash - 21.7.2010, 16:22

Присоединённый файл ( Кол-во скачиваний: 93 )
 Executed_tasks.rar 4,66 Kb

Хм... Не вижу архив

прошу прощения, исправил.

Замеченное при беглом просмотре:

• Отсутствие документации javadoc в первом задании.
• Отсутствие пробелов вокруг операторов (лично меня это порядком раздражает). В частности длинные условия в SimpleArrayList довольно трудночитаемы.
• В классе SimpleArrayList у вас много проверок не является ли параметр null'ом, и даже в таком случае методы отрабатывают и что-то возвращают. Наверное, предполагалось в таких случая бросать исключение NullPointerException, а не угождать пользователю.

Лично мне не понравилось:

0. Почему в "if else" в некоторых местах отсутствуют скобки? Нет если сложно их написать, то я пойму, но все же как то неаккуратно.

1. Высокая плотность кода. Вот нафига все в одну строку пихать? Отформатировать код довольно несложная штука, благо все нормальные IDE это могут.
Пример:


2. При реализации класса SimpleArrayList реализовать интерфейс java.util.List или на худой конец java.util.Collection.

Дополнительно:

0. В большинстве случаев можно было использовать staic импорты, особенно в тестах. Так красивше.

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

Дальше лениво думать, может быть еще стоит вдумчиво задание прочитать, может быть там есть что то важно-незаметное 

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

•  Unit тесты. Скачайте небольшое описание Junit and EasyMock , прочитайте и вы поймете что и где у Вас не так.
•  В ReverseList можно было не выдумывать велосипед, а использовать 

  Код

  Collections.reverse ( list );

   
  скорее всего, проверяющие как раз это и хотели увидеть (знаете ли вы полезные библиотеки и классы)
•  У list'ов не указаны generic типы (List<String>, например).

Плюс все другие замечания.
Конечно, при более долгом рассмотрении найдется еще что-то.

P.S. Метод equals в SimpleArrayList реализван неправильно... что будет, если obj == null?
P.P.S Очень мало Unit тестов. 

Это сообщение отредактировал(а) agR - 21.7.2010, 17:17

Понятно, замечаний действительно много и все резонные. Спасибо всем. Буду стараться дальше  

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

Implement SimpleArrayList and write unit tests for the class.


Implement ReverseList.reverseList (List list) method and write unit tests for the class.


Присоединённый файл ( Кол-во скачиваний: 12 )
 questions.zip 15,09 Kb

Ну давайте погадаем, исходя из того, что тест явно преследовал цель, чтобы
вы показали во всей красе свои знания. 
В этом-то и коварство таких тестов, когда задача сама по себе в духе "2+2=?"

1. За отсутствующую документацию, полагаю, что пример не делает скидок на "самоочевидность".
То, что у вас есть, лучше бы его не было вообще, тогда хотя бы могли  рассказать про самодокументированный код. 

2. За незнание/неумение пользоваться такими вещами как PMD, FireBug, Checkstyle, CodePro ...
Попробуйте и сразу поймете "чего в супе не хватает" - там пунктов на 50-60 в каждом классе.

Вот написал и подумал, что я не прав и вам не дали возможности ими воспользоваться.

НО!  Кто помешал ими воспользоваться сейчас ?!


3. За использование имен переменных из одной буквы, да еще буквы "o", расстреливал бы сразу, вместе с любителями буквы "l". 

4. За область видимости по-умолчанию для  Object[] data;

5. Отдельное "спасибо" за вещи типа:


Это вообще что было? Попытка склонировать?

И вообще, из названия метода getList следует только то, что он должен вернуть этот самый List, а не создавать его.

Ну кому нужен ваш класс, работающий с массивом, без этого самого массива?

Какой религиозный принцип заставил перенести создание массива в get метод?

6. Где GENERICS?

7. За "экономию" на фигурных скобках.

8. За множественные "return"

9. За list[i].equals(o) == true в методе indexOf(...
И там же за 
if (o == null) return -1;
Это почему же у вас я не могу поместить null в список ?

10. За подобный remove() и, особенно, за хвастовство 


11. За NPE при попытке добавить объект null. И особено за тесты, которые вообще не тестируют граничное поведение.

12. За приращение массива по единичке!
 
За одно это надо "rip your balls off" !  
Вы по сути одним этим сделали ваш класс никому не нужным!

13. Да даже за комментарии в духе "SimpleList implements some methods of List interface"
Как вы сами думаете, какую это информацию несет?

Типа (Mille pardons my English)  -  
Have a look at a List interface, write down all methods on a paper, then have a look at my super class and compare!
You will be amazed! 

Ну и т.д. и т.п.




Это сообщение отредактировал(а) carper - 3.3.2011, 17:45