5
\$\begingroup\$

This is another finished app of mine. It's a traditional pokemon clone simulation but very basic. Could anyone take a look and let me know what I can improve? How would you rate my app overall? Suggestions, corrections, whatever, I'm open to any and all input. I plan on making this a unity game someday so I want to get all the basics right. Right now though I feel like my current program is tightly coupled and a bit of a mess. So ways to make it more professional and scalable?

Full code on pastebin https://pastebin.com/cvaLMQZ9

public static class Program
    {
        // Player choices dictionary
        private static readonly Dictionary<string, string> Choices = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase) {
            { "CatchWildPokemon", "1" },
            { "TrainPokemon", "2" },
            { "Exit", "3" }
        };

        // Yes or No dictionary
        private static readonly Dictionary<string, string> YesOrNo = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase)
        {
            { "Yes", "Y" },
            { "No", "N" },
        };

        private static string Y = YesOrNo["Yes"];
        private static string N = YesOrNo["No"];
        private static string CatchWildPokemon = Choices["CatchWildPokemon"];
        private static string TrainPokemon = Choices["TrainPokemon"];
        private static string Exit = Choices["Exit"];

        private static PlayersCollection PlayersCollection = new PlayersCollection();
        private static GameState GameState = new GameState();
        private static bool IsGameOn;

        private static void Main(string[] args)
        {
            GameState.LoadGame(PlayersCollection);
            IsGameOn = true;
            Menu();
        }

        private static void DisplayMenu()
        {
            Console.WriteLine("\nWhat do you want to do?\n");
            Console.WriteLine($"{CatchWildPokemon}. Catch wild pokemon");
            Console.WriteLine($"{TrainPokemon}. Train your pokemon in gym");
            Console.WriteLine($"{Exit}. Exit");
            Console.WriteLine("");
        }

        public static void Menu()
        {
            while (IsGameOn)
            {
                DisplayMenu();

                var choice = Console.ReadLine();

                // User input validation
                while (!Choices.Any(n => n.Value == choice))
                {
                    Console.WriteLine($"\nPlease type '{CatchWildPokemon}' or '{TrainPokemon}', or exit typing '{Exit}'.\n");
                    choice = Console.ReadLine();
                }

                // Exit game if user chooses so
                if (choice == Exit)
                {
                    GameState.SaveGame(PlayersCollection.GetPlayersCollection());
                    IsGameOn = false;
                    break;
                }

                // Otherwise check player's choice
                else
                {
                    PlayerChoice(choice);
                }
            }
            Console.WriteLine("");
        }

        private static void PlayerChoice(string choice)
        {
            // Catch Wild Pokemon
            if (choice == CatchWildPokemon)
            {
                // Check whether player has caught all wild pokemon
                if (PlayersCollection.IsCollectionCompleted())
                {
                    Console.WriteLine("\nYou've caught all wild pokemon!");
                }
                else
                {
                    ChoiceCatchWildPokemon();
                }
            }

            // Train Pokemon
            else if (choice == TrainPokemon)
            {
                // Check whether player has caught any pokemon yet
                if (PlayersCollection.IsCollectionEmpty())
                {
                    Console.WriteLine("\nYou haven't caught any pokemon yet!");
                }
                else
                {
                    ChoiceTrainPokemon();
                }
            }
        }

        private static void ChoiceCatchWildPokemon()
        {
            Console.WriteLine("\nTime to catch some pokemon!");

            while (true)
            {
                // Caught pokemon
                var caughtPokemon = PlayersCollection.GetRandomWildPokemon();
                Console.WriteLine($"\nYou caught {caughtPokemon.Name} lvl {caughtPokemon.Level}");

                // Display owned pokemon
                var displayOwnedPokemon = new List<string>();

                foreach (var pokemon in PlayersCollection.GetPlayersCollection())
                {
                    displayOwnedPokemon.Add(pokemon.Name + " lvl " + pokemon.Level);
                }

                Console.WriteLine("So far you have caught: " + string.Join(", ", displayOwnedPokemon));

                // Keep catching pokemon (?)
                Console.WriteLine("\nDo you want to keep catching pokemons? (Y/N)\n");

                var keepCatchingPokemon = Console.ReadLine().ToUpperInvariant();

                // User input validation
                while (!YesOrNo.Any(x => x.Value == keepCatchingPokemon))
                {
                    Console.WriteLine($"\nPlease type '{Y}' or '{N}'\n");
                    keepCatchingPokemon = Console.ReadLine().ToUpperInvariant();
                }

                // Go back to Menu if user chooses so
                if (keepCatchingPokemon.Equals(N))
                {
                    break;
                }

                // Check that there are wild pokemon left
                if (PlayersCollection.IsCollectionCompleted())
                {
                    Console.WriteLine("\nYou've caught all wild pokemon!");
                    break;
                }
            }
        }

        // Train Pokemon player choice
        private static void ChoiceTrainPokemon()
        {
            Console.WriteLine("\nTime to head to pokemon gym!");
            var trainPokemon = true;

            while (trainPokemon)
            {
                // Display owned pokemon
                var displayOwnedPokemon = new List<string>();

                foreach (var pokemon in PlayersCollection.GetPlayersCollection())
                {
                    displayOwnedPokemon.Add(pokemon.Name + " lvl " + pokemon.Level);
                }

                Console.WriteLine("\nOwned pokemon: " + string.Join(", ", displayOwnedPokemon));

                // Variables
                var isValidPokemon = false;
                var isTeamSet = false;
                var gymTeam = new List<Pokemon>();
                var displayGymTeam = new List<string>();

                // Add pokemon to Gym Team while it isn't set
                do
                {
                    // Gym team is automatically set if player has chosen 6 pokemon or it contains all their pokemon
                    if (gymTeam.Count == 6 || gymTeam.Count == PlayersCollection.GetPlayersCollection().Count)
                    {
                        break;
                    }

                    // Display current gym team if at least one pokemon has been added
                    if (gymTeam.Count > 0 && isValidPokemon)
                    {
                        // Display current gym team
                        Console.WriteLine("\nCurrent pokemon in your gym team: " + String.Join(", ", displayGymTeam));

                        // Team is set if player doesn't want to add another pokemon to their team
                        if (!AddAnotherPokemonToGymTeam())
                        {
                            isTeamSet = true;
                            break;
                        }
                    }

                    // Validate player's pokemon and add it to their team
                    do
                    {
                        isValidPokemon = HasPokemonBeenAddedToGymTeam(gymTeam, displayGymTeam);
                    }
                    while (!isValidPokemon);

                } while (!isTeamSet);

                // Ask player if they want to train again and if so, if they want the same team as last time
                do
                {
                    // Display trained pokemon
                    var displayTrainedPokemon = new List<string>();

                    foreach (var pokemon in gymTeam)
                    {
                        displayTrainedPokemon.Add(pokemon.Name + " lvl " + pokemon.Level);
                    }

                    Console.WriteLine($"\nYour pokemon [{string.Join(", ", displayTrainedPokemon)}] gained experience!");

                    // Increase pokemon experience points
                    foreach (var pokemon in gymTeam)
                    {
                        PokemonLevelSystem.IncreasePokemonExp(pokemon);

                        // Check whether any pokemon has leveled up
                        if (PokemonLevelSystem.HasPokemonLeveledUp(pokemon))
                        {
                            Console.WriteLine(pokemon.Name + " has leveled up!");
                        }
                        if (PokemonLevelSystem.HasPokemonReachedMaxLevel(pokemon))
                        {
                            Console.WriteLine(pokemon.Name + " has reached max level! ");
                        }
                    }

                    // Go back to Menu if user chooses so
                    if (!TrainAgain())
                    {
                        trainPokemon = false;
                        break;
                    }

                } while (TrainSameTeam());
            }
        }


        private static bool TrainAgain()
        {
            // Train pokemon in gym again (?)
            Console.WriteLine("\nDo you want to train again? (Y/N)\n");

            var keepTrainingPokemon = Console.ReadLine().ToUpperInvariant();

            // User input validation
            while (!YesOrNo.Any(x => x.Value == keepTrainingPokemon))
            {
                Console.WriteLine($"\nPlease type '{Y}' or '{N}'\n");
                keepTrainingPokemon = Console.ReadLine().ToUpperInvariant();
            }

            // Go back to Menu if Player chooses so
            if (keepTrainingPokemon.Equals(N))
            {
                return false;
            }
            else
            {
                return true;
            }
        }


        private static bool TrainSameTeam()
        {
            // Train the same pokemon (?)
            Console.WriteLine("\nDo you want to train the same pokemon? (Y/N)\n");

            var trainSameTeam = Console.ReadLine().ToUpperInvariant();

            // User input validation
            while (!YesOrNo.Any(x => x.Value == trainSameTeam))
            {
                Console.WriteLine($"\nPlease type '{Y}' or '{N}'\n");
                trainSameTeam = Console.ReadLine().ToUpperInvariant();
            }

            if (trainSameTeam.Equals(N))
            {
                return false;
            }
            else
            {
                return true;
            }
        }

        // Returns whether player's chosen pokemon has been successfully added to their gym team
        private static bool HasPokemonBeenAddedToGymTeam(List<Pokemon> gymTeam, List<string> gymTeamInfo)
        {
            Console.WriteLine("\nType the name of a pokemon from your collection that you'd like to add to your gym team.");
            Console.WriteLine("You must add between 1 and 6 pokemon in your gym team.\n");

            var chosenPokemon = Console.ReadLine();

            if (String.IsNullOrWhiteSpace(chosenPokemon))
            {
                return false;
            }

            chosenPokemon = chosenPokemon.Trim().ToLowerInvariant();
            chosenPokemon = chosenPokemon.First().ToString().ToUpper() + chosenPokemon.Substring(1);

            // Check that player's input is a valid owned pokemon
            if (!PlayersCollection.GetPlayersCollection().Any(p => p.Name == chosenPokemon))
            {
                return false;
            }
            else
            {
                // Check that the pokemon wasn't already added in player's gym team
                if (gymTeam.Any(p => p.Name == chosenPokemon))
                {
                    Console.WriteLine($"\nYou already added {chosenPokemon}.");
                    return false;
                }
                else
                {
                    // Add unique pokemon to Gym Team
                    var pokemon = PlayersCollection.GetPlayersCollection().SingleOrDefault(p => p.Name == chosenPokemon);
                    gymTeam.Add(pokemon);
                    gymTeamInfo.Add(pokemon.Name + " lvl " + pokemon.Level);

                    return true;
                }
            }
        }

        // Returns whether player wants to add another pokemon to their gym team
        private static bool AddAnotherPokemonToGymTeam()
        {
            Console.WriteLine("\nDo you want to add another pokemon to your gym team? (Y/N)\n");

            var addAnotherPokemonToGymTeam = Console.ReadLine().ToUpperInvariant();

            // User input validation
            while (!YesOrNo.Any(x => x.Value == addAnotherPokemonToGymTeam))
            {
                Console.WriteLine($"\nPlease type '{Y}' or '{N}'\n");
                addAnotherPokemonToGymTeam = Console.ReadLine().ToUpperInvariant();
            }

            if (addAnotherPokemonToGymTeam.Equals(N))
            {
                return false;
            }
            else
            {
                return true;
            }
        }
    }

