Threaded Mode | Linear Mode

Efrit · 04.07.2012 19:44

Что ж, Пакка, как и просил - держи рецензию на свой код

Прокомментирую его так, будто это настоящий продакшн-код:

PHP Code:

//---------------------------------------------------------------------------

#include <vcl.h>
#include <math.h>
#pragma hdrstop

#include "Unit1.h"
#include <vector>
//---------------------------------------------------------------------------
#pragma package(smart_init)
#pragma resource "*.dfm"
using namespace std;
TForm1 *Form1;
int mainX, mainY, raznica_ammo_enemy_Y, ammoN, n;
bool build;
const int enemyN=5;

class Tower
{
public:
    int xx;
    int yy;
    int health;
    int damage;
    int target;
    int reload;

    Tower ( int x, int y, int h, int d, int t, int r);

    void Draw_Tower();
    void Zahvat_Target();
    void Draw_Lazer();
    void Reload_Tower();
};

class Enemy
{
public:
    int xx;
    int yy;
    int health;

    Enemy( int x, int y, int h );

    void Draw_Enemy();
    void Move_Enemy();
};


class Ammo
{
public:
    int xx;
    int yy;
    int number;
    int target;

    Ammo (int x, int y, int n, int t);

    void Move_Ammo( int j);
    void Spawn_Ammo( int j);
    void Draw_Ammo();
};


vector <Enemy> enemy_group;
vector <Tower> tower_group;
vector <Ammo> ammo_group;
vector <int> interval;

Graphics::TBitmap *enemy2 ;
Graphics::TBitmap *ammo ;
Graphics::TBitmap *tower ;
Graphics::TBitmap *boom ;
Graphics::TBitmap *main ;
//---------------------------------------------------------------------------
__fastcall TForm1::TForm1(TComponent* Owner)
        : TForm(Owner)
{
}
//---------------------------------------------------------------------------
Enemy::Enemy( int x, int y, int h )
{
 xx=x;
 yy=y;
 health=h;
}

//----------------------------

void Enemy::Draw_Enemy()
{
for (int i=0; i<enemyN; i++)
{
    if (enemy_group[i].xx!=mainX)
    {
        if (enemy_group[i].health>0)
        {
            Form1->Canvas->Draw(enemy_group[i].xx, enemy_group[i].yy, enemy2);
        }
        else
        {
            Form1->Canvas->Draw(enemy_group[i].xx, enemy_group[i].yy, boom);
        }
    }
}
}
//---------------------

void Enemy::Move_Enemy()
{
        if ( xx != mainX )
        {
            if (xx>0)
            {
                xx+=1;
            }
        }
}
//===========================

Tower::Tower( int x, int y, int h, int d, int t, int r)
{
    xx=x;
    yy=y;
    health=h;
    damage=d;
    target=t;
    reload=r;
}
//---------------

void Tower::Zahvat_Target()
{
int rasstoyanie[5]; //Массив расстояний от башни до врага
        for (int j=0; j < enemyN; j++)
        {
        if (enemy_group[j].health>0)
        {
        int raznicaXtarget = xx - enemy_group[j].xx; //Разница по Х
        int raznicaYtarget = yy - enemy_group[j].yy; //Разница по Y
                if (raznicaYtarget<0){         //Если башня выше цели
                raznicaYtarget=enemy_group[j].yy - yy;
                }
        rasstoyanie[j]=sqrt(pow(raznicaXtarget,2)+pow(raznicaYtarget,2)); //Расстояние от врага до башни, по Пифагору
        }
        int min = rasstoyanie[0];
        int b=0;
        for (int j=0; j<enemyN; j++){
                if (rasstoyanie[j]<min){
                        min=rasstoyanie[j]; //Находим ближайшего врага
                        b=j;
                }

                if (rasstoyanie[j]<350)
                { //Если враг в области поражения башни (пока числом)
                target=b; //Запоминаем номер врага
                }
        }
        }


}
//-------------

void Tower::Draw_Tower()
{
Form1->Canvas->Draw(xx, yy, tower);
}

//-------------

void Tower::Draw_Lazer()
{
    Form1->Canvas->Pen->Color=(TColor)RGB(200,0,0);
    Form1->Canvas->MoveTo(xx,yy);
    Form1->Canvas->LineTo(enemy_group[target].xx,enemy_group[target].yy);
}
//--------------

void Tower::Reload_Tower()
{
    reload+=1;
}

//=====================================================================

Ammo::Ammo (int x, int y, int n, int t)
{
    xx=x;
    yy=y;
    number=n;
    target=t;
}

//-----------------------

