Это говнокод или норма JS?

Question

[medianoche]

Если программист пишет такой JS код:

money = 1500;

var strMoney = money.toString();
var endNum = strMoney.substr(strMoney.length - 1); // последняя цифра числа
var preEndNum = strMoney.slice(strMoney.length - 2, -1); // предпоследняя цифра числа

var moneyName = "рублей";
if (endNum == 1 && preEndNum != 1) {
	moneyName = "рубль";
} else if (endNum >= 2 && endNum <=4 && preEndNum != 1) {
	moneyName = "рубля";
}

1. То, что можно сказать о качестве его работы? Это говнокод и норма?
2. Если программист заморачивается такими вещами, вместо того чтобы выводить «руб.», то это характеризует его с положительной или отрицательной стороны?

Answer 1

[Марк]

1. код плохой, я на кодревью отправляю переписывать такое
2. начинание хорошее, надо похвалить (но все равно заставить переделать нормально)

Comments

[medianoche]

Очень много противоречивых ответов. Один говорит, что не надо использовать var (я не прогер конечно, но всегда думал что засорение глоб.видимости - зло), другой говорит юзать библиотеку на 100кб. Если коротко, то чем плох код? Как бы вы его переписали?

[Марк]

zapolnoch: да var тут ни при чем, алгоритм страшный очень. Дано ведь число, нафига его переводить в строку, а потом назад в число, но по одной цифре?

Без ненужных операций со строками за три минуты написалось что-то такое вот: jsfiddle.net/mvasilkov/zce03Lwn

[veitmen]

Марк: из разряда: как удалить две ненужные переменные :)

[Марк]

veitmen: тут не ненужные переменные, а бессмысленные преобразования типов данных, и математика вида «вторая с конца цифра равна одному». Это довольно безумный способ задать числовой отрезок, запись вида 10 ≤ x < 20 лучше же.

[Марк]

zapolnoch: ну так я о том же, к числам нафиг не нужно применять substr и slice. В алгебре нет substr, потому что такая операция совершенно не нужна для вычислений.

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

Это довольно распространенное явление: когда в руках молоток, везде мерещатся гвозди. В эту же категорию ошибок можно отнести, например, разбор HTML регулярными выражениями. Такие решения заведомо субоптимальны.

[Марк]

zapolnoch: кстати да, забыл написать: применить-то можно. Например, вот так:

String.prototype.substr.call(123, 1, 1)

Такое коммитят приблизительно за день до увольнения (а если в пятницу вечером, то за три дня).

[medianoche]

Марк: вот теперь понял)

Answer 2

[Юрий Ярош]

1. Это код человека не удосужившегося поискать любую существующую библиотеку интернационализации, либо ознакомившегося с Intl'ом для которого есть вполне вменяемые полифилы.
2. Это характеризует его как человека который хочет делать "хорошо", а получается "как всегда" из-за недостатка знаний и опыта.

Comments

[Dartess]

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

Answer 3

[Алексей Николаев]

С алгоритмической точки зрения и некоторых других моментов (например, постоянное использование var) скорее говнокод, чем нет, приемлемого среднего уровня 5 \ 10. Но по этому куску трудно судить о всём коде в целом - он слишком прост.

это характеризует его с положительной или отрицательной стороны?

Если в макете \ ТЗ было четко написано "рублей", то это характеризует его как человека, внимательного к мелочам. Это положительное качество.

P.S. Библиотеки для склонения числительных существительных в данном случае избыточны.

Comments

[Константин Китманов]

Чем плохо использование var и где вы тут видите склонение числительных?

[Алексей Николаев]

Константин Китманов: избыточно. Можно написать var один раз вместо четырёх.

Насчет термина "числительные" опечатался - ночь, как никак. Имелось в виду склонение существительных.

[Константин Китманов]

Алексей Николаев: ну да, я тоже так писал когда-то:)

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

[Kir ---]

Алексей Николаев: за 1 написание var вместо четырёх, как раз руки отрывать надо. Потому что очень не удобно отлаживать такой код. Дебаггер останавливается на первой конструкции и если нажать перешагнуть через конструкцию, то он перепрыгнет через остальные объявления.
Так же это плохо тем, что при редактировании кода одна из самых распространённых операций это дублирование строки и/или перенос её в другое место. С одним оператором var рефакторинг становится очень не удобным.

[Kir ---]

OnYourLips: может стоит:
а) F10 гораздо быстрее чем клик а потом F8
б) понимать что брейкпоинт в случае с одним var все равно НЕ ставится

в) от проблемы рефакторинга никуда не уйти

советчики нашлись...

[Юрий Ярош]

Эм, есть JSCS, зачем спорить о том чего много или мало ?
ИМХО я пока пишу 1 var в require'aх, а в остальных случаях "много" var'ов.

[OlegTar]

с каких пор постояное использование var стало говнокодом?

Answer 4

[Виталий Пухов]

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

Comments

[65536]

охватывающий код выглядит так

function ending(number, one, two, five)
{
    var ending;

    number = number % 100;

    if ((number > 4 && number < 21) || number == 0)
    {
        ending = five;
    }
    else
    {
        var last_digit = substr(number, -1);

        if (last_digit > 1 && last_digit < 5)
        {
            ending = two;
        }
        else
        {
            if (last_digit == 1)
                ending = one;
            else
                ending = five;
        }
    }

    return ending;
}

[65536]

money + ' ' + ending(money, 'рубль', 'рубля', 'рублей');

или

money + ' рубл' + ending(money, 'ь', 'я', 'ей');

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

[medianoche]

65536: по-моему, ваш код делает те же самые проверки, только расписан излишне подробно. Например, какой смысл проверять "number > 4 && number < 21", если это решается проверкой предпоследней цифры ("preEndNum != 1" в первом сообщении). Или не?

[65536]

zapolnoch: да пашет его код, проверил. обычно с этим окончанием всегда какая-нибудь проблема)

[Юрий Ярош]

Я б за такое уволил, из-за того что человек не удосужился поискать что-то готовое, изучить и использовать.

[Виталий Пухов]

Юрий Ярош: Не совсем согласен, если нужно всего навсего подставить "лей" и на это потратить пару операций процессора это нормально, а если для тех же "лей" подтягивать громозкою библиотеку лексико-морфического анализа и тратить Епическое колическое ресурсов это не правильно, зато готовое и все такое

[Юрий Ярош]

Виталий Пухов: а если эта библиотека уже часть ECMA стандарта и уже встроена во многих браузерах и в node.js/io.js ?

[Виталий Пухов]

Юрий Ярош: если она уже встроена "во все" браузеры то безусловно вы правы, но это сомнительно и проверить встроена она в ту или иную версию IE будет тот еще веселый процесс. Что касательно node.js и ему подобных то согласен, опять же при условии, что они уже включены в проекте, в противном случае ради только этой мелкой функции подгружать такие тяжелые библиотеки неправильно.

Answer 5

[Денис Инешин]

А ведь мог просто написать "руб." ) так что сойдет.

Answer 6

[OlegTar]

1. Не говнокод (я прогнал тесты, и всё проходит), но я бы переписал на проверку остатка от деления, а не на проверку цифр в числе.
2. Положительно, внимателен к мелочам