Прошу оценить код

Написал я тут небольшую игрушку, Крепс называется (исполняемый файл находится здесь). Как всегда прошу оценить код. В данный момент, на предмет правильности построения класса и самой реализации (за интерфейс игры прошу камнями не закидывать, упор на него не делал :)).
Код исходников находится здесь.
ЗАРАНЕЕ СПАСИБО :)

почему решено было использовать указатель, а не ссылку?

void input(string * str)

Бытует мнение, что работа с адресами повышает производительность.
Но в данном случае str является объектом типа string, а с объектом лучше использовать ссылку, правильно?

Бытует мнение, что работа с адресами повышает производительность.

В подавляющем большинстве случаев код с указателями и с ссылками транслируется в одинаковый код ассемблера.

Получается нет никакой принципиальной разницы? :)

Удобней использовать. Без операции разыменовования при каждом использовании.

Это уже дело вкуса:)

Без операции разыменовования при каждом использовании.

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

Хотите вы песен? Их есть у меня... (с) ))

(1) Конструктор должен создавать объект, полностью готовый к употреблению. Почему для вывода на экран костей требуется некий внешний (по отношению к объекту) массив? Либо этот массив должен быть передан конструктору объекта как параметр, либо, что лучше, вообще не иметь внешнего массива, а использовать внутренний. (Также см. п.5)

(2) Почему константа размера массива SIDE_OF_DICE попала в перечисление Status?? Да ещё таким образом, что две константы в перечислении (SIDE_OF_DICE и CONTINUE) имеют одинаковое значение 2.

(3) Вне класса Craps используются только методы. Все переменные класса закрытые. Хорошо. Зачем нужны тривиальные геттеры и сеттеры? («Тривиальные» — в том смысле, что кроме доступа к переменной, они не выполняют больше никаких функций.) Вне методов класса они не используются. Зачем они используются внутри класса? Переменные класса в методах класса и так доступны.

(4) Примерно то же можно сказать про метод Craps::rollDie(). Зачем он нужен?

В свете (3) и (4) метод Craps::rollDice() будет выглядеть проще:

// casting dice
void Craps::rollDice()
{
    sumOfDice = 0;

    for (short i = 0; i < SIDE_OF_DICE; i++){
        sideOfDice[i] = 1 + rand() % 6;
        sumOfDice += sideOfDice[i];
    }
}   // end rollDice

И размер кода сократится наверное процентов на 50, если убрать всё лишнее.

(5) Если предположить, что в дальнейшем игра будет отображать кости каким-то более сложным образом (в чём я лично сомневаюсь (с) Ослик Иа), то массив функций для отображения каждого состояния кости ещё может быть оправдан. При этом, эти функции не должны быть видны ни где, кроме методов класса Craps. Как они могут быть реализованы? (а) Как свободные функции, как сейчас это сделано. (б) Или как закрытые статические методы класса. В обоих случаях в классе надо организовать массив указателей на функции (тоже статический член класса) и инициализировать его (статический член класса можно инициализировать вне конструктора).

(6) Продолжая тему, мне кажется, что проще и логичнее было бы в классе иметь метод, который отрисовывал бы кость, исходя из числа очков, переданное в метод как параметр.

(7) Вместо массива указателей на функции для отрисовки костей напрашивается массив C-строк.

(8) Функцию input() надо перенести из файла craps.cpp в другое место. Она к классу Craps отношения не имеет.

Общее впечатление от кода: перемудрил.

PS-1. Большое желание забить камнями за интерфейс.....

PS-2. Вообще сама игра идиотская. Покер на костях — значительно интереснее ;-)

ЧЕРЕП, СПАСИБО!

Общее впечатление от кода: перемудрил.

Наверное, из-за переизбытка информации. Пока не совсем понятно, что и когда использовать и как это будет работать. Все хочется посмотреть и проверить. У меня такое впечатление, что когда начал знакомится с классами, как-будто С++ заново начал изучать. %)
Все методом проб и ошибок. Например:

(7) Вместо массива указателей на функции для отрисовки костей напрашивается массив C-строк.

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

Или

(2) Почему константа размера массива SIDE_OF_DICE попала в перечисление Status?? Да ещё таким образом, что две константы в перечислении (SIDE_OF_DICE и CONTINUE) имеют одинаковое значение 2.

