problem1.4: замечания

[GoogleCodeExporter]

На будущее, не надо делать то, чего не 
требуется. Стоило обойтись вектором в 
качестве контейнера. Но выглядит вроде 
ничего, так что проверю.

Убери комментарии с include "....cpp".

big_int.h:
* Не надо делать using в заголовочном файле. 
Если кто-то подключит этот файл, то он не 
сможет скрыть то, что ты подключил, из 
области видимости.
* Я бы делал константы base и base_length 
статическими полями класса big_int.
* Лучше бы сделать конструктор от digit_t explicit, 
хотя с этим можно поспорить.
* Почему бы не сделать дефолтное значение 
для конструктора от цифры равное нулю, 
тогда автоматически получится хороший 
дефолтный конструктор?
* norm лучше перименовать в normalize, т.к. norm может 
быть нормой.
* Непонятно, что ты хотел сказать делая void 
swap(big_int& a, big_int& b) и void norm(container& a, size_t&) const 
методами.

big_int.cpp:
* Почему не всегда используешь списки 
инциализации?
* Зачем в первых двух конструкторах 
вызывается нормализация?
* Какой-то неинтеллектуальный swap. Почему бы 
там не использовать своп контейнеров?
* Что-то в operator>> расстановка фигурных 
скобок не такая, как везде.
* Почему бы не выделить конструктор от 
строки?
* "for (len = 0; now2 > 9; now2 /= 10, len++);" — не надо так 
писать, хотя бы перенеси ; на следующую 
строчку, чтоб было понятно,что там 
специально ничего не делается.
* Что-то я не понял, зачем нужно два norm'а. 
Почему бы не оставить один, который без 
параметров.
* Непонятно, что это за магия и зачем она:
        if (i % 8 == 0)
            norm(res, tmp);
* Если я правильно понимаю, то рассмотрение 
этих частных случаев не нужно:
    if(*this < b)
        return *this = 0;
    if(*this < b * 2)
        return *this = sign ? -1 : 1;
* Что за магия с 3 / 4?

container.h:
* Конструктор от size_t точно надо сделать 
explicit.
* using
* norm, которй, к тому же и не определен, что 
намекает на то, что его надо выкинуть.
* Почему operator= не делает удаление массива? К 
тому же, не будет работать a = a.
* Удалять надо не с помощью delete, а с помощью 
delete[].
* Плохой swap, он ничем не лучше стандартного. 
Твой своп работает за O от размеров, а можно 
это делать за O(1).
* Непонятно зачем нужен такой resize. Посмотри, 
например, что делает resize у вектора.
* Не надо использовать C-style приведение 
типов. Используй static_cast.

Original issue reported on code.google.com by alsergbox on 20 May 2011 at 1:17

[GoogleCodeExporter]

Всё поправлено, кроме:

* Зачем в первых двух конструкторах 
вызывается нормализация?
# Потому что могут вызвать конструктор от 
цифры, которая больше базы

* Что-то я не понял, зачем нужно два norm'а. 
Почему бы не оставить один, который без 
параметров.
# Без параметров - "починить число"
С параметрми(теперь он static) - "починить 
контейнер"

* Непонятно, что это за магия и зачем она:
        if (i % 8 == 0)
            norm(res, tmp);
# Защита от переполнения long long-а. За 8 
итераций ещё не успевает перепониться, а за 
9 - успевает

* Что за магия с 3 / 4?
# Это нужно для реализации деления. Для 
этого алгоритма важно, чтобы первая цифра 
делителя была больше половины базы. 
Заменил на менее магическое 1 / 2

* Непонятно зачем нужен такой resize. Посмотри, 
например, что делает resize у вектора.
# Вроде бы нормальный resize - поменять 
вместимость контейнера. Особенно с учётом 
той особенности, что при обращении к 
несуществующему элементу контейнера, 
возвращается 0.

Original comment by tau...@gmail.com on 22 May 2011 at 9:11

• Changed state: Fixed

[GoogleCodeExporter]

Да, и по поводу замечания про то, что не надо 
было в 1.4 писать свой контейнер. У меня в 2.1 и 
в 1.4 файлы big_int.* и container.* физически одни и те 
же, так что проверять как раз придётся 
меньше. 

Original comment by tau...@gmail.com on 22 May 2011 at 9:15

[GoogleCodeExporter]

