Магические числа и насколько это плохо?

Вопрос по задаче

Архивный

Решил задачу и проверил стиль кода, как результат много замечаний по магическим числам. Как лучше делать? допустим класс Date. Если в конструкторе мы вводим год как число int, то прибавляет это значение к 1900. Следовательно, чтобы получать корректные значения, нужно отнимать 1900. Что я и сделал:

allPeople.add(Person.createMale(args[1], new Date(BirthayDay[2]-1900,BirthayDay[1]-1,BirthayDay[0])));

Валидатор принял задачу, но как сделать код грамотнее? код с ошибками

public static void main(String[] args) throws Exception{
        //start here - начни тут
       int[] BirthayDay; // массив, который хранит дату рождения = первый элемент год, второй месяц, третий день

       switch (args[0]){
           case "-c":
               BirthayDay=calender(args[3]); // функция, которая заполняет массив, разделяя строку на символы, в данной ситуации дата это третий элемент
               int id;

               if(args[2].equals("м")){

                   allPeople.add(Person.createMale(args[1], new Date(BirthayDay[2]-1900,BirthayDay[1]-1,BirthayDay[0])));
               }
               else{
                   allPeople.add(по аналогии );
               }

                System.out.println(allPeople.size()-1);


           break;



           case "-u":
               BirthayDay=calender(args[4]);
               // Заполняем дату рождения
               Date personDate = new Date(BirthayDay[2]-1900,BirthayDay[1]-1,BirthayDay[0]);
               //присваиваем ид
               id= Integer.parseInt(args[1]);
               //DeletePerson(id);
               allPeople.remove(id);
               if(args[3].equals("м")){

                   allPeople.add(id,Person.createMale(args[2],personDate));
               }
               else {
                   allPeople.add(девушку по аналогии);

               }
               System.out.println(printPerson(id));
               break;


           case "-d":
               id= Integer.parseInt(args[1]);
              DeletePerson(id);
               break;

           case "-i":
               id= Integer.parseInt(args[1]);
               System.out.println(printPerson(id));
               break;
       }


    }
    private static int[] calender(String s){
        int[] a=new int[3];
        String[] dataArr = s.split("/");

        for (int i = 0; i <dataArr.length ; i++) {
            a[i]=Integer.parseInt(dataArr[i]);
        }
      return a;
    }
     private static String printPerson(int id){
       if(allPeople.get(id).getSex().toString().equals("MALE")) {
           return allPeople.get(id).getName() + " м " +
                   new SimpleDateFormat("dd-MMM-yyyy", Locale.ENGLISH).format(allPeople.get(id).getBirthDate());
       }
       else {
          или возвращаем девушку
       }
}
        private static void DeletePerson(int id){
        Person P = создаем Р из списка по id
        P.setSex(null);
        P.setBirthDate(null);
        P.setName(null);


}

Комментарии (6)

популярные
новые
старые

Для того, чтобы оставить комментарий Вы должны авторизоваться

JustinianJudge в Mega City One

Master

29 июля 2019, 13:57

Та все читаемо, код вполне ясен, много сильных сторон видно, в целом хорошо, но меня интересует то что можно улучшить :) Если какие-то замечания мимо из-за сознательного изменения готового решения, так и будет. Попрошу не удалять вопрос, пусть другие тоже читают. И высказывают свои предложения, возражения, дополнения :)

JustinianJudge в Mega City One

Master

29 июля 2019, 13:57

По поводу Date вопрос простой в принципе. Если не знаешь где выход ищи вход. Зачем тебе передавать в конструктор Date информацию по частям? Тебе же не нужно отдельно год, отдельно число, зачем тебе внутренности и как хранит в себе дату этот класс. Тебе нужна дата целиком. Так получи эту дату.

Date birthday = parseDate(args[3]);

метод calender ты удаляешь. Вместо него:

private static Date parseDate(String value) {
   Date date = null;
   try {
      SimpleDateFormat dateFormat = new SimpleDateFormat("dd/MM/yyyy", Locale.ENGLISH);
      date = dateFormat.parse(value);
   } catch (ParseException e) {
   }
   return date;
}

И потом просто используешь эту дату:

if (args[3].equals("м")) {
     allPeople.add(id, Person.createMale(args[2], birthday));
}

Просто, лаконично, и никаких раздутых строк. Замечания: 1.

int[] BirthayDay;

javaNamingConventions - имена методов и переменных с маленькой буквы (кроме констант). 2. Приветствую разбивку на хелпер методы, это действительно очень хорошо. Во-первых, использование switch case. Рано или поздно сам от него откажешься, чем раньше тем лучше. Также, нужно разбивать все по методах. Каждый логический блок кода, как правило выделяется в отдельный метод. Вот например мой код этой задачи:

