Оцените код, пожалуйста! (читабельность, наглядность и т.д.)

← →
Lip (2008-01-04 03:33) [0]

Написал функцию перевода чисел в разных системах счисления. Имеется ввиду позиционная система счисления с основанием от 2 до 36.
Прокомментируйте, пожалуйста, код, а точнее такие параметры как, читабельность, наглядность, ошибки, комментарии (обилие, непонятность, недостаточность, неправильность) и прочие недочеты.
Заранее очень благодарен!
function Convert(Number: string; const InBase, OutBase: Integer): string; var DecValue, i, k, j : Integer; // Number - исходное число OutBaseValue: string; // InBase - основание в котором находится Number begin // OutBase - основание в которое переводим Number Result := ""; // Переводим все символы Number в верхний регистр for i := 1 to Length(Number) do Number[i] := UpCase(Number[i]); // Проверка корректности переданных параметров (Number, InBase, OutBase) if (InBase < 2) or (InBase > 36) or (OutBase < 2) or (OutBase > 36) then Exit; for i := 1 to Length(Number) do if not (Number[i] in ["0".."9", "A".."Z"]) then Exit; // Переводим Number из основания InBase в 10 систему счисления k := 1; DecValue := 0; for i := Length(Number) downto 1 do begin j := 48; while (Number[i] <> Char(j)) and (j <= 90) do Inc(j); if (Number[i] in ["0".."9"]) then Dec(j, 48) else Dec(j, 55); DecValue := DecValue + k * j; k := k * InBase; end; // Переводим Number из основания 10 в систему счисления c основанием OutBase while (DecValue <> 0) do begin k := DecValue mod OutBase; DecValue := DecValue div OutBase; if k < 10 then Inc(k, 48) else Inc(k, 55); OutBaseValue := Char(k) + OutBaseValue; end; Result := OutBaseValue; end;

← →
Дуболом (2008-01-04 08:41) [1]

1. 23+10=33<>36
2. Почему получение из стринга числа, названо переводом в 10-ую систему?
3. Ну, произошел выход при неверных данных и чего дальше?
4. j := 48; while (Number[i] <> Char(j)) and (j <= 90) do Inc(j); if (Number[i] in ["0".."9"]) then Dec(j, 48) else Dec(j, 55);

в мемориз.
5. Для начала хватит.

← →
Zeqfreed © (2008-01-04 09:28) [2]

> j := 48;

Магическое число, да еще и не прокомментированное.

> while (Number[i] <> Char(j)) and (j <= 90) do
> Inc(j);

Это называется индусский код? :)

← →
Zeqfreed © (2008-01-04 09:35) [3]

А, кстати. Я бы функцию назвал как минимум BaseConvert. А то прямо универсальный конвектор получается :)

← →
MsGuns © (2008-01-04 10:34) [4]

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

← →
Zeqfreed © (2008-01-04 11:02) [5]

> MsGuns © (04.01.08 10:34) [4]

Ну, в системах счисления с основанием больше 10 цифры записываются буквами латинского алфавита, которые могут быть как в нижнем, так и в верхнем регистре.

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

← →
Virgo_Style © (2008-01-04 11:08) [6]

Lip (04.01.08 3:33)
for i := 1 to Length(Number) do
Number[i] := UpCase(Number[i]);

Хм... я что-то пропустил в этой жизни?

← →
Dimaxx © (2008-01-04 11:15) [7]

Number:=UpperCase(Number);

← →
Kolan © (2008-01-04 11:15) [8]

Как и часто это бывает комменты тут  это дезодарант, который набрызгали на немытые подмышки&#133

Все четыре прокомментированых блока кондидаты на ExtractMethod.

> Переводим все символы Number в верхний регистр

Вообще идиотизм, способствующий глоб. потоплению&#133

← →
uw © (2008-01-04 11:36) [9]

OutBaseValue не инициализирована.

← →
DiamondShark © (2008-01-04 14:01) [10]

Проверка на допустимость аргументов не полная.
Например, число ZZZZZZZ(33) уже не не влезет в Integer, который используется для промежуточного представления числа, и функция вернёт мусор.

← →
Riply © (2008-01-04 14:22) [11]

> [0] Lip (04.01.08 03:33)
Не обращай внимания на "подкалывания" - они не со зла :)
Для начала, не очень уж и плохая ф-ия.
Попробуй учесть все замечания и переписать ее заново.

← →
{RASkov} © (2008-01-04 15:29) [12]

> OutBaseValue не инициализирована.

> [0] Lip (04.01.08 03:33)