Pokemon.cs

    [Serializable]
    public class Pokemon
    {
        public string Name { get; set; }     
        public string Type { get; set; }     
        public int Level { get; set; } = 5;    
        public int Exp { get; set; } = 0;
    }

PokemonGenerator.cs

public static class PokemonGenerator
    {
        public static IList<Pokemon> PokemonInWorld = new List<Pokemon>();

        static PokemonGenerator()
        {
            PopulateWorldWithPokemon();
        }

        private static void PopulateWorldWithPokemon()
        {
            PokemonInWorld.Add(new Pokemon { Name = "Pikachu" });
            PokemonInWorld.Add(new Pokemon { Name = "Eevee" });
            PokemonInWorld.Add(new Pokemon { Name = "Charmander" });
            PokemonInWorld.Add(new Pokemon { Name = "Pachirisu" });
            PokemonInWorld.Add(new Pokemon { Name = "Riolu" });
            PokemonInWorld.Add(new Pokemon { Name = "Vulpix" });
            PokemonInWorld.Add(new Pokemon { Name = "Squirtle" });
            PokemonInWorld.Add(new Pokemon { Name = "Magikarp" });
            PokemonInWorld.Add(new Pokemon { Name = "Snorlax" });
            PokemonInWorld.Add(new Pokemon { Name = "Jigglypuff" });
            PokemonInWorld.Add(new Pokemon { Name = "Togepi" });
            PokemonInWorld.Add(new Pokemon { Name = "Growlithe" });
            PokemonInWorld.Add(new Pokemon { Name = "Abra" });
            PokemonInWorld.Add(new Pokemon { Name = "Pidgey" });
            PokemonInWorld.Add(new Pokemon { Name = "Togepi" });
            PokemonInWorld.Add(new Pokemon { Name = "Sandshrew" });
            PokemonInWorld.Add(new Pokemon { Name = "Zubat" });
            PokemonInWorld.Add(new Pokemon { Name = "Meowth" });
            PokemonInWorld.Add(new Pokemon { Name = "Ponyta" });
            PokemonInWorld.Add(new Pokemon { Name = "Seel" });
            PokemonInWorld.Add(new Pokemon { Name = "Dratini" });
            PokemonInWorld.Add(new Pokemon { Name = "Cyndaquil" });
            PokemonInWorld.Add(new Pokemon { Name = "Sentret" });
            PokemonInWorld.Add(new Pokemon { Name = "Aipom" });
            PokemonInWorld.Add(new Pokemon { Name = "Treecko" });
            PokemonInWorld.Add(new Pokemon { Name = "Taillow" });
            PokemonInWorld.Add(new Pokemon { Name = "Azurill" });
            PokemonInWorld.Add(new Pokemon { Name = "Skitty" });
            PokemonInWorld.Add(new Pokemon { Name = "Monferno" });
            PokemonInWorld.Add(new Pokemon { Name = "Treecko" });
            PokemonInWorld.Add(new Pokemon { Name = "Luxio" });
            PokemonInWorld.Add(new Pokemon { Name = "Sneasel" });
        }
    }

