Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

For the code sample that I am asked to submit with most of my job applications (usually Gameplay Programmer), I created this console application in which I attempt to figure out what is the best loadout for a mech to win a match within the simple turn-based combat system that I came up with. Since I will submit this as a code sample, it is important to me that it works and looks the best, and I would really appreciate all the input that you could give me.

I would like to mention that I already posted this sometime last summer, but since then I reworked it considerably (mostly because I studied up on C++11), so I hope it's OK to resubmit.

What I'm interested in is really everything that comes to your mind when you see this — about the design, implementation, presentation, naming, or really anything that you think is important.

In particular, am I really taking advantage of all the C+11 concepts that I can, and am I using them in a correct manner?

The code as well as the rules are available online: http://www.mz1net.com/code-sample

(UPDATE: I've updated the online version with the edits that I made based on Jamal's response.)

This is Main.cpp - please see the 'MechArena.cpp' and 'MechArena.h' online:

#include <conio.h>
#include <iostream>
#include <sstream>
#include <time.h>

#include "MechArena.h"

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

void init() 
{
    srand((int)time(0));

    std::cout << "=== MECH ARENA ============================" << std::endl;
    std::cout << "===========================================" << std::endl;
    std::cout << std::endl;
}

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

int promptPopulationSize() 
{
    std::cout << "Enter the population size (10K recommended): ";

    // Loop until we have a valid result:
    while (true) 
    {
        std::string input;
        getline(std::cin, input);

        int size; 
        std::stringstream parser (input);
        if (parser >> size) 
        {
            if (size % 2 > 0) 
            {
                std::cout << "Population size was rounded up to an even number: " << size+1 << std::endl;
                size++;
            }

            std::cout << std::endl;
            return size;
        }
        else 
        {
            std::cout << "Please enter a valid number: ";
        }
    }
}

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

void run() 
{
    init();
    int populationSize = promptPopulationSize();

    std::cout << "Random population seed... " << std::endl;

    Population population (populationSize);
    std::cout << "Population created." << std::endl;
    std::cout << std::endl;

    // This is the main application loop.
    // Repeatedly, we prompt the user to choose a command, and than we act on his or her decision.
    // We break the loop when the user chooses the 'exit' command.
    while (true) 
    {
        std::cout << "Instructions:" << std::endl;
        std::cout << "-------------" << std::endl;
        std::cout << "[Q/W/E/R]: Run 50/25/5/1 evolution steps." << std::endl;
        std::cout << "[A/S/D]:   Display the 10/5/1 best loadouts in the current population." << std::endl;
        std::cout << "[X]:       Exit." << std::endl;

        char command = _getch();
        std::cout << std::endl;

    // NEXT GENERATION

        int evolutionSteps = 0;
        if (command == 'q') evolutionSteps = 50;
        if (command == 'w') evolutionSteps = 25;
        if (command == 'e') evolutionSteps = 5;
        if (command == 'r') evolutionSteps = 1;

        // Run the evolution steps:
        for (int m = 0; m < evolutionSteps; m++) 
        {
            std::cout << "Generation " << m+1 << "/" << evolutionSteps << ": ";
            population.createNextGeneration();

            std::cout << "Done." << std::endl;
        }

    // DISPLAY WINNERS

        size_t displayWinners = 0;
        if (command == 'a') displayWinners = 10;
        if (command == 's') displayWinners = 5;
        if (command == 'd') displayWinners = 1;

        if (displayWinners > 0) 
        {
            // Make sure that we were not asked to display more 
            // mechs than how many there are in the entire population.
            size_t populationSize = population.getSize();
            if (displayWinners > populationSize) 
            {
                std::cout << "There are not that many mechs in the entire population - only " << populationSize << " will be shown.";
                std::cout << std::endl;

                displayWinners = populationSize;
            }

            std::cout << "TOP " << displayWinners << ((displayWinners > 1)? " MECHS" : " MECH") << " IN THE POPULATION: " << std::endl;
            std::cout << "--------------------------------------" << std::endl;
            std::cout << std::endl;

            for (size_t m = 0; m < displayWinners; m++) 
            {
                // Retrieve the mech to show.
                const Mech& mech = population.getMech(m);

                // Print out the mech's position and its score in the last match round.
                std::cout << "TOP #" << m+1 << " (Last score: " << mech.getScore() << ")" << std::endl;

                // Print out the mech's component setup.
                std::cout << mech;
            }
        }

    // EXIT

        // Break the main application loop.
        if (command == 'x') 
            break;

    // CLEAR

        std::cout << std::endl;
    }
}

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

int main (int argc, char* argv[])
{
    try 
    {
        run();
    }
    catch (const std::exception& e) 
    {
        std::cout << std::endl;
        std::cout << "[!] RUNTIME EXCEPTION ==============================" << std::endl;
        std::cout << e.what() << std::endl;

        std::cout << "Press any key to exit." << std::endl;
        std::cin.ignore();
    }

    return 0;
}
share|improve this question
    
Please see Review code on Github? on meta. –  ChrisW Jan 12 at 22:49
    
@ChrisW I am aware that there's a lot of code. I pasted MechArena.cpp, the most important file, into the question now, instead of the Main.cpp which is basically only a launcher. –  electroLux Jan 12 at 23:17
1  
I think you'd better put Main.cpp back into the question (as well as or instead of MechArena.cpp). Given that Jamal already gave an answer based on your Main.cpp it's unfair on him to remove it from your question. And/or discuss with Jamal on Chat about how to handle it. –  ChrisW Jan 12 at 23:24
    
