critique my coding

Started by
6 comments, last by freeworld 12 years, 4 months ago
come one, come all. I've never had any schooling in programming or worked with a team were I wasn't the only coder. So While I let my brain ponder my current bug, I thought I'd post the source in my winmain file and let you guys point out the bad habits i'm using... if there are any... I'm sure there are. As long as it's justified, don't worry about being harsh... I wouldn't have posted this if I couldn't handle it.


#include <windows.h>
#include "ZESystemWindow.h"
#include "ZESystemTimer.h"
#include "ZEGraphicsInterface.h"
#include "ZEInputInterface.h"
#include "ZEMath.h"
#include "ZECollision.h"

#include "cwToken.h"
#include "cwPlayer.h"
#include "cwDeckViewer.h"
#include "cwDeckController.h"
#include "cwGrid.h"

#include <vector>

int WINAPI WinMain(HINSTANCE _instance, HINSTANCE _prevInstance, LPSTR _cmdLine, int _cmdShow)
{
srand(timeGetTime());

/** Setup and create a window for the application. */
ZESystem::Window window;
window.Create(_instance, ZESystem::WINDOW_PARAMETERS("data/config.xml"));
window.Show(_cmdShow);

/** Setup a graphics interface and render list to display our sprites. */
ZEGraphics::Interface display;
display.Create(window.windowHandle, ZEGraphics::InterfaceParameters("data/config.xml"));
display.SetupDeferredLighting();

/** Setup the input system for mouse and keyboard suport. */
ZEInput::Interface input;
input.Create(&window);

/** setup the main loops timer. */
ZESystem::Timer timer;
timer.Update();

/** Create the primary render list */
DWORD renderList = display.CreateRenderList(NULL, true);

/*********************************************************
GAME SPECIFIC ASSETS
*********************************************************/
cw::Library masterLibrary;
std::vector<cw::Player> players;
cw::DeckViewer handViewer;
cw::DeckController handController;
cw::Grid grid;

ZEGraphics::Sprite background;
ZE::Font font;

/*********************************************************
GAME INITIALIZATION
*********************************************************/
players.push_back( cw::Player(&display, masterLibrary, "data/deck_arden.xml") );
players.push_back( cw::Player(&display, masterLibrary, "data/deck_thorfin.xml") );
players.push_back( cw::Player(&display, masterLibrary, "data/deck_muuk.xml") );
players.push_back( cw::Player(&display, masterLibrary, "data/deck_elzba.xml") );

handController.SetAlignment(players[0].Hand(), ZE::VEC3(512, 680, 50), ZE::VEC2(1024, 160), ZE::VEC2(96, 160));
handViewer.SetAlignment(players[0].Hand(), ZE::VEC3(512, 680, 50), ZE::VEC2(1024, 160), ZE::VEC2(96, 160));
std::sort( players[0].Hand().begin(), players[0].Hand().end(), cw::CostMore);

grid.Create(ZE::VEC2(8, 8), ZE::VEC3(249, 37, 0), ZE::VEC2(75, 75));

background.Create(&display, "data/background.png", false, 0, 0, 1, 1);
font.Create(display.CreateTexture("data/font.png", D3DFMT_A8R8G8B8, true), "data/font.fnt", 32);

/*********************************************************
GAME LOOP
*********************************************************/

bool placeToken = false;

while (window.isOpen)
{
window.HandleMessages();

float delta = (float)timer.Update();
input.Update();

handViewer.Render(players[0].Hand(), masterLibrary, &display, 0, 0);

players[0].RenderStats(&display, ZE::VEC3(106, 150, 60), ZE::VEC2(250, 400), ZE::VEC3(40, 8, 48), &font);
players[1].RenderStats(&display, ZE::VEC3(106, 500, 60), ZE::VEC2(250, 400), ZE::VEC3(40, 308, 48), &font);
players[2].RenderStats(&display, ZE::VEC3(918, 150, 60), ZE::VEC2(250, 400), ZE::VEC3(820, 8, 48), &font);
players[3].RenderStats(&display, ZE::VEC3(918, 500, 60), ZE::VEC2(250, 400), ZE::VEC3(820, 308, 48), &font);

display.DrawQuad(0, ZE::VEC3(512, 384, 99), ZE::VEC2(1024, 768), &background, 0, 1);

display.Display();

/* check if the player is choosing a card to use when they pressed the left mouse button
and varify that they can afford the token they selected. */
if (input.mouse.LeftButtonPressed())
{
if (handController.MouseClick(input.mouse.Position(), placeToken))
{
int cardInHand = 0;
int cardInLibrary = 0;
handController.CardSelected(cardInHand);
cw::ConvertDeckIndexToLibraryIndex(players[0].Hand(), cardInHand, cardInLibrary);

if ( !cw::CanAfford(masterLibrary[cardInLibrary].Cost(), players[0].Resources()) )
{
handController.DeselectCard();
placeToken = false;
}
}
}

/** If the player releases the left mouse button over the map while a token is selected.
try to apply it's ability if it's an ability card to all the player till one excepts it, else
try to generate a map token for the main player, if either pass then purchase the token,
else deselect it.
*/
if (input.mouse.LeftButtonReleased())
{
int cardInHand = 0;
int cardInLibrary = 0;
if ( !handController.CardSelected(cardInHand) )
break;

if ( !cw::ConvertDeckIndexToLibraryIndex(players[0].Hand(), cardInHand, cardInLibrary) )
break;

ZE::VEC2 cell(0, 0);

if ( grid.Cell(input.mouse.Position(), cell))
{
bool tokenUsed = false;
bool spotUnavailable = false;
cw::Token token;

for (int p = 0; p < (int)players.size(); ++p)
{
if (players

.GetToken(cell, token))
spotUnavailable = true;

if (players

.ApplyAbility(cell, masterLibrary[cardInLibrary]))
tokenUsed = true;
}

// try to give the token to the main player.
if (tokenUsed == false && spotUnavailable == false)
{
if (players[0].ApplyToken(cell, masterLibrary[cardInLibrary]))
tokenUsed = true;
}

// if token used then purchase it.
if (tokenUsed)
{
cw::Purchase(masterLibrary[cardInLibrary].Cost(), players[0].Resources());
cw::RemoveCardFromDeck(players[0].Hand(), cardInHand);
}
}

// else deselect the token.
handController.DeselectCard();
}

int index = 0;
if (handController.CardSelected(index))
handViewer.SetSelectedCard(index, input.mouse.Position());
else
handViewer.SetSelectedCard(-1, input.mouse.Position());

}

input.Release();
display.Release();
window.Release();

return 0;
}

