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

Generating a zoo of animals

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

Problem

The pattern I've come up here is for the AnimalGenerator itself, outside of the (abstract) Factory Pattern used within it. The factory classes themselves are not present in this code because that is not what I'm focussing on here.

There are many types of animals, and AnimalGenerator::generateAnimals(num, int...) is to generate num specific types of animals (randomly chosen) that are of category Type with the int... pack specifying how many there are of each specific type.

```
#include
#include
#include
#include
#include
#include
#include
#include

class Animal { public: virtual std::string tag() const = 0; };
class Dog : public Animal { virtual std::string tag() const override {return "Dog";} };
class Cat : public Animal { virtual std::string tag() const override {return "Cat";} };
class Trout : public Animal { virtual std::string tag() const override {return "Trout";} };
class Whale : public Animal { virtual std::string tag() const override {return "Whale";} };
class Wolf : public Animal { virtual std::string tag() const override {return "Wolf";} };
class Deer : public Animal { virtual std::string tag() const override {return "Deer";} };
class Fox : public Animal { virtual std::string tag() const override {return "Fox";} };
class Shark : public Animal { virtual std::string tag() const override {return "Shark";} };

enum {Land, Water};

class AnimalGenerator {
public:
template static inline std::list generateAnimals (int, Args...);
private:
template struct CreateNewAnimals;
template struct Animals;
template static inline std::list createNewAnimals (int, int);
};

template <>
struct AnimalGenerator::Animals {
using type = std::tuple;
static const int size = std::tuple_size::value;
};

template <>
struct AnimalGenerator::Animals {
using type = std::tuple;
static const int size = std::tuple_size::value;
};

template
inline std::list AnimalGenerator::generateAnimals (int numTypes, Args...

Solution

-
Those child classes may be somewhat short, but they would still look more readable as multiple lines. Also, if they ever need to be maintained, it would be harder to do so with them as single lines. Rule of thumb: code is primarily meant to be read vertically.

-
The enum looks a little unusual as it's lacking a name that describe the types:

enum {Land, Water};


Try something like this:

enum Environment { Land, Water };


-
Since you appear to be using C++11, you should consider some C++11 alternatives, such as std::shuffle() instead of std::random_shuffle(). The former requires an RNG as an argument, so you're not stuck using the deprecated and unsafe std::rand(). You should also use this in place of std::srand().

-
Regarding std::srand() anyway, seed is wrong since std::time() doesn't return an int, but an std::time_t. You should especially be careful about types when it comes to time.

-
You probably don't need to use std::endl at all in main(). It also flushes the buffer, so it's generally slower, especially when used repeatedly. Newlines can also be outputted using "\n", which doesn't do any flushing. See this post for more information.

Code Snippets

enum {Land, Water};
enum Environment { Land, Water };

Context

StackExchange Code Review Q#79900, answer score: 2

Revisions (0)

No revisions yet.