Перечисление Status хотел определить как константу в самом классе. Но функция Status Craps::getGameStatus() const; возвращает одно из значений перечисления. В таком случае пришлось вынести определение перечисления за пределы класса, так как компилятор выдает ошибку "не существует типа данных Status ".
Это я все к тому, что хочется все попробовать...
А насчет SIDE_OF_DICE и CONTINUE, то для CONTINUE не принципиально какое значение в нем будет хранится, а для SIDE_OF_DICE это значение должно быть 2. Наверное определение перечисления можно переписать так

enum Status{BEGIN = -1, WON, LOST, SIDE_OF_DICE, CONTINUE};

Я понимаю, что SIDE_OF_DICE не созвучна с Status, но не создавать же для SIDE_OF_DICE еще одно перечисление с названием , допустим DICE?!

В общем, обещаю добраться и до Покера на костях, как только приведу свой «серый стек» в порядок. )))

Я понимаю, что SIDE_OF_DICE не созвучна с Status, но не создавать же для SIDE_OF_DICE еще одно перечисление с названием , допустим DICE?!

Для таких констант вообще не место в перечислении. Я дважды просмотрел код двух файлов, пока обнаружил где определяется SIDE_OF_DICE. Оно должно определяться либо через const, либо через #define.

Если в твоём последнем варианте для enum Status изменить значение BEGIN или вставить до SIDE_OF_DICE ещё одно состояние игры, то что получится? Правильно — ничего хорошего. Надо избегать кода, чреватого ошибками.

Кстати, в программе про класс List ты использовал такой же приём: определение константы размера массива через enum. Лучше возможности языка использовать традиционно.

Перечисление Status хотел определить как константу в самом классе. Но функция Status Craps::getGameStatus() const; возвращает одно из значений перечисления. В таком случае пришлось вынести определение перечисления за пределы класса, так как компилятор выдает ошибку «не существует типа данных Status ».

Перечисление Status всё-таки не константа. Это во-первых.

Во-вторых, тип данных Status таки можно определить внутри определения класса Craps.

class  Craps
{

    public:

        enum Status { BEGIN = -1, WON, LOST, CONTINUE, SIDE_OF_DICE = 2 };

    private:

        short sumOfDice;                        // two dice' sum
        short myPoint;                          // "point" -- game is't won or lost
        short sideOfDice[SIDE_OF_DICE];         // die's side
        Status gameStatus;                      // it can contain CONTINUE, WON or LOST
        void resetSumOfDice() { sumOfDice = 0; }

    public:

        // constructor
        Craps();

    // ...
        // get status of the game
        Craps::Status getGameStatus() const;
    // ...

Спасибо, Череп, за объяснения.

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

Череп, я тут переписал код в соответствии с твоими инструкциями (код здесь). Посмотри пожалуйста.
Добавил в craps.cpp функцию отображения случайных костей и в mainCraps.cpp функцию прокрутки костей, которая собственно говоря меня и смущает.

Здесь (и в функции turnDice() аналогично):

// show dice
void Craps::showDice() const
{
    void (*pshow[DICE])() = {one, two, three, four, five, six};

    for (short i = 0; i < SIDE_OF_DICE; i++){
        cout << "Die " << i + 1 << ":\n";
        (*pshow[sideOfDice[i] - 1])();
        cout << '\n';
    }
}   // end showDice

ты объявляешь автоматический массив pshow[], который экранирует одноимённый массив-член класса. С ним ты дальше и работаешь.

Надо так:

void (*Craps::pshow[DICE])() = { Craps::one, Craps::two, Craps::three, Craps::four, Craps::five, Craps::six };

// show dice
void Craps::showDice() const
{
    for (short i = 0; i < SIDE_OF_DICE; i++){
        cout << "Die " << i + 1 << ":\n";
        (*pshow[sideOfDice[i] - 1])();
        cout << '\n';
    }
}   // end showDice

Соответственно в turnDice() объявление массива надо просто удалить.

Это — основная ляпа.

Из мелких улучшений ещё можно посоветовать:

(1) Вместо литерала 6 использовать константу DICE (я так понял, что это количество граней кости и, соответственно, максимальное количество очков на одной стороне кости).

(2) Сделать приватную функцию:

int Craps::randDice() {
    return 1 + rand() % DICE;
}

поскольку это выражение встречается неоднократно,

или оптимизировать метод:

// turning dice
void Craps::turnDice() const
{
    cout << "Die 1:\n";
    (*pshow[rand() % DICE])();

    cout << "\nDie 2:\n";
    (*pshow[rand() % DICE)();

}   // end turnDice

(3) Я бы код функции turn() перенёс в Craps::rollDice(). Поскольку turn() занимается только анимацией и вызывается всегда перед Craps::rollDice(). Или, если тебя смущает такое решение, сделал бы turn() приватным членом класса и вызывал в Craps::rollDice(). Собственно и вызов Craps::showDice() тоже просится в Craps::rollDice(). Тогда получится, что Craps::rollDice() сразу показывает анимацию, вычисляет результат броска и показывает его на экране. Вроде логично. Впрочем это, наверное, уже дело вкуса.

По поводу пользовательского интерфейса и общего вида: попробуй посмотреть модуль CScreen из «Змейки». Ты ж вроде под Виндой живёшь?

Первое что меня смущало, это то что при входе в функции turnDice() и showDice() постоянно происходит инициализация массива указателей.

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

Ты ж вроде под Виндой живёшь?

И это третья проблема. Класс Craps получился кросс-платформенным (наш товарищ porshe об этом мечтает :) ) А тут в функции прокрутки костей нужно постоянно сбрасывать экран. Еще для задержки экрана, в коде я использовал Sleep() и дабы не привязывать реализацию методов класса к Винде, в main-файле объявил функцию turn(Craps & c), именно поэтому и функция turnDice() появилась, чтоб после нее сброс и задержку экрана делать.
Короче говоря я попытался использовать <ctime> и с учетом твоих корректировок по инициализации указателя на функции и мелких ляпов переписал я функцию showDice() следующим образом, но... один system("cls") не захотел уйти :)