[ dev journal ]
[ current projects' videos ]
[ Zolo Project ]
I'm not mean, I just like to get to the point.
Advertisement
Why use 'Create' functions rather than constructors?
[TheUnbeliever]

Why use 'Create' functions rather than constructors?


good question, especially when the constructors are fully implemented... guess it's just an eye pleasing thing for me. I try to keep the length of each line within my monitors screen space. The constructors for those objects, call Create(), so they do exactly the same thing, except Create returns a boolean, which of course I'm haphazardly not using.:huh:
[ dev journal ]
[ current projects' videos ]
[ Zolo Project ]
I'm not mean, I just like to get to the point.
You sort of lost me on the flow of control in the game loop there. Mixing input handling and game logic in one place like that can become messy and confusing, but in your case the game loop is small enough that it's manageable I guess (I've seen MUCH worse cases, with essentially the entire gui code implanted in nested switch-cases in the main loop.)

What do the breaks in the second if do? They seem to exit the main loop, but with no indication of what the exit conditions are. Are they victory conditions? Error handling?

You sort of lost me on the flow of control in the game loop there. Mixing input handling and game logic in one place like that can become messy and confusing, but in your case the game loop is small enough that it's manageable I guess (I've seen MUCH worse cases, with essentially the entire gui code implanted in nested switch-cases in the main loop.)

What do the breaks in the second if do? They seem to exit the main loop, but with no indication of what the exit conditions are. Are they victory conditions? Error handling?


That's what I was looking for. I'll admit I hacked those two if blocks together, and still can't think where it makes sense for them to be contained, since it deals with four of my objects, and one object (the map) which is completely fictional other than one function that calculates the cell position from a screen position.... if the position is valid.

Maybe I need to do some reading up on how break and continue actually work. In this case I'm basically using them like 'return' to exit the parent if block, if there is no card selected, or the given card is invalid. My understanding of break and continue is it works on a block (scope) basis, ie break should exit the next higher up block of code, whether that block is an if, for, or while.

I haven't stepped through to varify that it's working as i suspect, but I don't get the errors that I'm using it to prevent, so I just assumed it did. I'm off to step, and read.

Please correct me if I'm wrong here



if (foo)
{
if (bar)
break; // exits the foo if

if (bar)
continue; // ? I'd assume, it would jump back to the foo if causing an infinite loop?
}

[ dev journal ]
[ current projects' videos ]
[ Zolo Project ]
I'm not mean, I just like to get to the point.
break and continue do not affect if() statements.

Wielder of the Sacred Wands
[Work - ArenaNet] [Epoch Language] [Scribblings]




if (foo)
{
if (bar)
break; // exits the foo if

if (bar)
continue; // ? I'd assume, it would jump back to the foo if causing an infinite loop?
}



That's what I suspected you suspected :) Actually, breaks and continues exit the current loop, not code block. But I encourage you to actually test this and not just take my word for it, as it's useful to get into the habit of testing these things. Programming is very much an empirical science.

The way to solve what you are trying to do would be more in the lines of checking all the conditions up front, ie.


if(foo && !bar)
{
...
}

That's what I suspected you suspected :) Actually, breaks and continues exit the current loop, not code block. But I encourage you to actually test this and not just take my word for it, as it's useful to get into the habit of testing these things. Programming is very much an empirical science.

The way to solve what you are trying to do would be more in the lines of checking all the conditions up front, ie.


if(foo && !bar)
{
...
}



That's how I had it originally and changed it back... though I liked the breaks because it makes it easier for me to read and debug. I still haven't justified putting it into a function that could just swap the breaks for return... though. The code just doesn't seem like it associates it self with only one object or even one header. I did some research on the subject last nite, and unfortunately, break and continue aren't the all powerful things I thought they were.
[ dev journal ]
[ current projects' videos ]
[ Zolo Project ]
I'm not mean, I just like to get to the point.

This topic is closed to new replies.

Advertisement