Вычислить день года

Я новичок в программировании. Я видел этот код Java с большим количеством if - else, которые копируются ниже. Люди говорят, что такой код называется «вонючим кодом», поэтому я написал свою собственную версию кода ниже. Может ли кто-нибудь помочь мне с любыми улучшениями в программе (связанные со стилем, программированием, форматированием или чем-либо еще)?

package dayOfYear;

/**
 *  
 * Two (smelly and not so smelly) ways to commute the day of the year,
 * when the day, month and year are entered by the user
 * 
 * @author TheProgrammer
 * 
 * Some code (mentioned below) taken from:
 * http://gsathish.github.io/eece210/a-Testing/
 * 
*/

import java.util.Scanner;

public class DayOfYear {
    public static final int JANUARY = 1, FEBRUARY = 2, MARCH = 3, APRIL = 4,
        MAY = 5, JUNE = 6, JULY = 7, AUGUST = 8, SEPTEMBER = 9, OCTOBER= 10,
        NOVEMBER = 11, DECEMBER = 12;

    public static final int MONTH_31 = 31, DAYS_IN_MONTH_31 = 31;
    public static final int MONTH_30 = 30, DAYS_IN_MONTH_30 = 30;
    public static final int MONTH_28 = 28, DAYS_IN_MONTH_28 = 28;

    public static final int ON = 1, OFF = 0;

    public static final int Month_length[]={31,28,31,30,31,30,31,
        31,30,31,30,31}; 

    public static void main(String[] args){

        int day, month, year, invalid_date_flag=OFF, month_type;
        Scanner in = new Scanner(System.in);
        do{ //do while loop to check the correctness of the entered date
            if(invalid_date_flag==ON)
                System.out.println("Invalid date. Please enter again.");

            invalid_date_flag=OFF;

            System.out.println("Enter month");
            month = in.nextInt();

            System.out.println("Enter day of the month");
            day = in.nextInt();

            System.out.println("Enter year");
            year = in.nextInt();

            if(month == FEBRUARY)
                month_type = MONTH_28;
            else if(month == JANUARY|| month == MARCH || month == MAY||
                    month == JULY || month == AUGUST || month == OCTOBER||
                    month == DECEMBER)
                month_type = MONTH_31;
            else
                month_type = MONTH_30;

            invalid_date_flag = 1;

        }while(month<JANUARY ||
                month>DECEMBER ||
                day<=0 ||
                (month_type == MONTH_31 && day > MONTH_31) ||
                (month_type == MONTH_30 && day > MONTH_30) ||
                (month_type == MONTH_28 && day > MONTH_28));        

        System.out.println(dayOfYear(month,day,year));
        in.close();
    }

    /*
     * Two (smelly and not so smelly) ways to commute the day of the    year.
     * @param month month of the year; requires month>0 and month<13
     * @param dayOfMonth day of the month
     * @param year year
     * @return the day of the year
     * For example. dayOfYear(2,1,1993)=32
     * */

    public static int dayOfYear(int month, int dayOfMonth, int year) {

        int dayOfMonth2=dayOfMonth;

        /*
         * Following if-else statements were taken from:
         * http://gsathish.github.io/eece210/a-Testing/
         * 
         * A better version of the same code is written below.
         */

        if (month == 2) {
            dayOfMonth += 31;
        } else if (month == 3) {
            dayOfMonth += 59;
        } else if (month == 4) {
            dayOfMonth += 90;
        } else if (month == 5) {
            dayOfMonth += 31 + 28 + 31 + 30;
        } else if (month == 6) {
            dayOfMonth += 31 + 28 + 31 + 30 + 31;
        } else if (month == 7) {
            dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30;
        } else if (month == 8) {
            dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30 + 31;
        } else if (month == 9) {
            dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30 + 31 + 31;
        } else if (month == 10) {
            dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30 + 31 + 31 + 30;
        } else if (month == 11) {
            dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30 + 31 + 31 + 30 + 31;
        } else if (month == 12) {
            dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30 + 31 + 31 + 30 + 31 + 30;
        }

        System.out.print("SmellyCode Answer: " + dayOfMonth +
                "\nNotSoSmellyCode Answer: ");

        /*Following is the better version (written by me) of the 'smelly code'
        above*/
        for( int month_index = FEBRUARY; month_index <= month; 
                month_index++ ) //months runs from FEBRUARY to month variable
            dayOfMonth2 += Month_length[month_index];   
        return dayOfMonth2;
    }
}
11 голосов | спросил TheProgrammer 28 J0000006Europe/Moscow 2015, 10:33:42

