# critique my coding

This topic is 2259 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Recommended Posts

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; }

##### Share on other sites
Why use 'Create' functions rather than constructors?

##### Share on other sites

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.

##### Share on other sites
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?

##### Share on other sites

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? } 

##### Share on other sites
break and continue do not affect if() statements.

##### Share on other sites

 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) { ... } 

##### Share on other sites

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.