Она вообще там не нужна.... работай с Result.....
и почему многие пренебрегают Result"ом, а в конце делают "тупое" присвоение ....? :)

← →
{RASkov} © (2008-01-04 15:36) [13]

> и почему многие пренебрегают Result"ом, а в конце делают
> "тупое" присвоение ....? :)

Ну на крайняк, может кому-то именно название переменной ненравится, хотя бы так:

function Convert(Number: string; const InBase, OutBase: Integer): string; var DecValue, i, k, j : Integer; // Number - исходное число OutBaseValue: string absolute Result; begin //Result:=""; // Вместо этого OutBaseValue:=""; ......... //Result := OutBaseValue; - Это уже не нужно end;

ЗЫ [12] и этот - это дополнение к остальным "недочетам" :)

← →
Lip (2008-01-04 15:51) [14]

> 2. Почему получение из стринга числа, названо переводом
> в 10-ую систему?

Потому что это название отражает суть действия.

> 3. Ну, произошел выход при неверных данных и чего дальше?

И строка окажется пустой. Может быть передавать какой -то флаг в параметрах? Посоветуйте, пожалуйста, что сделать лучше.

> 1. 23+10=33<>36

а почему именно 23??

> чем отличаются цифры на верхнем регистре от них же на нижнем
> ?

Там могут быть не только цифры. Не хочется делать лишних проверок.

> Number:=UpperCase(Number);

Учту. Не знал о существовании такой функции.

> Все четыре прокомментированых блока кондидаты на ExtractMethod.

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

> Магическое число, да еще и не прокомментированное.

Я думал над этим. Решал оставить как есть. Посчитал что число 48 известно в обществе программистов.

> > Переводим все символы Number в верхний регистрВообще идиотизм,
> способствующий глоб. потоплению…

С чего бы это?

> OutBaseValue не инициализирована.

Учту.

> Не обращай внимания на "подкалывания" - они не со зла :)Для
> начала, не очень уж и плохая ф-ия.Попробуй учесть все замечания
> и переписать ее заново.

Спасибо. Так и сделаю!

← →
Lip (2008-01-04 16:12) [15]

Написал новую версию. Прокомментируйте, пожалуйста!
function BaseConvert(Number: string; const InBase, OutBase: Integer; var Error: Boolean): string; var DecValue, i, k, j : Integer; // Number - исходное число begin // OutBase - основание в которое переводим Number Result := ""; // InBase - основание в котором находится Number Error := True; Number := UpperCase(Number); // Проверка корректности переданных параметров (Number, InBase, OutBase) if (InBase < 2) or (InBase > 36) or (OutBase < 2) or (OutBase > 36) then Exit; for i := 1 to Length(Number) do if not (Number[i] in ["0".."9", "A".."Z"]) then Exit; for i := 1 to Length(Number) do if Number[i] in ["0".."9"] then begin if Ord(Number[i]) - 48 >= InBase then Exit; end else if Number[i] in ["A".."Z"] then begin if Ord(Number[i]) - 55 >= InBase then Exit; end; Error := False; // Переводим Number из основания InBase в 10 систему счисления k := 1; DecValue := 0; for i := Length(Number) downto 1 do begin j := 48; while (Number[i] <> Char(j)) and (j <= 90) do Inc(j); if (Number[i] in ["0".."9"]) then Dec(j, 48) else Dec(j, 55); DecValue := DecValue + k * j; k := k * InBase; end; // Переводим Number из основания 10 в систему счисления c основанием OutBase while (DecValue <> 0) do begin k := DecValue mod OutBase; DecValue := DecValue div OutBase; if k < 10 then Inc(k, 48) else Inc(k, 55); Result := Char(k) + Result; end; end;

Правда магические числа 48, 55, 90 самому до сих пор не нравятся.

← →
Kolan © (2008-01-04 16:25) [16]

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

Что это у тебя за ситуация?