PlayersCollection.cs

public class PlayersCollection
    {
        private List<Pokemon> _playersCollection = new List<Pokemon>();
        private int _totalNumberOfWildPokemon = PokemonGenerator.PokemonInWorld.Count;

        public Pokemon GetRandomWildPokemon()
        {
            var randomPokemon = RandomPokemonGenerator.GetRandomPokemon();

            // Check that random pokemon is unique in player's collection
            while (_playersCollection.Contains(randomPokemon))
            {
                randomPokemon = RandomPokemonGenerator.GetRandomPokemon();
            };

            AddPokemonToPlayersCollection(randomPokemon);

            return randomPokemon;
        }

        public void AddPokemonToPlayersCollection (Pokemon pokemon)
        {
            _playersCollection.Add(pokemon);
        }

        public List<Pokemon> GetPlayersCollection()
        {
            return _playersCollection;
        }

        public bool IsCollectionCompleted()
        {
            return _playersCollection.Count == _totalNumberOfWildPokemon;
        }

        public bool IsCollectionEmpty()
        {
            return _playersCollection.Count == 0;
        }
    }

RandomPokemonGenerator.cs

public static class RandomPokemonGenerator
    {
        private static Random _random = new Random();
        private static Pokemon _randomPokemon = new Pokemon();
        private static int _totalPokemons = PokemonGenerator.PokemonInWorld.Count;

        public static Pokemon GetRandomPokemon()
        {
            var randomNum = _random.Next(_totalPokemons - 1);
            _randomPokemon = PokemonGenerator.PokemonInWorld[randomNum];

            return _randomPokemon;
        }
    }

