HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

'Genetic algorithm' implementation for constructing a battle mech

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
battlegeneticalgorithmconstructingformechimplementation

Problem

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;

}

Solution

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: https://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: https://codereview.stackexchange.com/a/25721/24670

Context

StackExchange Code Review Q#26600, answer score: 7

Revisions (0)

No revisions yet.