OK, I rollbacked to the previous state. –  electroLux Jan 12 at 23:29
    
ChrisW is correct. If you'd like your other code snippets reviewed, please post them as separate questions. Also try not to include a huge amount of code in one question (we can determine that), which may deter reviews. For more information, see this Meta post. –  Jamal Jan 12 at 23:41
show 6 more comments

2 Answers

up vote 5 down vote accepted

#includes:

  • <time.h> is a C library; use <ctime> instead.
  • Consider omitting <conio.h> as there could be some portability issues. It's also not widely used in C++, especially since you have <iostream>.
  • Add <cstdlib> for std::srand()/std::rand().
  • Add <exception> for std::exception.

Everything else:

  • Only call std::srand() once in an application, preferably in main(). Even if init() will be called only once, keeping it in main() will help with maintainability.

    In addition, prefer to static cast cast to an unsigned int and, especially in C++11, use nullptr instead of NULL or 0 for this use.

    So, you'll have this:

    std::srand(static_cast<unsigned int>(std::time(nullptr)));
    

    As you're not really using any random number generation here (no calls to std::rand()), just omit the relevant headers and the call to std::srand().

    You should refrain from using rand altogether in C++11 as it is considered harmful. Instead, utilize C++11's <random> library.

    You also no longer need <ctime> and should use C++11's <chrono>. From this library, you can obtain a seed for a PRNG with this:

    auto chrono_seed = std::chrono::system_clock::now().time_since_epoch().count();
    
  • init() is needless as it's primarily outputting hard-coded text, which shouldn't be any function's primary task. Just move all the output to main() and remove the function.

    Also, use "\n" for that output instead of std::endl. You don't need all that flushing there.

    std::cout << "=== MECH ARENA ============================\n";
    std::cout << "===========================================\n\n";
    
  • Be careful with catching exceptions here. Make sure you know which exception you're expecting to catch while knowing how to handle it. It appears you're trying to catch a runtime error, so you should catch an std::runtime_error instead.

  • Anything modular 2 is either 0 or 1, so just compare inequality to 0 (or equality to 1) if you're determining if size is an odd number:

    if (size % 2 != 0)
    

    if (size % 2 == 1)
    

    Another (possibly faster) alternative is to use the bitwise AND operator:

    if ((size & 1) == 0)
    
share|improve this answer
    
Good points, thanks a lot. I'll update the online version in a minute: it now uses <ctime>, includes <exception> (it was included via <iostream>), has all the std::endl replaced with '\n', has the entire init() moved into main(), supplies nullptr as the parameter to std::srand, and I rewrote the size % 2 line. Fair point with the <conio.h>, but I think I'll probably keep it, because being able to read the control character from the console without Enter makes the application so much easier to use, and I think that pretty much all the studios that will receive this are on Windows. –  electroLux Jan 12 at 20:47
    
@electroLux: I'm making some additional corrections now. –  Jamal Jan 12 at 20:48
    
If you're talking about pausing the console, there are alternatives that do not use <conio.h>. –  Jamal Jan 12 at 20:49
1  
@electroLux: That's fine, then. I just wanted to at least make you aware of the portability. –  Jamal Jan 12 at 21:29
1  
Both srand and rand are ugly and harmful as well. We have got C++11. Leave that C-style stuff and move on. –  no1 Apr 9 at 18:26
show 8 more comments

There's something which worries me about using this as a sample for a job application: which is that it contains no virtual methods and no subclasses.

No matter how clearly-written your code may be, it doesn't demonstrate an understanding of inheritance: which (I don't know) might be an important criterion in a job application.

If this is valid criticism, perhaps change the game design so as to use inheritance in a natural way.

One (advanced) way to do this might be to have the type of weapon have a different effect depending on the type of armour (as in a game or "rock, paper, sissors", the effect of the weapon varies depending on the target). I'm thinking of this as an example, because a "implementing a video game" is the canonical example used to teach an 'advanced' technique called double-dispatch. Working that implementation (if game-rule have a natural need for that implementation technique) into your sample might impress someone reviewing resumes/samples.

People might also like to see a knowledge of (i.e. appropriate use of some) common design patterns.

My apologies for this advice: it's based on a cynical guess at the type of code which some interviewers may be looking for.

Again based on experience with interview questions, they might also like to know:

  • How do/did you test this?
  • Have you thought about how to extend/maintain it various ways? I mean, add new types of feature: a GUI; playing it over the network; persisting (saving and restoring) game-state; making it a 0-player, 1-player, 2-player, or many-player game; etc.
share|improve this answer
    
My answer is based on a review of the code posted at mz1net.com/code-sample (not of the code quoted above which Jamal was already inspecting). –  ChrisW Jan 12 at 21:42
    
If relevant to this answer at all, consider including some snippets from that linked code in case the link dies. –  Jamal Jan 12 at 21:56
    
Good point. Fortunately, I am asked to deliver several code samples, so besides the two that I already have, I'll probably prepare another one where I will use some polymorphism. I have an idea about a simple Visitor example, so that would also address the design patterns aspect. –  electroLux Jan 12 at 22:21
    
@electroLux Some knowledge of multi-threading / thread-safety is another popular topic, at least in the C++ jobs I apply for. –  ChrisW Jan 12 at 22:24
    
@ChrisW For sure. I think that the issue with the code samples is that I would obviously like them to represent all that I can do, but they are supposed to be quite short, so ultimately I will inevitably only demonstrate only a subset of the skills that the studios would me like me to have. –  electroLux Jan 12 at 22:37
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.