PokemonLevelSystem.cs

public static class PokemonLevelSystem
    {
        private static int ExpPoints = 20;
        private static int ExpPointsLimit = 100;
        private static int MaxLevel = 7;

        public static void IncreasePokemonExp(Pokemon pokemon)
        {
            if(pokemon.Level < MaxLevel)
            {
                pokemon.Exp += ExpPoints;
            }
        }

        public static bool HasPokemonLeveledUp(Pokemon pokemon)
        {
            if (pokemon.Exp >= ExpPointsLimit && pokemon.Level < MaxLevel)
            {
                pokemon.Level++;
                pokemon.Exp -= ExpPointsLimit;

                return true;
            }
            else
            {
                return false;
            }
        }

        public static bool HasPokemonReachedMaxLevel(Pokemon pokemon)
        {
            if(pokemon.Level == MaxLevel)
            {
                return true;
            }
            else
            {
                return false;
            }
        }
    }

GameState.cs

public class GameState
{
        private static string GetPath()
        {
            return @"C:\SavedPokemonGame\mySavedPokemonData.bin";
        }

        public static void SaveGame(List<Pokemon> pokemonList)
        {
            using (Stream stream = File.Open(GetPath(), FileMode.Create))
            {
                var bformatter = new 
           System.Runtime.Serialization.Formatters.Binary.BinaryFormatter();

            bformatter.Serialize(stream, pokemonList);
            }
         }