Тут:
(InBase < 2) or (InBase > 36) or (OutBase < 2) or (OutBase > 36)
И тут:
Number[i] in ["0"&#133"9"

Дублирующийся код. Лечится опять таки выделением метода&#133

А почему комментарии:
// Number  исходное число // OutBase  основание в которое переводим Number // InBase  основание в котором находится Number
Комментируют не те строки к которым относятся?

> Правда магические числа 48, 55, 90 самому до сих пор не
> нравятся.

Замени на константы с понятными именами&#133

← →
DiamondShark © (2008-01-04 16:28) [17]

> > 2. Почему получение из стринга числа, названо переводом
> > в 10-ую систему?
>
> Потому что это название отражает суть действия.

Суть действия -- это перевод числа в Integer. А это никак не 10-я система.

> > 3. Ну, произошел выход при неверных данных и чего дальше?
>
> И строка окажется пустой. Может быть передавать какой -то
> флаг в параметрах? Посоветуйте, пожалуйста, что сделать
> лучше.

Исключение возбуждать.

> > 1. 23+10=33<>36
>
> а почему именно 23??

Это тебя обманули. Латинских букв, всё-таки, 26.

> > чем отличаются цифры на верхнем регистре от них же на
> нижнем ?
>
> Там могут быть не только цифры. Не хочется делать лишних
> проверок.

Ты не понял. Цифры -- это вообще всё, что используется для записи числа.
В числе 12ABC(16) A, B, C -- тоже цифры. Так вот, чем 123abc отличается от 12ABC? Ничем.

> > > Переводим все символы Number в верхний регистрВообще
> идиотизм,
> > способствующий глоб. потоплению…
>
> С чего бы это?

Лишние действия, лишние такты процессора, лишние джоули высокоэнтропийного тепла.

Посмотри на коды заглавных и строчных латинских букв. Заметь, что коды соответсвующих букв отличаются одним битом:
"A" = 0x41 "a" = 0x61
"B" = 0x42 "b" = 0x62
...
"Z" = 0x5A "z" = 0x7A

DecValue := 0;
for i := Length(Number) downto 1 do
begin
if Number[i] <= "9" then j := Ord(Number[i]) - Ord("0");
else j := Ord(Number[i]) and $DF - Ord("A") + 10;
DecValue := DecValue * InBase + j;
end;

А преобразование регистра выкинуть нафиг.

← →
DiamondShark © (2008-01-04 16:32) [18]

> function BaseConvert(Number: string; const InBase, OutBase:
> Integer; var Error: Boolean): string;

Неудобная сигнатура.
Либо выкинуть булевский флаг, а при ошибке кидать исключение (см. поведение библиотечных функций форматирования), либо заменить на

function BaseConvert(Number: string; const InBase, OutBase: Integer; var OutNumber: String): Boolean;

Так удобнее пользоваться.

← →
Lip (2008-01-04 16:34) [19]

> Дублирующийся код. Лечится опять таки выделением метода…

Да не хочется писать новый метод, потому что уже функция BaseConvert не будет работать автономно без этих доп. функций. А описывать новые функции внутри BaseConvert - дурной тон.

> Комментируют не те строки к которым относятся?

Ну куда их тогда написать?
сверху параметров функции BaseConvert

← →
Lip (2008-01-04 16:38) [20]

> Суть действия -- это перевод числа в Integer. А это никак
> не 10-я система.

А в Integer может храниться число в 20 СС? Когда ещё используется буквы A, B, C, D...

> Это тебя обманули. Латинских букв, всё-таки, 26.

Меня попытались обмануть, но я то знаю что пишу...

← →
{RASkov} © (2008-01-04 16:38) [21]

> [19] Lip (04.01.08 16:34)
> Ну куда их тогда написать?
> сверху параметров функции BaseConvert

Излишний комментарий - тоже не есть хорошо :)
Помоему все параметры и так понятны..... без комментария)

Комментировать нужно именно непонятные участки кода, а не все подряд.... имхо)

← →
DiamondShark © (2008-01-04 16:39) [22]

> Правда магические числа 48, 55, 90 самому до сих пор не
> нравятся.

Ну так и напиши вместо них
Ord("0"), Ord("A") - 10, а 90 нафиг вообще не нужно.
Не бойся, компилятор константные выражения сам рассчитает и на числа заменит.

← →
Lip (2008-01-04 16:40) [23]

> function BaseConvert(Number: string; const InBase, OutBase:
> Integer; var OutNumber: String): Boolean;

А вот читал, что функция должна возвращать только одно значение, то есть через параметром желательно чтобы ничего не передавалось через var

← →
DiamondShark © (2008-01-04 16:49) [24]

> А в Integer может храниться число в 20 СС?

В Integer (как в типе данных) хранится число. Просто число.
В машинном представлении (на большинстве землянских машин) Integer хранится как двоичное число. На каком-нибудь инопланетянском компьютере с троичной логикой Integer будет храниться в памяти как троичное число. И т.д.
СС -- это способ записи числа (в памяти, на бумаге и т.п.)

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

← →
DiamondShark © (2008-01-04 16:52) [25]

> Lip (04.01.08 16:40) [23]
>
> А вот читал, что функция должна возвращать только одно значение,
> то есть через параметром желательно чтобы ничего не передавалось
> через var

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