* Не надо смешивать пробелы и табы для 
отступов. Необходимо придерживатся строго 
одного стиля (лучше с пробелами), иначе у 
кого-нибудь что-нибудь поедет и это будет 
невозможно читать. В гуглокоде, где таб — 8 
пробелов, — невозможно. В main.cpp и big_int.h.
* Цифра по определению не может быть больше 
основания. Значит надо сделать конструктор 
от int, а не от digit_t.
* Непонятно, что за len. Длина чего? И мне не 
нравится, что приходится постоянно таскать 
ее с собой. Лучше бы хранить в контейнере 
только то, что надо.
* Я бы сделал конструкторм от контейнера 
private — снаружи должно быть не особо важно 
что у него внутри.
* swap интеллектуальнее не стал. У тебя есть 
конейнер, у которого swap может работать 
очень быстро (просто swap указателей), 
поэтому целесообразно реализовать и 
быстрый swap для big_int. Надо просто посвопать 
все поля с помощью std::swap.
* Замечание про "for (len = 0; now2 > 9; now2 /= 10, len++);" 
проигнорировано.
* Я не понимаю, зачем отдельный normalize для 
контейнера.
* «Защита от переполнения long long-а. За 8 
итераций ещё не успевает перепониться, а за 
9 - успевает» — очень хочется выругаться. 
Во-первых, в коде не должно быть магических 
констант, особенно настолько магических. 
Во-вторых, такое обязательно надо 
комментировать.
* Что за friend big_int operator-(big_int);? big_int operator-(); уже 
не катит?

Original comment by alsergbox on 4 Jun 2011 at 9:14

• Changed state: Accepted

[GoogleCodeExporter]

* Не надо смешивать пробелы и табы для 
отступов. Необходимо придерживатся строго 
одного стиля (лучше с пробелами), иначе у 
кого-нибудь что-нибудь поедет и это будет 
невозможно читать. В гуглокоде, где таб — 8 
пробелов, — невозможно. В main.cpp и big_int.h.
# исправлено

* Цифра по определению не может быть больше 
основания. Значит надо сделать конструктор 
от int, а не от digit_t.
# исправлено

* Непонятно, что за len. Длина чего? И мне не 
нравится, что приходится постоянно таскать 
ее с собой. Лучше бы хранить в контейнере 
только то, что надо.
* Я не понимаю, зачем отдельный normalize для 
контейнера.
# проблема в том, что при реализации длина 
длинного числа, и длина контейнера могут не 
совпадать. От этого можно избавиться, 
поменяв бОльшую часть кода, и сделав больше 
ресайзов вектора, что замедлит программу. 
То есть, len --- длина длинного числа, и она 
может в некоторые моменты не совпадать с 
размером вектора. Также, иногда при 
выполнении вычислений модифицируется 
непосредственно вектор, поэтому нужна 
нормировка вектора. 

* Я бы сделал конструкторм от контейнера 
private — снаружи должно быть не особо важно 
что у него внутри.
# а вроде так и было, теперь точно так

* swap интеллектуальнее не стал. У тебя есть 
конейнер, у которого swap может работать 
очень быстро (просто swap указателей), 
поэтому целесообразно реализовать и 
быстрый swap для big_int. Надо просто посвопать 
все поля с помощью std::swap.
# теперь стал

* Замечание про "for (len = 0; now2 > 9; now2 /= 10, len++);" 
проигнорировано.
# не заметил тогда :(

* «Защита от переполнения long long-а. За 8 
итераций ещё не успевает перепониться, а за 
9 - успевает» — очень хочется выругаться. 
Во-первых, в коде не должно быть магических 
констант, особенно настолько магических. 
Во-вторых, такое обязательно надо 
комментировать.
# теперь эта константа не такая магическая

* Что за friend big_int operator-(big_int);? big_int operator-(); уже 
не катит?
# исправлено

Original comment by tau...@gmail.com on 16 Jun 2011 at 8:29

• Changed state: Fixed

[GoogleCodeExporter]

* Непонятно, что за len. Длина чего? И мне не 
нравится, что приходится постоянно таскать 
ее с собой. Лучше бы хранить в контейнере 
только то, что надо.
* Я не понимаю, зачем отдельный normalize для 
контейнера.
# проблема в том, что при реализации длина 
длинного числа, и длина контейнера могут не 
совпадать. От этого можно избавиться, 
поменяв бОльшую часть кода, и сделав больше 
ресайзов вектора, что замедлит программу. 
То есть, len --- длина длинного числа, и она 
может в некоторые моменты не совпадать с 
размером вектора. Также, иногда при 
выполнении вычислений модифицируется 
непосредственно вектор, поэтому нужна 
нормировка вектора. 
* Про то, что замедлит — это спорный 
аргумент, это надо измерять, но ОК. Назови 
только тогда это поле как-нибудь, чтоб оно 
больше отражало суть. Что-нибудь типа 
используемого размера, или еще как-нибудь.

Original comment by alsergbox on 16 Jun 2011 at 8:48

[GoogleCodeExporter]

Хотя ладно, и так нормально.

Original comment by alsergbox on 18 Jun 2011 at 1:25

• Changed state: Verified