void Ammo::Move_Ammo( int j )
{
for(int j=0; j<ammoN; j++)                       // Для каждого патрона из массива... Делать столько раз, сколько патронов в потоке.
    {
        if ( enemy_group [target].xx == xx && enemy_group[target].yy == yy )
        {
            enemy_group[target].health-=50;
        }
        else
        {
            xx-=1;   // Движение по оси Х

            //Разница между высотой врага и патрона=высота патрона - высота"выделенного" врага
            raznica_ammo_enemy_Y=0;
            if (enemy_group[target].yy > yy) //Если высота врага < высота патрона
            {
                raznica_ammo_enemy_Y = enemy_group[target].yy - yy;
                if (raznica_ammo_enemy_Y < 5)
                {
                    yy += raznica_ammo_enemy_Y;
                }
                else
                {
                    yy += 5; //Опускаемся на 5
                }
            }
            else
            {
                raznica_ammo_enemy_Y = yy - enemy_group[target].yy;
                if (raznica_ammo_enemy_Y<5)
                {
                    yy -= raznica_ammo_enemy_Y;
                }
                else
                {
                    yy -= 5; //Поднимаемся на 5
                }
            }
        }
    }
}

//--------------------------
void Ammo::Spawn_Ammo ( int j )
{
    if ( tower_group[j].target >= 0 && tower_group[j].target < 5 )           //Если у башни есть цель
    {
        if (enemy_group[tower_group[j].target].health>0)
        {
                Ammo newAmmo (tower_group[j].xx, tower_group[j].yy, ammoN, tower_group[j].target);
                ammo_group.push_back( newAmmo );
                ammoN++;
                tower_group[j].reload=0;   //Обнуляем для нового отсчета
        }
    }
}
//-----------------

void Ammo::Draw_Ammo()
{
    Form1->Canvas->Draw( xx, yy, ammo);
}
//===========================

void __fastcall TForm1::Button1Click(TObject *Sender)
{
for (int i=0; i < enemyN; i++)
{
    Enemy newEnemy( random(100),random(300), 100 );
    enemy_group.push_back( newEnemy );
}
Timer1->Enabled = true;
}
//---------------------------------------------------------------------------

void __fastcall TForm1::Timer1Timer(TObject *Sender)
{
for (int i=0; i<enemyN; i++)
{
enemy_group[i].Move_Enemy();
}
//
if (tower_group.empty()!=true)
{
    for (int i=0; i<tower_group.size(); i++)
    {
    tower_group[i].Zahvat_Target(); //Башни ищут ближайшую цель
    }
    for(int j=0; j<n; j++)
    {
        if (tower_group[j].reload==10)
        {
            ammo_group[j].Spawn_Ammo( j );
        }
        else
        {
            tower_group[j].Reload_Tower();
        }
    }
    for (int k=0; k<ammoN; k++)
    {
    ammo_group[k].Move_Ammo( k );  //Двигаем все патроны
    }
}
Invalidate();
}
//---------------------------------------------------------------------------

void __fastcall TForm1::FormPaint(TObject *Sender)
{
for (int i=0; i<enemyN; i++)
{
enemy_group[i].Draw_Enemy();
}

if (tower_group.empty()!=true)
{
    for (int i=0; i<tower_group.size(); i++)
    {
        tower_group[i].Draw_Tower();
        if ( tower_group[i].target >= 0 )           //Если у башни есть цель то рисуем "лазер"
        {
            if (enemy_group[tower_group[i].target].health>0)
            {
              tower_group[i].Draw_Lazer();
            }
        }
    }
    for(int j=0; j < ammoN; j++)
    {
        ammo_group[j].Draw_Ammo();
    }
}
Form1->Canvas->Draw(610, 235, main);
}
//---------------------------------------------------------------------------

void __fastcall TForm1::FormCreate(TObject *Sender)
{
DoubleBuffered=true;
for (int i=0; i < enemyN; i++)
{
    Enemy newEnemy( random(100),random(300), 100 );
    enemy_group.push_back( newEnemy );
}
n=0;
mainX=610;
mainY=235;
ammoN=0;
tower = new Graphics::TBitmap;
tower->LoadFromFile("tower.bmp");
main = new Graphics::TBitmap;
main->LoadFromFile("main.bmp");
enemy2 = new Graphics::TBitmap;
enemy2->LoadFromFile("enemy2.bmp");
ammo = new Graphics::TBitmap;
ammo->LoadFromFile("ammo.bmp");
boom = new Graphics::TBitmap;
boom->LoadFromFile("boom.bmp");
}
//---------------------------------------------------------------------------


void __fastcall TForm1::FormMouseDown(TObject *Sender, TMouseButton Button,
      TShiftState Shift, int X, int Y)
{
if (build==true)
{
    Tower newTower ( X,Y, 100, 50, -1, 10);
    tower_group.push_back( newTower );
    n++;
}
Invalidate();
Form1->Edit1->Text=tower_group.size();
}
//---------------------------------------------------------------------------