4 ответа


15

Добро пожаловать в программирование!

Пожалуйста, не расстраивайтесь количеством обратной связи, которую вы получите. Добро пожаловать и получайте удовольствие!

Рассмотрим обычную практику

Например, распространенная практика заключается в том, что месяц основан на нулевом значении. Я не знаю ни одного API, в котором один месяц. Все API, о которых я знаю, используют месяц на основе нуля. В вашем коде месяц один на один. Это удивительно.

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

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

Общепринятая практика - это POLA - Принцип наименьшего удивления (также известный как правило наименьшего удивления), как придумал Майк Коулислав в своей книге о языке программирования Rexx. Уорд Каннингем сформулировал это как «Вы знаете, когда читаете хороший код, когда все, что вы читаете, в значительной степени то, что вы ожидаете».

Следуйте стилю кода

Есть несколько второстепенных вещей:

  • Поместите пробел между ключевым словом и (, например if ( вместо if(.
  • Следуйте соглашениям об именах Java. Измените month_type на monthType и т. Д.

Это хорошая идея использовать среду IDE с хорошим форматированием исходного кода или украшением. Я рекомендую IntelliJ IDEA.

Favor boolean с помощью true и false over int с помощью ON и OFF.

И используйте предикаты для имен переменных и функций, которые содержат эти значения boolean.

Например, int invalid_date_flag должен быть boolean wasLastDateInvalid

if (wasLastDateInvalid)
    System.out.println("Invalid date. Please enter again.");

Избегайте двойного отрицания

Потому что двойное отрицание сделает его более трудным для читателей.

Это начинается с имен переменных. wasLastDateInvalid и !wasLastDateValid имеют одинаковую семантику. Но wasLastDateValid имеет преимущество перед wasLastDateInvalid

+--------------------+----------+---------------------+
|   Variable Name    |   case   |      expression     |
+--------------------+----------+---------------------+
|                    | negative | wasLastDateInvalid  |
| wasLastDateInvalid +----------+---------------------+
|                    | positive | !wasLastDateInvalid | Double Negation!
+--------------------+----------+---------------------+
|                    | negative | !wasLastDateValid   |
|  wasLastDateValid  +----------+---------------------+
|                    | positive | wasLastDateValid    |
+--------------------+----------+---------------------+

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

Используйте stderr not stdout для сообщения об ошибках.

Ваши сообщения об ошибках печатаются на stdout (Java: System.out). Это затрудняет автоматизацию. Сообщения об ошибках должны быть напечатаны на stderr вместо (Java: System.err).

Напишите небольшие методы

Например, код, определяющий количество дней в месяце, может быть извлечен в собственном методе. В идеале, методы с более чем 5 строками крайне редки. «Брекеты - это возможность извлечь». (Роберт «Дядя Боб» К. Мартин)

Сделать переменные final

Ну, вам не нужно писать final всюду в Java-коде. Да, другие нет. Дело в том, что не переписывайте переменные. Вместо этого напишите меньшие методы, в которых область действия переменной настолько мала, что вам не нужно повторно назначать переменную.

Используйте значащие имена переменных.

Я нашел метод dayOfYear запутанный в начале, пока не узнал, что переменная dayOfMonth на самом деле больше не содержит dayOfMonth, но dayOfYear

Я рекомендую, не переустанавливайте переменные с другой семантикой. Вместо этого выделите новую переменную для новойсемантика.

Сделать наиболее частым случаем по умолчанию /ухватить все.

В вашем коде

if (month == FEBRUARY)
    month_type = MONTH_28;
else if (month == JANUARY|| month == MARCH || month == MAY||
        month == JULY || month == AUGUST || month == OCTOBER||
        month == DECEMBER)
    month_type = MONTH_31;
else
    month_type = MONTH_30;

наиболее часто встречающийся случай, когда month_type = MONTH_31. Следовательно, это должна быть ветвь else. Тогда ваш код будет немного короче, например:

if (month == FEBRUARY)
    month_type = MONTH_28;
else if (month == APRIL || month == JUNE ||
        month == SEPTEMBER || month == NOVEMBER)
    month_type = MONTH_30;
else
    month_type = MONTH_31;

Заменить if или switch с помощью []

if и switch, которые сравните значение для равенства, которое легко конвертируется в индекс массива и назначает или запрашивает одно значение, которое может быть преобразовано в массивы.

Предполагая, что JANUARY есть 0 , следующий код

if (month == FEBRUARY)
    month_type = MONTH_28;
else if (month == JANUARY|| month == MARCH || month == MAY||
        month == JULY || month == AUGUST || month == OCTOBER||
        month == DECEMBER)
    month_type = MONTH_31;
else
    month_type = MONTH_30;

можно заменить на:

month_type = Month_length[month];

Ваша петля хороша (но ...)

Ваша петля - это именно то, как эту паршивую цепочку if-операторов следует заменить. Молодцы!

Просто ... вы уверены, что работаете с правильным индексом в массиве и получаете правильный результат? Индексы массива начинаются с нуля, а ваш FEBRUARY равен 2, поэтому вы фактически пропускаете 2 записи массива вместо 1. И если месяц декабрь (12), он будет бросать ArrayIndexOutOfBoundsException.

То есть, кстати, одна из причин, почему в большинстве API-интерфейсов month основана на нуле.

Итак, хотя ваш цикл хорош, он не работает и нуждается в модификации. Это ваша домашняя работа, чтобы исправить этот цикл. ;)

Проверьте свой код

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

В вашем случае вы можете сделать так называемое тестирование свинцово-золотого свинца . Вы можете создать два метода: dayOfYear(), оригинальный и новый, и вы можете проверить, дают ли оба метода одинаковые выходные значения для те же входные значения. Вывод оригинала называется gold , вывод вашего нового метода называется lead . Вы знаете, что ваш метод работает, если lead идентичен gold .

ответил Christian Hujer 28 J0000006Europe/Moscow 2015, 12:02:56
7

Рассмотрим следующее дополнение к отличным исходным данным @Christian Hujer's . :)

Резервный код и имена переменных

public static final int MONTH_31 = 31, DAYS_IN_MONTH_31 = 31;
public static final int MONTH_30 = 30, DAYS_IN_MONTH_30 = 30;
public static final int MONTH_28 = 28, DAYS_IN_MONTH_28 = 28;

Ваши переменные DAYS_IN_... не используются, поэтому они считаются резервным кодом duplicate , поскольку числа уже представленный как MONTH_31 /MONTH_30 / ---- +: = 4 =:. + ----

MONTH_28

В Java имена переменных часто находятся в public static final int Month_length[] = /* ... */; , поэтому это обычно записывается как camelCase

Валидация и високосные годы

Ваша достоверность основывается на этом:

monthLength

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

month<JANUARY || month>DECEMBER || day<=0 ||
    (month_type == MONTH_31 && day > MONTH_31) ||
    (month_type == MONTH_30 && day > MONTH_30) ||
    (month_type == MONTH_28 && day > MONTH_28)

А, к сожалению високосный год - это то, что вы не рассматривали в своей реализации . Пожалуйста, не забудьте сделать это за количество дней в феврале!

Эффективно использовать аргументы метода

// assuming still using your 1-based month-length array
boolean isValidDayAndMonth = month >= JANUARY && month <= DECEMBER 
                                && day > 0 && day <= monthLength[month];

public static int dayOfYear(int month, int dayOfMonth, int year) { // ... } вообще не используется. Это имеет смысл, так как без проверки високосного года вам не нужно знать точный год для решения вашей проблемы. Поэтому, если по какой-то причине вы с удовольствием решаете только даты non-leap-year , подпись этого метода должна быть упрощена как таковая:

year

Знайте свои библиотеки

Я понимаю, что вы начинаете Java, и у вас, вероятно, не было много времени, чтобы погрузиться в богатство Спецификация API Java, связанная с временем , а тем более хорошая Joda- Время . Однако, пожалуйста, найдите время, чтобы посмотреть, как такие вычисления на основе календаря могут быть сделаны гораздо проще с использованием API существующих библиотек, когда вам гораздо удобнее работать с Java и получать скорость. :)

ответил h.j.k. 28 J0000006Europe/Moscow 2015, 12:51:24
4

У вас есть еще один шаг - вместо массива длин месяцев, предварительно рассчитайте день года первого дня каждого месяца, вам вообще не нужен цикл.

Вы запрашиваете год, но не используете его для проверки на високосные годы.

Вы изменяете значение dayOfMonth, а затем возвращаете его как день года. Это разные вещи, и их лучше использовать в качестве отдельных переменных, если вы хотите поместить день года в переменную. Хотя у меня был бы только один лайнер:

public static int dayOfYear(int month, int dayOfMonth, int year) {
    return ( isLeapYear ( year ) ? MonthDayOffsetLeap : MonthDayOffset ) [ month ] + dayOfMonth;
}
ответил Pete Kirkham 28 J0000006Europe/Moscow 2015, 18:16:24
2

Первое правило написания кода даты /времени - это ... не делать этого! Есть ошеломляющие подводные камни . Другие рецензенты указали на очевидное упущение високосного года. (Обратите внимание, что 1700, 1800, 1900 и 2100 не являются високосные годы в соответствии с григорианским календарем.) Что менее очевидно, это частный случай год 1582 , в котором 31 декабря был 355-й день года во многих странах.

Следовательно, вы должны использовать стандартные библиотечные функции для обработки дат и времени. Я бы написал функцию таким образом, используя java.util.GregorianCalendar :

public static int dayOfYear(int month, int dayOfMonth, int year) {
    return new GregorianCalendar(year, month - 1, dayOfMonth).get(GregorianCalendar.DAY_OF_YEAR);
}

С Java 8 ответ: еще более простой :

public static int dayOfYear(int month, int dayOfMonth, int year) {
    return LocalDate.of(year, month, dayOfMonth).getDayOfYear();
}

Prof. Страница Гопалакришнана, из которой взят вонючий код, защищает обзор кода и модульное тестирование. Я рад, что вы делаете обзор кода, но вы не проводите модульное тестирование. (Вы можете визуально проверять вывод, что не является тем же самым, что и автоматическое тестирование.) Очень сложно выполнить код модульного тестирования, который использует System.out.print(), как вы это сделали.

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

public class DayOfYear {
    public static int dayOfYear(int month, int dayOfMonth, int year) {
        /*
         * Following if-else statements were taken from:
         * http://gsathish.github.io/eece210/a-Testing/
         */

        if (month == 2) {
            dayOfMonth += 31;
        } else if (month == 3) {
            dayOfMonth += 59;
        } else if (…) {
            …
        } else if (month == 12) {
            dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30 + 31 + 31 + 30 + 31 + 30;
        }
        return dayOfMonth;
    }
}

public class DayOfYearImproved extends DayOfWeek {
    public static final int JANUARY = 1, FEBRUARY = 2, MARCH = 3, APRIL = 4,
        MAY = 5, JUNE = 6, JULY = 7, AUGUST = 8, SEPTEMBER = 9, OCTOBER= 10,
        NOVEMBER = 11, DECEMBER = 12;

    private static final int[] MONTH_LENGTH = {31,28,31,30,31,30,31,31,30,31,30,31};

    public static int dayOfYear(int month, int dayOfMonth, int year) {
        int d = dayOfMonth;
        for( int month_index = FEBRUARY; month_index <= month; 
                month_index++ ) //months runs from FEBRUARY to month variable
            d += MONTH_LENGTH[month_index];
        assert(d == DayOfYear.dayOfYear(month, dayOfMonth, year));
        return d;
    }
}

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


Вы опустили фигурные скобки из цикла for. Меня не волнует, когда вы кладете фигурные скобки (если это согласовано), но код хорошего качества всегда должен включать «необязательные» брекеты. Путем опускания брекетов вы, вероятно, вносите свой вклад в будущую ошибку кодирования, и она будет всей вашей ошибкой .


Вместо

public static final int Month_length[] = { … };

вам следует написать

private static final int MONTH_LENGTH[] = { … };

, потому что

  • Код в других классах может перезаписывать содержимое массива , несмотря на то, что массив final. final просто гарантирует, что Month_length не может быть позже изменилось, чтобы указать на совершенно новый массив. Сделать это частным означает, что только код в этом классе имеет доступ к массиву, и, конечно же, вы будете осторожны, чтобы не мусорный массив.
  • Константы должны быть записаны в ALL_CAPS.
ответил 200_success 3 J000000Friday15 2015, 10:57:08

Похожие вопросы

Популярные теги

security × 330linux × 316macos × 2827 × 268performance × 244command-line × 241sql-server × 235joomla-3.x × 222java × 189c++ × 186windows × 180cisco × 168bash × 158c# × 142gmail × 139arduino-uno × 139javascript × 134ssh × 133seo × 132mysql × 132