← →
{RASkov} © (2008-01-04 16:57) [26]

> [21] {RASkov} © (04.01.08 16:38)

А вообще можно вот так
{function BaseConvert(Number: string; const InBase, OutBase: Integer; var OutNumber: String): Boolean; * Функция конвертирования строкового представления числа между разными системами счисления Вернет False при какой либо ошибки Number - исходное число InBase - основание в котором находится Number OutBase - основание в которое переводим Number OutNumber - результат конвертации //* Эту строку я может и не верно написал, я в этих терминах как козел в ананасах :)} function BaseConvert(Number: string; const InBase, OutBase: Integer; var OutNumber: String): Boolean; var DecValue, i, k, j : Integer; begin

> [23] Lip (04.01.08 16:40)
> что функция должна возвращать только одно значение,

функция более одного результата вернуть не сможет
Вот смотри, при успешном выполнении функции if BaseConvert(..., SOutInt) then мы в SOutInt имеем преобразованное "число"....

ЗЫ Именно такой вариант заголовка функции, предложенный [18] DiamondShark ©, выглядит более менее красиво на мой взгляд.... Опять имхо)

← →
{RASkov} © (2008-01-04 17:18) [27]

> [23] Lip (04.01.08 16:40)
> А вот читал, что функция должна возвращать только одно значение,
> то есть через параметром желательно чтобы ничего не передавалось
> через var

А посмотри API"шные функции.... они "через одну" именно такие(и результат и параметры указателями)...

← →
Lip (2008-01-04 17:59) [28]

> j := Ord(Number[i]) and $DF - Ord("A") + 10;

А это что?
в частности это: Ord(Number[i]) and $DF

← →
koha overload (2008-01-04 18:05) [29]

> [0]
> наглядность

Ты, что егона выставку собрался.......

Мда.....

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

Интернет как жизнь - смысла нет а уходить не хочеться.

← →
Lip (2008-01-04 18:13) [30]

> Ты, что егона выставку собрался.......

Нет! Чтобы не delphimaster выложить было не стыдно!

> [30] Lip (04.01.08 18:13)
> Нет! Чтобы не delphimaster выложить было не стыдно!

:)

> Lip (04.01.08 17:59) [28]
>
> > j := Ord(Number[i]) and $DF - Ord("A") + 10;
>
>
> А это что?
> в частности это: Ord(Number[i]) and $DF

Это вычисление значения цифры.
В частности, Ord(Number[i]) and $DF -- это быстрый аналог UpCase

> DiamondShark © (04.01.08 19:04) [32]
> В частности, Ord(Number[i]) and $DF -- это быстрый аналог UpCase

Насколько быстрее и насколько аналог? :)))

> Оцените код, пожалуйста! (читабельность, наглядность и т.д.)

> Ord(Number[i]) and $DF -- это быстрый аналог UpCase

:o)

← →
Lip (2008-01-04 23:14) [35]

Вот спрашивается что нагляднее?
Писать Ord(Number[i]) and $DF или UpCase(Number[i])

Код должен быть максимум понятным. А так сиди и думай, что такое and $DF

> Lip (04.01.08 23:14) [35]

Наверное, это был пример комментария.
:o)

> {RASkov} (04.01.2008 17:18:27) [27]

АПИ не агрумент, у него длинная запутанная история и необходимость взаимодействовать с любым языком.

> Johnmen (04.01.2008 19:43:33) [33]

Чуть чуть быстрее и совсем не аналог

>[15]
Не лучше ли переписать второй абзац, например так:

for i := 1 to Length(Number) do if ((Number[i] in ["0".."9"]) and (Ord(Number[i])-48<InBase)) or ((Number[i] in ["A".."Z"]) and (Ord(Number[i])-55<InBase)) then else exit;

А почему это ф-ции не положено через параметр ничего возвращать? Ведь поднимать исключение почем зря тоже не дело. Ведь тогда ф-цию придется засовывать в защищенный блок, что явно не удобно. Вот я бы её так и сделал булевской через параметр результат возвращающей. И пользвоваться удобно:
If convert (..., Str) then
ShowMessage(Str)
else
ShowMessage("Что же ты, гнида, пишешь?!!!!");
Почему это плохо писать локальные ф-ции? Их что же, зря придумали? КАк раз нормально: повторяющийся код в локальные, а сама ф-ция самодостаточной получается, а так ей бы потребовались какие-то внешние. А потом гадай, зачем они, может, они ещё кому-то нужны. А локальная сразу видно где используется.

Оцените код, пожалуйста! (читабельность, наглядность и т.д.) Найти похожие ветки