current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 5 down vote favorite

This is a simple but complete console application where I experiment with genetic algorithm to try to figure out the best way to construct a battle mech that would win in a simple turn-based combat.

I welcome your criticism and pointers about what I could improve in the code above: architecture-wise, technique-wise, presentation-wise, and whatever else catches your interest. To provide at least a sample code as per the local rules, this is the method to make two mechs meet in combat:

// Match::match()
// ==============
// A static method that can be used to execute a match between the two provided mechs.
// Return value: 0 = draw, 1 = mech #1 wins, 2 = mech #2 wins.

int Match::match (Mech& m1, Mech& m2) {

    m1.resetCombatValues();
    m2.resetCombatValues();

    int turn = 0;
    static const int turnLimit = 25;

    // Each turn:
    while (turn < turnLimit) {

        m1.actCombatTurn (m2);
        m2.actCombatTurn (m1);

        bool mech1Alive = m1.isAlive();
        bool mech2Alive = m2.isAlive();

        if (!mech1Alive && !mech2Alive) return 0;

        if (!mech1Alive) return 2;
        if (!mech2Alive) return 1;

        turn++;

    }

    // Turn limit reached. 
    return 0;

}
share|improve this question
    
Instead of coded return values, I'd consider using an enumeration for clarity: en.cppreference.com/w/cpp/language/enum // A good rule of thumb is that if you have to document a special meaning of (implicit) enumerators, like "0 = draw, 1 = mech #1 wins, 2 = mech #2 wins", then it's probably better to use an explicit enumeration. –  Matt May 25 '13 at 13:36
    
I would also suggest making this a namespace non-member function instead of a static member function: stackoverflow.com/a/1435105/859774 –  Matt May 25 '13 at 13:52
    
Further, unless you're supporting negative turns count turnLimit or want turn to go backward (would that even make sense in your context? BTW, turn++ looks awkward, change it to ++turn), I would use size_t instead of int for turn and turnLimit: en.cppreference.com/w/cpp/types/size_t –  Matt May 25 '13 at 13:56
    
I think I also wouldn't use mech1Alive or mech2Alive but rather their opposites (e.g., mech1Dead or mech2Dead) -- note that you're always negating them in your if statements, so it would improve code clarity to do so straight away. –  Matt May 25 '13 at 13:59
    
You do realize that the rules are there for a reason, right? –  svick May 25 '13 at 15:56

1 Answer 1

up vote 6 down vote accepted

Here's a summary of my comments in the answer format:

  • Instead of coded return values, I'd consider using an enumeration for clarity: http://en.cppreference.com/w/cpp/language/enum
    // A good rule of thumb is that if you have to document a special meaning of (implicit) enumerators, like "0 = draw, 1 = mech #1 wins, 2 = mech #2 wins", then it's probably better to use an explicit enumeration.

  • I would also suggest using a namespace non-member function instead of a static member function whenever possible: http://stackoverflow.com/a/1435105/859774

  • Further, unless you're supporting negative turns count turnLimit or want turn to go backward (would that even make sense in your context? BTW, turn++ looks awkward, change it to ++turn), I would use size_t instead of int for turn and turnLimit: http://en.cppreference.com/w/cpp/types/size_t
    // NOTE: in fact, the cases where int is the right type to choose are significantly rarer than most beginning developers seem to assume -- as a good rule of thumb, whenever you think you should use an int, you should always stop and think whether it's actually a good idea -- e.g., are you sure you really want negative values?

  • I also wouldn't use mech1Alive or mech2Alive but rather their opposites (e.g., mech1Dead or mech2Dead) -- note that you're always negating them in your if statements, so it would improve code clarity to do so straight away, at the outset.

  • I've just looked at the online version, BTW. Your next step should be to remove all uses of new (and, consequently, all manual invocations of delete) and all of the resource-owning raw pointers: preferably change them to (stack-allocated) automatic variables (using references, preferably to const, when avoiding copies is necessary), then only for the ones for which you can't possibly do that (if there are any, don't just assume this, but carefully verify that free-store/heap dynamic allocation is in fact absolutely required!) use std::unique_ptr, and finally, as a last resort, use std::shared_ptr (and, as a last resort after this last resort ;], in the extremely rare case you have need to break a cycle, consider std::weak_ptr).
    See: http://codereview.stackexchange.com/a/25721/24670

share|improve this answer
    
Thanks a lot – I've reworked the structure so that I do not allocate 'on my own' almost anywhere now. I still allocate new Mechs dynamically and only store the pointer within the population vector, because it is quite a heavy structure to move around, and I do move it around quite a lot. I store those as unique_ptr's in the Population that created them, so AFAIU that should be OK. –  electroLux May 26 '13 at 14:11

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.