if (inputParam.equals("-c")) {
    addPerson(name);
} else if (inputParam.equals("-u")) {
    updatePerson(id, name);
} else if (inputParam.equals("-d")) {
   ereasePersonData(id);
} else if (inputParam.equals("-i")) {
   printPerson(id);
}

Это не образец для подражания, тоже чисто учебный вариант, но пример как можно еще. Код написанный блоками/методами гораздо легче рефакторить и гораздо сложнее в нем допустить ошибку, когда мухи отдельно, котлеты отдельно.

JustinianJudge в Mega City One

Master

29 июля 2019, 13:56

private static String printPerson(int id) {

а вот это плохо. Открываю я твой код, за минутку причесал, чтобы он запустился и был рабочим, думаю потестирую-ка ключ "-c". Отработало нормально, выдало на экран "2". Думаю, хочу глянуть что оно добавило. Смотрю, о, метод printPerson(int id) есть, я обрадовался, добавляю строку:

printPerson(2);

и получаю вывод в консоль: 2. В недоумении открываю метод, и оказыватся, метод с названием "печатаю Персон по айдишнику" на самом деле возвращает стрингу ....м-дамсь. Это пример того как не нужно делать. Имена методов не должны вводить в заблуждение. Если в методе есть слово принт - он должен печатать :) 4. Это пример перегруженных конструкций, запомни, если все скидано в одну строку, это хуже чем если бы было много строк.

allPeople.add(Person.createMale(args[1], new Date(BirthayDay[2]-1900,BirthayDay[1]-1,BirthayDay[0])));

вместо этого лучше:

Date date = new Date(BirthayDay[2]-1900,BirthayDay[1]-1,BirthayDay[0]);
Person person = new Person(args[1], date);
allPeople.add(person);

Легче читается, легче рефакторится, сложнее допустить ошибку.

JustinianJudge в Mega City One

Master

29 июля 2019, 13:56

private static String printPerson(int id) {
  if (allPeople.get(id).getSex().toString().equals("MALE")) {
      return allPeople.get(id).getName() + " м " +
           new SimpleDateFormat("dd-MMM-yyyy", Locale.ENGLISH).format(allPeople.get(id).getBirthDate());
        } else {
            в таком же духе

Я так понимаю метод выглядел как-то так? Тут нужно уточнение, что бывают действительно случаи, когда мы пишем так, в методах типа toString мы действительно собираем строку и это выглядет громоздко. Но мы там собираем строку из стрингов, а в этом случае происходит и сравнение и много логики. Что здесь плохо, хотел предложить тебе посчитать,сколько и чего используется в этих строках: литералов - 5, 2 переменные - 10 раз, 7-8 классов и 14 методов, 42 оператора о.О Это уже слишком. +заметное дублирование кода, две практически идентичных строки возвращаются. Давай отрефакторим вместе. 1. Этот метод, должен печатать. В блоке case "-i" меняем: System.out.println(printPerson(id)); на printPerson(id); Объявление метода меняем: private static void printPerson(int id) { В условии также у тебя: if (allPeople.get(id).getSex().toString().equals("MALE")) { но лучше использовать enum: if (allPeople.get(id).getSex().equals(Sex.MALE)) { Хотя нам это не нужно. Я бы предложил:

private static void printPerson(int id) {
    Person person = allPeople.get(id);
    String sex = person.getSex().equals(Sex.MALE) ? "м" : "ж";
    String date = parseDateForOutput(person.getBirthDate());

    System.out.println(person.getName() + " " + sex + " " + date);
}

private static String parseDateForOutput(Date date) {
    SimpleDateFormat simpleDateFormat = new SimpleDateFormat("dd-MMM-yyyy");
    return simpleDateFormat.format(date);
}

JustinianJudge в Mega City One

Master

26 июля 2019, 19:58

Как минимум не такой матрешкой. Код бы сбросить, прикрепи код - спрячь только в нем две - четыре ошибки чтобы он некорректно работал, индексы например, модификаторы, прояви творчество, чтобы код по форме остался таким каким он есть, но в пару незаметных мест была поломана логика. А то готовые решения нельзя, а если выложить и удалить, то неинтересно, так хоть другие почитают потом, для себя что-то возьмут, что-то дополнят, такие вопросы нужно оставлять. Так что давай в пост прилагай, не забывай о форматировании через тег < code > . по 1900 у меня есть ответ, но чувствую там еще много чего интересного есть )

Павел

Уровень 23

29 июля 2019, 08:51

приложил, готов объяснить, если что-то совсем не читаемо)