void __fastcall TForm1::Button2Click(TObject *Sender)
{
 if (build==true){
build=false;
}
else {build=true;}
}
//--------------------------------------------------------------------------- 

Оформление:

1) Бросается в глаза разный стиль именования переменных: mainX, mainY, raznica_ammo_enemy_Y. Нотация везде должна быть единой, даже если эта нотация - венгерская.
2) Конструктор класса Tower вынесен за его пределы - это ладно (хотя и не ясно, зачем), но почему он записан где-то в середине файла, а не сразу вслед за классом? А ещё лучше - выносить классы в отдельные файлы.
3) Да и что значат его переменные, почему они все однобуквенны? В строке Tower ( int x, int y, int h, int d, int t, int r) разобраться сможет только автор этого кода, и никто другой. А писать код надо так, будто сопровождать его будет склонный к насилию психопат - который знает, где ты живёшь. (с)
4) За названия вроде "raznica_ammo_enemy_Y" или "Zahvat_Target()" нужно вырвать и застрелить руки. Если уж не знаешь полностью английского (что вполне простительно) - то пиши всё транслитом, но смешивать языки - это моветон.
5) Все символы, относящие к названию типа, нужно писать слитно. В строке vector <Enemy> у тебя Enemy является частью типа, поэтому пробел ставить не надо. Это же касается и звёздочек в объявлениях указателей.
6) А вот в строках типа enemy_group[i].xx!=mainX, наоборот, нужно обязательно отделять пробелом знаки операций. Иначе визуально ничего не понятно. Арифметические операции - это полноценные функции, и они заслуживают "отделения".
7) Скобки {} кое-где поплыли, а кое-где и отступы вместе с ними. Местами вообще получилась нечитабельная каша, как в функции Zahvat_Target().

Кодинг:

1) Строка #include <vector> должна стоять выше, чем #pragma hdrstop. Потому как в твоём случае <vector> будет инклудиться всегда, хотя достаточно всего одного раза.
2) Используется множество глобальных переменных - которые, как известно, не есть гут. В большинстве случаев их можно заменить локальными, если только это не синглтоны. Да и хотя бы прокомментировать их не мешало бы.
3) Из-за конструкций вроде Form1->Canvas->Draw(610, 235, main) есть риск быть похороненным в закрытом гробу бирюзового цвета. ~~Почему именно бирюзового? Потому что 610.~~ А если уж эта строка нужна чисто "для дебага", то нужен комментарий об этом (а ещё лучше - условная компиляция).
4) В конструкторах лучше использовать списки инициализации, а не поочерёдное присваивание. Во-первых, так безопаснее. Во-вторых, только так можно проинициализировать не-статические константы класса.
5) Кстати, а где константы-то? В коде хватает переменных, которые не изменяются после инициализации. А ведь "константность" защищает от ошибок на этапе исполнения.
6) Раздражают конструкции вида if (tower_group.empty()!=true) или reload+=1. В первом случае сравнение с true можно не писать, а во втором нужен обычный инкремент.
7) Приведение типов в стиле C, как в строке (TColor)RGB(200,0,0), не всегда безопасно - лучше использовать "плюсовые" static_cast и dynamic_cast.
8) Зачем в функцию Move_Ammo( int j ) передаётся параметр j, когда он же вводится в первой строчке самой функции? Либо неиспользуемая переменная, либо просто ошибка.
9) Ввело в ступор содержимое функции Button2Click(). Надеюсь, что это просто "заглушка" такая?

Рекомендации:

1) Использовать везде единый стиль записи. Это касается именования переменных, расстановки скобок и комментариев. Имена должны быть осмысленными.
2) Упаковывать данные внутрь классов, используя идиому RAII. Если картинка относится к элементу конкретного класса, не надо таскать её как глобальную переменную - нужно выделить память под неё в конструкторе, и освободить в деструкторе.
3) Почему не стоит обходить вектор через int-переменную, скромно умолчу. Но на всякий случай посоветую почитать про итераторы. Через них всё идёт намного быстрее.
4) Вместо "голых" указателей лучше использовать shared_ptr<> и создавать всё в стеке, избегая кучи вообще. Хотя тебе ещё далековато до такой техники.
5) Вместо множества функций типа Draw_Tower() и Draw_Lazer() лучше иметь одну перегруженную функцию Draw(), которая могла бы принимать объекты разных типов. Это удобно для вызова функций (IDE сама показывает перегрузки), да и наследоваться от таких классов куда проще.
6) В любом классе желательно наличие конструктора по умолчанию (без параметров), дабы его объекты можно было использовать внутри массивов и STL-контейнеров. У тебя же далеко не везде так.

P.S. Только не стоит расстраиваться от прочитанного - я в твоём возрасте кодил намного хуже Rolleyes

Threaded Mode \| Linear Mode С++, общая тема
Author	Message