// параметр массива указателей на функции -- случайное отображение положения костей
    void (*Craps::pshow[DICE])(int t) = { Craps::one, Craps::two, Craps::three, Craps::four, Craps::five, Craps::six };

    // show dice
    void Craps::showDice() const
    {
        int temp;
        double seconds;

    // цикл проходит 10 раз на каждом проходе увеличивая время задержки

        for (int i = 1; i < 520; i *= 2){
            cout << "Die 1:\n";
            temp = 1 + rand() % DICE;
            (*pshow[temp - 1])(temp);

            cout << "\nDie 2:\n";
            temp = 1 + rand() % DICE;
            (*pshow[temp - 1])(temp);

            seconds = i / 800.0;        // доли секунды увеличиваются на каждом проходе цикла

            clock_t dely = static_cast<clock_t>(seconds * CLOCKS_PER_SEC);  // delay
            clock_t start = clock();
            while (clock() - start < dely)
                ;

            system("cls");                              // clearing screen

        }   // end for (turning dice)

        for (short i = 0; i < SIDE_OF_DICE; i++){
            cout << "Die " << i + 1 << ":\n";
            (*pshow[sideOfDice[i] - 1])(1 + rand() % DICE);
            cout << '\n';
        }   // end for (display dice)
    }   // end showDice

По поводу пользовательского интерфейса и общего вида: попробуй посмотреть модуль CScreen из «Змейки».

Я об этом уже думал и бегло просматривал твой код. Мне хотелось бы:
— интерфейс игры сместить от полей консоли к центру;
— квадрат под «дырочками» закрасить в белый цвет, а «дырочки» сделать черными (типа кубики).

Это можно сделать? Я имею в виду цвет.

Если о кроссплатформенности, то тут porshe выложил ссылку на портированные исходники «Змейки» на Линукс.

• интерфейс игры сместить от полей консоли к центру;

Легко. И даже избавиться от постоянной очистки экрана легко: перезаписывать только изменяющуюся область экрана. И вывести всю информацию по игре: общую статистику, текущее положение в игре, сами кости...

• квадрат под «дырочками» закрасить в белый цвет, а «дырочки» сделать черными (типа кубики).

Закрасить фон в белый цвет можно. «Дырочки» будут чёрные колечки на белом фоне (ты там вроде маленькую латинскую букву «о» используешь).

Метод text_attr(WORD attr) принимает слово, первый байт которого определяет цвет фона, а второй — цвет символа. Итого: WORD(0x0a) — ярко-зелёный на чёрном фоне, WORD(0x70) — чёрный на светло-сером фоне.