        public static void LoadGame(PlayersCollection PlayersCollection)
        {
            if (File.Exists(GetPath()))
            {
                using (Stream stream = File.Open(GetPath(), FileMode.Open))
                {
                    var bformatter = new 
      System.Runtime.Serialization.Formatters.Binary.BinaryFormatter();

                    List<Pokemon> retrievedPokemonList = 
      (List<Pokemon>)bformatter.Deserialize(stream);

                    foreach (var pokemon in retrievedPokemonList)
                    {

                   PlayersCollection.AddPokemonToPlayersCollection(pokemon);
                    }
                }
            }
       }
}
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

Code is pretty long then some issues apply to multiple cases.

First and most important thing is that you have too many static methods. You're not using objects and this makes your code hard to test. Can you, for example, test RandomPokemonGenerator in isolation? No, because it requires almost all the other classes and they can't be mocked because most of them are static. Using instances you solve this issue nicely:

sealed class RandomPokemonPicker : IPokemonPicker
{
    public RandomPokemonPicker (IPokemonWorld world)
    {
        Debug.Assert(world != null);

        _world = world;
    }

    public Pokemon Next()
    {
        Debug.Assert(_world != null);
        Debug.Assert(_world.Population.Count > 0, "Oops all Pokemon died!");

         int randomPokemonId = _random.Next(_world.Population.Count);
         return _world.Population[randomPokemonId];
    }

    private readonly Random _random = new Random();
    private readonly IPokemonWorld _world;
}

Few notes. I changed name from generator to picker (better names are welcome) because it's not generating your Pokemons but picking them from an existing list. Fields you initialize only in ctor should be marked readonly; you do not need to keep track of latest generated pokemon; I'm using assertion to assert about pre-conditions, this is extremely useful during debugging and when running your tests in the development machine (then probably compiled in debug). Class is not intended to be inherited and it has not any extension point (so far) then it's marked as sealed. I'm using an interface IRandomPokemonPicker which exposes a single Next() function (no need to repeat Pokemon in the method name if it's already in the class name), this will help you to mock this class for testing.

Note: to me this logic seems little bit too naive, the same Pokemon can be picked multiple times and another one might be never picked. If you want to exhaust all available Pokemons then easier way, I think, is to put them in a list and then shuffle the list. To pick them just enumerate the list and you will have 'em all.

From this you can see that also PokemonGenerator has to be a concrete class you have to instantiate, it's your world IPokemonWorld (implemented by PolemonWorld. Also, following the Single Responsibility Principle to fill the world with Pokemons is a task that should be done by someone else, that's a generator PokemonGenerator and it should be a separate class. Note that to generate Pokemons and to put them in the world are separate responsibilities, proof of concept:

sealed class PokemonWorld : IPokemonWorld
{
    public IReadOnlyPokemonCollection Population => _population;

    public void Populate(IPokemonGenerator generator, int maximumNumberOfPokemon)
    {
        Debug.Assert(generator != null);
        Debug.Assert(_population != null);
        Debug.Assert(_population.Count == 0, "There are enough Pokemon in this world");

       _population.AddRange(generator.Generate().Take(maximumNumberOfPokemon));

       Debug.Assert(_population.Count > 0 && _population.Count <= maximumNumberOfPokemon));
    }

    private readonly PokemonCollection _population = new PokemonCollection();
}

