悪いプログラムの書き方 今回は応募作品中に見られる 初心者にありがちな悪いプログラムの書き方について 例をあげて説明したいと思います 悪い書き方これだけではなく まだ沢山ありますので更に本を読んだりして勉強して下さい また 書き方について更に学びたい場合は コーディングルール をみてみると良いでしょう 会社やチームごとに細かな違いがありますが コーディングルール はどの様にすれば間違いが少なく 無駄が少なく 統一的な書き方が出来るか を考えて作られた規約です ネット上では様々な コーディングルール が公開されています Google で "C++ コーディングルール " を検索してみて下さい ローマ字表記を使用している 少数ですがプログラム中にローマ字表記を使う人がいます その人たちは例外なくプログラムの書き方が良くありませんので 評価する側はローマ字表記を見た時点で初心者と判断します プログラムでは英語表記が基本です 英単語は辞書ですぐ調べられるので 必ず 英語を使って下さい int teki_suu; int zikan; Foo* Kensaku( const char* namae ); int enemy_num; int time; Foo* Find( const char* name ); 表記が統一されていない 会社説明会のスライドでも説明しましたが 書き方が統一されていないプログラムは読みにくく チーム制作ではバグの原因になります 大文字 小文字の混在 複数単語の表記順番が違う といった間違いが多いです 以下の例ではスピードを表す単語が Spd, spd,speed の 3 つがあり 前に動詞が付いたり付かなかったりしています また 変数名の開始が大文字だったり 小文字だったりしています class Player Vector3 pos; float Rot; float jumpspd; float speedx; float speedy; float enemy_spd; int Action; int ActCount; int ActCntTemp; ; class Player Vector3 pos; float rot; float jumpspd; float movexspd; float moveyspd; float enemyspd; int action; int actcnt0; int actcnt1; ;
関数のコメントが細かく多い クラスや 関数 1つ 1 つにコメントを細かく多く入れる事を正しいと勘違いしている人は多くいます また 学校によってはその様な指導をする所もあります しかし プログラムが分かり易ければコメントを書く必要性は低く 多いと逆に読みにくくなります 特に関数の説明をするコメントの内容の多くは関数自体を見れば分かるので 書く必要がありません 例えば Draw() 関数の説明に 描画する と書いてあっても無意味です シンプルなコメントを心がけましょう そして 分かりにくい動作をする場合や 関数の呼び出しに注意するべき事項がある場合にコメントを入れましょう 例えば以下の様な動作が単純な関数では 書かなくても意味が理解できます また引数も分かりやすい名前を付ける事で分かりやすく出来ます /******************************************************************************/ /* 関数名 : Stage::Clear() 書いてあるのでいらない説明 : ステージの初期化用関数 関数名から意味がわかるので必要ない引数 1 : ステージ番号 なるべく引数の変数名で意味がわかる様にしておく戻り値 : なし担当 : 宇佐見 バージョン管理ツールをつかえば誰が何を作成 変更したかわかる作成日 : 2014/06/06 バージョン管理ツールをつかえば誰が何を作成 変更したかわかる */ /******************************************************************************/ void Stage::Initialize( int n ) m_ stageid = n; /******************************************************************************/ /* 関数名 : Stage::Draw() 説明 : ステージを描画する 引数 1 : なし 戻り値 : なし 担当 : 宇佐見 作成日 : 2014/06/06 */ /******************************************************************************/ void Stage::Draw() m_stageobject.draw(); void Stage::Initialize( int stageid ) m_stageid = stageid; void Stage::Draw() m_stageobject.draw(); 即値を多用する 会社説明会のスライドでも説明しましたが 調整や変更される可能性が高い箇所に直に数値を指定してはいけません 以下の例では三次元のマップデータを生成しています しかし これでは固定のサイズのマップデータしか生成できません class MapBlock int m_blocktbl[10][30][30]; public: void Initialize(); ; void MapBlock::Initialize() for( int y = 0; y < 10; ++y ) for( int z = 0; z < 30; ++z ) for( int x = 0; x < 30; ++x ) m_blocktbl[y][z][x] = BLOCK_SPACE; m_blocktbl[1][ 2][ 2] = BLOCK_DOOR; m_blocktbl[3][15][ 4] = BLOCK_START; m_blocktbl[7][ 4][28] = BLOCK_GOAL;
m_blocktbl[6][14][18] = BLOCK_SW1; m_blocktbl[0][22][ 6] = BLOCK_SW2; プログラムが長くなりますが 以下の様に大きさが変わっても問題無く動くようにするべきです そして 生成する際の大きさやアイテムの配置はデータ化しています この様に設定部分をデータ化する事で 後で容易に変更できるようになります データ部分以外に 0 以外の即値がない点が重要です struct MapItem int m_x; int m_y; int m_z; int m_type; ; struct MapData int m_xsize; int m_zsize; int m_ysize; const MapItem* m_itemtbl; ; class MapBlock int* m_blocktbl; int m_xsize; int m_zsize; int m_ysize; int m_xzsize; int m_xzysize; public: void Initialize( const MapData& data ); void SetItem( const MapItem& item ); ; void MapBlock::Initialize( const MapData& data ) m_xsize = data.m_xsize; m_zsize = data.m_zsize; m_ysize = data.m_ysize; m_xzsize = m_xsize * m_zsize; m_xzysize = m_xzsize * m_ysize; m_blocktbl = new int[m_xzysize]; for( int y = 0; y < m_ysize; ++y ) for( int z = 0; z < m_zsize; ++z ) for( int x = 0; x < m_xsize; ++x ) int n = ( y * m_xzsize ) + ( z * m_xsize ) + x; m_blocktbl[n] = BLOCK_SPACE; for( int i = 0; i < data.m_itemtbl[i].m_type!= BLOCK_SPACE; ++i ) SetItem( data.m_itemtbl[i] ); void MapBlock::SetItem( const MapItem& item ) if( item.m_x < 0 item.m_x >= m_xsize ) return; if( item.m_z < 0 item.m_z >= m_zsize ) return; if( item.m_y < 0 item.m_y >= m_ysize ) return; int n = ( item.m_y * m_xzsize ) + ( item.m_z * m_xsize )+ item.m_x; m_blocktbl[n] = item.m_type; void CreateMap00() const MapItem mapitem00[] = 2, 2, 1, BLOCK_DOOR, 15, 4, 3, BLOCK_START, 4, 28, 7, BLOCK_GOAL, 14, 18, 6, BLOCK_SW1, 22, 6, 0, BLOCK_SW2, 0, 0, 0, BLOCK_SPACE, ;
const MapData mapdata00 = 30, 30, 10, mapitem00 ; MapBlock mapblock; mapblock.initialize( mapdata00 ); 1 つの式に [] 演算子 -> 演算子. 演算子が 何個も使われている オブジェクトを参照する為に使われる [] 演算子 -> 演算子. 演算子が連なって使われる場合は プログラムの構造化が出来ていないか 構造体やクラスの設計に問題がある場合が殆どです 処理ごとに関数を分けて下さい そうすると これらの演算子を多用する事は無くなります 以下の例では Game クラスの中に Player クラスの処理が入っています その為 Player クラスやその先のオブジェクトを参照する為の演算子を多用します void Game::UpdatePlayer() for( int i = 0; i < PLAYER_NUM; ++i ) if( g_playertbl[i].getactionflag() == ACT_ATTACK ) Vector3 dirc = g_playertbl[i].getweapon()->getdirction(); Vector3 pos = g_playertbl[i].getposition(pl_arm_r); g_playertbl[i].getweapon()->createbullet( pos, dirc ); if( g_playertbl[i].getactionflag() == ACT_GUARD ) // ガードの処理 Player クラスに対する処理は Player クラスの関数で処理を行う様にします また プレイヤーの条件による処理の分岐 と プレイヤー起こす行動 で動作の意味が違いますので これも関数を分けます void Game::UpdatePlayer() for( int i = 0; i < PLAYER_NUM; ++i ) g_playertbl[i].update(); void Player::Update() if( GetActionFlag() == ACT_ATTACK ) Attack(); if( GetActionFlag() == ACT_GUARD ) Guard(); void Player::Attack() Weapon* weapon = GetWeapon(); Vector3 dirc = weapon->getdirction(); Vector3 pos = GetPosition(PL_ARM_R); weapon->createbullet( pos, dirc ); void Player::Guard() // ガードの処理 関数 1 つの行数が長い 関数の行数が多い場合 1 つの関数に処理を詰め込みすぎている場合が殆どです
長くなる場合は機能ごとに関数を分けて下さい そうすると これらの長い関数になる事は無くなります 基本 100 行以内に収めましょう 以下の例では プレイヤーの移動処理全てを 1 つの関数にまとめて書いてしまっています この様なパターンはよく見かけます プレイヤーの状態判定と 行動処理 ステージとの衝突判定が全て入っています void Game::UpdatePlayer() const float limit = 0.0f; D3DXVECTOR3 vec; if( player.getflyflag() == false && player.getactiveflag() == false ) D3DXVECTOR3 movepos; if( IsInputKey(KEY_UP) ) bool ishit = false; D3DXVec3TransformCoord(&vec,&D3DXVECTOR3( 0, 0, 1 ), &player.getrot() ); D3DXVec3Normalize( &vec, &vec ); for(int i = 0; i < stage.getobjmax(); i++ ) D3DXVECTOR3 dist = player.getpos() - stage.getobj(i)->getpos(); if( D3DXVec3Length( &dist ) <= 12 ) short collisionid = stage.getobj(i)->getcollisionid(); if(collisionid!=-1) if( player.frontray( xobj.getobj( collisionid )->GetMesh(), &stage.getobj(i)->getmatrix(), limit, &movepos ) == true ) ishit=true; else if( player.frontray( xobj.getobj( stage.getobj(i)->getobjnum() )->GetMesh(), &stage.getobj(i)->getmatrix(), limit, &movepos ) == true) ishit=true; if( ishit == true ) bool bflg = false; D3DXVec3TransformCoord( &vec, &D3DXVECTOR3( 0, 0, -1 ), &player.getrot() ); D3DXVec3Normalize( &vec, &vec ); for( int i = 0; i < stage.getobjmax(); i++ ) D3DXVECTOR3 dist = player.getpos() - stage.getobj(i)->getpos(); if( D3DXVec3Length( &dist ) <= 12 ) if( bflg == false ) D3DXVECTOR3 plpos = player.getpos() + movepos; player.initpos( plpos.x, player.gety(), plpos.z ); else if(player.getjumpflg()==false) player.setpos( 0, 0, 0.5f ); else player.setpos( 0, 0, 0.25f ); if( IsInputKey( KEY_DOWN ) ) if( IsInputKey( KEY_LEFT ) )
if( IsInputKey( KEY_RIGHT ) ) if( IsInputKey( KEY_SPACE ) ) この様な場合 衝突判定は Stage クラスの関数に プレイヤーの処理は Player クラスの関数に分け 更に Player クラスの関数は 条件判定処理 と 行動処理 で更に関数に分けます この様に関数 1 つ 1 つが何の処理を担当しているか 明確に区分する事が重要です const float limit = 0.0f; bool Stage::CollisionPlayer( Player& player, D3DXVECTOR3* movepos ) D3DXVECTOR3 vec; D3DXVec3TransformCoord(&vec,&D3DXVECTOR3( 0, 0, 1 ), &player.getrot() ); D3DXVec3Normalize( &vec, &vec ); for( int i = 0; i < GetObjMax(); i++ ) SMesh* meshobj = stage.getobj(i); D3DXVECTOR3 dist = player.getpos() - meshobj->getpos(); if( D3DXVec3Length( &dist ) <= 12 ) short collisionid = GetObj(i)->GetCollisionId(); LPD3DXMESH d3dmesh = collisionid!= -1? xobj.getobj( collisionid )->GetMesh() : xobj.getobj( meshobj->getobjnum() )->GetMesh() ; if( player.frontray( d3dmesh, &meshobj->getmatrix(), limit, movepos ) ) return true; return false; bool Stage::CollisionPlayer2nd( Player& player, D3DXVECTOR3* movepos ) D3DXVECTOR3 vec; D3DXVec3TransformCoord( &vec, &D3DXVECTOR3( 0, 0, -1 ), &player.getrot() ); D3DXVec3Normalize( &vec, &vec ); for( int i = 0; i < GetObjMax(); i++ ) SMesh* meshobj = stage.getobj(i); D3DXVECTOR3 dist = player.getpos() - meshobj->getpos(); if( D3DXVec3Length( &dist ) <= 12 ) return false; void Player::MoveForward() D3DXVECTOR3 movepos; bool ishit = stage.collisionplayer( *this, &movepos ); if( ishit == true ) bool ishit2 = stage.collisionplayer2nd( *this, &movepos ); if( ishit2 == false ) D3DXVECTOR3 plpos = GetPos() + movepos; InitPos( plpos.x, GetY(), plpos.z );
else if( GetJumpFlg() == false ) SetPos( 0, 0, 0.5f ); else SetPos( 0, 0, 0.25f ); void Player::Update() if( GetFlyFlag() == false && GetActiveFlag() == false ) if( IsInputKey( KEY_UP ) ) MoveForward(); if( IsInputKey( KEY_DOWN ) ) MoveBack(); if( IsInputKey( KEY_LEFT ) ) TurnLeft(); if( IsInputKey( KEY_RIGHT ) ) TurnRight(); if( IsInputKey( KEY_SPACE ) ) Jump(); //----------------------------- File game.h void Game::UpdatePlayer() player.update(); グローバル変数 スタティック変数を多用する ゲーム全体で使用する変数を利用するとき グローバル変数 スタティック変数を使わないようにしましょう 初心者はこのような変数を 1 つのファイルに設定値をまとめて それを参照するための global.h を定義するといった傾向があります 以下の様な例では ゲームでの設定値は Game クラスのオブジェクトに グラフィックデバイス関連の値は GraphicDevice クラスに と機能ごとにクラスを作り そのメンバとして定義する様にして下さい //----------------------------- File global.h extern LPDIRECT3DDEVICE g_d3ddevice; extern int g_nowstageid; extern int g_player1id; extern int g_player2id; extern int g_playtime; extern bool g_debugflag; //----------------------------- File main.cpp LPDIRECT3DDEVICE g_d3ddevice; // グラフィックデバイスへのアクセス int g_nowstageid; // 現在のステージ番号 int g_player1id; // 1Pのプレイヤー番号 int g_player2id; // 2Pのプレイヤー番号 int g_playtime; // プレイ残り時間 bool g_debugflag; // デバッグ機能ありorなし //----------------------------- File stage.h class Stage int m_id; ; //----------------------------- File graphicdevice.h class GraphicDevice LPDIRECT3DDEVICE m_device; ; //----------------------------- File player.h class Player int m_id; ; //----------------------------- File game.h
class Game int m_debugflag; int m_playtime; ; // それぞれのクラスは 必要な段階で new をして確保する 同じ処理を何度も呼び出す 以下の例では 関数は整頓されているように見えますが 条件判断文の条件と関数の引数が違うだけで 同じ関数呼び出しが並んでいます このようなプログラムは データ と 処理 に分けることが出来ます void Enemy::DropItem() int temp = GetRand(19); switch( kind ) case MANTIS: if( temp < 10 ) MakeItem( Item::POINT_LOW,position ); else if( temp == 10 ) MakeItem( Item::HP_MINI,position ); else if( temp < 14 ) MakeItem( Item::BOM_MINI,position ); case APPLE: if( temp < 8 ) MakeItem( Item::POINT_HIGH,position ); else if( temp < 12 ) MakeItem( Item::SCORE,position ); else if( temp < 16 ) MakeItem( Item::RAPID,position ); else MakeItem( Item::HP_BIG,position ); case MISSILETANK: if( temp < 3 ) MakeItem( Item::HP_MINI,position ); else if( temp < 4 ) MakeItem( Item::HP_BIG,position ); else if( temp < 9 ) MakeItem( Item::BOM_MINI,position ); else if( temp < 14 ) MakeItem( Item::BOM_BIG,position ); 違う部分 ( 条件判断文の条件と関数の引数 ) をデータとして分けます この部分は将来 ファイルから読み込ませたり エディタで値を変更させられるようにしたり といった変更を簡単に出来るようになります また データ部分が増えてもプログラムは増えることはありません static Item::Type GetDropItem( const DropItemTbl* dropitemptr ) int rate = GetRand(19); for( ; dropitemptr->rate!= 0; ++dropitemptr ) if( rate < dropitemptr->rate ) return dropitemptr->itemtype; void Enemy::DropItem() const DropItemTbl dropitemtbl_mantis[] = 10, Item::POINT_LOW, 11, Item::HP_MINI, 14, Item::BOM_MINI, 0, Item::NONE, ; const DropItemTbl dropitemtbl_apple[] = 8, Item::POINT_HIGH, 12, Item::SCORE, 16, Item::RAPID, 0, Item::HP_BIG, ; const DropItemTbl dropitemtbl_missiletank[] = 10, Item::POINT_LOW, 11, Item::HP_MINI, 14, Item::BOM_MINI, 0, Item::NONE, ; const DropItemTbl* dropitemtbl[] = dropitemtbl_mantis, dropitemtbl_apple, dropitemtbl_missiletank, ; // テーブル数はテーブルから求める const int tblsize = sizeof(dropitemtbl)/sizeof(dropitemtbl[0]);
if( kind < 0 kind >= tblsize ) return; Item::Type itemtype = GetDropItem( dropitemtbl[kind] ); if( itemtype!= Item::NONE ) MakeItem(itemType,position); 変数の確保定数に const 指定を行わない 定数には必ず const を付けましょう 以下の例では 関数内にデータテーブルを持っていて それが書きかえられないにも関わらず const を付けていません この様な箇所が多いと 変数や定数の区別が付いていない初心者と判断されてしまいます void StageBlock::CheckCorner( int xpos, int ypos ) int checktbl[][2] = 1,1, 1,-1, -1,-1, -1,1 ; unsigned int result = 0; for( int i = 0; i < 4; ++i ) result = CheckPos( xpos + checktbl[i][0], ypos + checktbl[i][1] ); void StageBlock::CheckCorner( int xpos, int ypos ) const int checktbl[][2] = 1,1, 1,-1, -1,-1, -1,1 ; unsigned int result = 0; for( int i = 0; i < 4; ++i ) result = CheckPos( xpos + checktbl[i][0], ypos + checktbl[i][1] );