I think you now understand what I mean then I won't repeat this for all the other classes (but come back for a Code Review after you did this!)


In general your methods are too long, it means they're doing too many things and that's Bad ™. You should split them, for example to ask for confirmation might be reused multiple times:

private static bool Confirm(string prompt)
{
    do
    {
        Console.WriteLine($"{prompt} [{KeyYes}/{KeyNo}]");
        var input = Console.ReadLine();

        if (String.Equals(input, KeyYes, StringComparer.InvariantCultureIgnoreCase))
            return true;

        if (String.Equals(input, KeyNo, StringComparer.InvariantCultureIgnoreCase))
            return false;
    } while (true);
}

Note that I'm not doing case insensitive string comparison with ToUpper()/ToLower() because String.Equals() already accepts a StringComparer and if you will publish your game for other cultures you will then need to use current culture (and ToUpper()/ToLower() will then fail).

Reading your own methods you will see that there are blocks that perform a task. Move them to separate methods and you will probably see more blocks of methods: they're new classes.


Naming conventions: try to follow standard naming conventions (I won't repeat all the guidelines here).

Sometimes you have methods which might be properties, usually I reserve methods for actions and properties for state or attributes (unless something perceived as a state implies a computation intensive calculation or it might fail somehow). For example:

 public PlayerCollection Players => _players;
 public bool IsCompleted => _players.Count == _totalNumberOfWildPokemon;
 public bool IsEmpty => _players.Count == 0;

Note that you do not need to repeat Pokemon and Collection everywhere, if IsEmpty is a property of, for example, PokemonCollection then it's implicit and you do not expose an implementation detail in your names.


Error checking: you're not validating anything. All inputs should be validated, at least with assertions if they are not public.


Abstractions and separation. You might want to abstract input/output to a separate class/interface. Now you're using Console but in future you may want to give an UI to your game and all the logic must be the same. Better, you should separate UI logic and game logic (because they will vary independently) and they need to be tested separately. In your CuiBasedInteraction you will then abstract the underlying implementation detail (the console) into ConsoleIoChannel, if for example you will need to add colors then you will need to change only one class...

\$\endgroup\$
9
  • \$\begingroup\$ Thanks a lot for your answer man! Extremely helpful! I'm still trying to understand everything you listed though, especially the interfaces, I actually don't know what exactly should be inside PokemonGenerator, PokemonCollection and PokemonWorld, I understand that one needs to create the pokemon and PokemonWorld needs to add all those pokemon to the world but it's hard to think how to implement all these (plus their interfaces!). But I'm working on it thanks man \$\endgroup\$
    – user123850
    Commented Jul 13, 2017 at 10:48
  • \$\begingroup\$ You can start without interfaces, just classes. You may need interfaces later because of testing but for a simple domain model you might go with abstract base classes. Start making a class for each responsibility (cum grano salis...) and everything should just flow \$\endgroup\$ Commented Jul 13, 2017 at 10:51
  • \$\begingroup\$ I see! Thanks, I do see that it's definitely slower haha. Okay just for clarification, PokemonGenerator should have 1 prop field List PokemonCollection and 1 method Generate() that will add new pokemons to this list. Therefore PokemonCollection will contain only 1 field with all pokemon, and PokemonWorld, like you showed, will add a given max number of the PokemonCollection list to the Population property. Right? Or am I messing things up again \$\endgroup\$
    – user123850
    Commented Jul 13, 2017 at 11:16
  • \$\begingroup\$ Hmmmm, I think no. PokemonGenerator should ONLY create a sequence of new Pokemon (if you're using a built-in list you can use yeld return and keep your actual code, just return new pokemons instead of add them a list). This is...a generator. Only that. Now create a PokemonWorld which has a property PokemonCollection (let's leave out the read-only stuff for now) named, for example, Population. That collection is simply sealed class PokemonCollection : Collection<Pokemon> { }. \$\endgroup\$ Commented Jul 13, 2017 at 11:21
  • 1
    \$\begingroup\$ Got it! Thanks a lot for your help! Will work on everything and then post again in a new question! \$\endgroup\$
    – user123850
    Commented Jul 13, 2017 at 13:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.