Object Oriented WinForm App

Question

I have started my first foray into object oriented programming and I wanted to find out if what I have done here makes sense:

Pokedex.cs

using PokedexLibrary;
using System.ComponentModel;
using System.Diagnostics;
namespace Pokedex
{
 public partial class Pokedex : Form
 {
 BindingList<string> list = new BindingList<string>();
 List<string> pokemonNames = new List<string>();
 List<Pokemon.PokemonInfo> pokemonData = new List<Pokemon.PokemonInfo>();
 Pokemon p = new Pokemon();
 public Pokedex()
 {
 pokemonData = LoadPokemonData();
 pokemonNames = GetPokemonNames(pokemonData);
 InitializeComponent();
 }
 private void WireUpList()
 {
 foreach (var item in filterResults(SearchBox.Text))
 {
 list.Add(item);
 }
 ResultsWindow.DataSource = list;
 }
 private void SearchBox_TextChanged(object sender, EventArgs e)
 {
 WireUpList();
 }
 private List<string> GetPokemonNames(List<Pokemon.PokemonInfo> pokemonData)
 {
 var list = new List<string>();
 foreach (var item in pokemonData)
 {
 list.Add(item.name);
 }
 return list;
 }
 private List<Pokemon.PokemonInfo> LoadPokemonData()
 {
 return p.LoadPokemonData();
 }
 private List<string> filterResults(string s)
 {
 List<string> match = new List<string>();
 list.Clear();
 if (SearchBox.Text.Length >= 1)
 {
 foreach (var item in pokemonNames)
 {
 if (item.ToLower().Contains(s.ToLower()))
 {
 match.Add(item);
 }
 }
 return match;
 }
 return match;
 }
 private void ResultsWindow_DoubleClick(object sender, EventArgs e)
 {
 Pokemon.PokemonInfo result = p.GetSelectedPokemonData(ResultsWindow.SelectedItem.ToString(), pokemonData);
 flowLayoutPanel1.Visible = true;
 TypeBox.Text = String.Join(Environment.NewLine, result.type);
 PokemonStats.Text = String.Join(Environment.NewLine, p.GetStatsList(result));
 }
 }
}

Pokemon.cs

using Newtonsoft.Json;
using System.ComponentModel;
namespace PokedexLibrary
{
 public class Pokemon
 {
 public class Stats
 {
 public int HP { get; set; }
 public int Attack { get; set; }
 public int Defense { get; set; }
 public int SpAttack { get; set; }
 public int SpDefense { get; set; }
 public int Speed { get; set; }
 }
 public class PokemonInfo
 {
 public int id { get; set; }
 public string name { get; set; }
 public List<string> type { get; set; }
 public Stats @stats { get; set; }
 }
 public List<PokemonInfo> LoadPokemonData()
 {
 List<PokemonInfo> items;
 using (StreamReader r = new StreamReader("file.json"))
 {
 string json = r.ReadToEnd();
 items = JsonConvert.DeserializeObject<List<PokemonInfo>>(json);
 }
 return items;
 }
 public PokemonInfo GetSelectedPokemonData(string pokemon, List<PokemonInfo> p)
 {
 foreach (var item in p)
 {
 if (item.name == pokemon)
 {
 return item;
 }
 }
 return null;
 }
 public List<string> GetStatsList(PokemonInfo p)
 {
 List<string> pStats = new List<string>();
 foreach (PropertyDescriptor descriptor in TypeDescriptor.GetProperties(p.stats))
 {
 string name = descriptor.Name;
 object value = descriptor.GetValue(p.stats);
 pStats.Add(value.ToString() + " = " +name);
 }
 return pStats;
 }
 }
}

The basic idea is that it loads up some information that you can then click on to drill down further.

I am wondering if there is a better way of doing the two following things:

1. In Pokemon.cs I have a Method that accepts an instance of PokemonInfo, this feels a bit odd as the PokemonInfo class is defined within the same class as the Method - Is this an indication that the class is doing too much?

2. A lot of what is done is working with a list of classes, what means I am often passing the list of classes, or the classes themselves around as parameters:

 pokemonData = LoadPokemonData();
 pokemonNames = GetPokemonNames(pokemonData);

Is there a better way of doing this?

Any other comments would also be great!

• radarbob: "A lot of what is done is working with a list of classes" => "working with a list of objects" would be correct.

Answer 1

The Pokemon class has no instance data and contains only methods and nested classes.

The PokemonInfo class, on the other hand, really describes Pokémons.

I suggest making these methods static. This allows you to call them without creating a class instance:

var pokemons = Pokemon.LoadPokemonData();

I also would remove the PokemonInfo class and instead move its properties to the class Pokemon:

public class Pokemon
{
 public class Stats
 {
 ...
 }
 public int id { get; set; }
 public string name { get; set; }
 public List<string> type { get; set; }
 public Stats @stats { get; set; }
 public static List<Pokemon> LoadPokemonData()
 {
 ...
 }
 public static Pokemon GetSelectedPokemonData(string pokemon, List<Pokemon> p)
 {
 ...
 }
 public static List<string> GetStatsList(Pokemon p)
 {
 ...
 }
}

Other points:

• Since now the Pokemon class contains the Pokémon info, the parameter to GetStatsList(Pokemon p) is not required anymore. The method can access the properties directly by making it an instance property again:

  public List<string> GetStatsList()
  {
   var pStats = new List<string>();
   foreach (PropertyDescriptor descriptor in TypeDescriptor.GetProperties(Stats)) {
   string name = descriptor.Name;
   object value = descriptor.GetValue(Stats);
   pStats.Add(value.ToString() + " = " + name);
   }
   return pStats;
  }
  

  Instead of calling Pokemon.GetStatsList(pokemon), now you have to call pokemon.GetStatsList().
• According to common naming conventions for C#, properties should be written in PascalCase (e.g.: Id, Name)
• Move the Stats class to its own file Stats.cs. It makes it easier to find and manage and also removes the name conflict with the property Stats. (Note that the @ enables keywords to be used as identifiers, but does not help with members having the same name.)
• Naming: The method GetSelectedPokemonData has the parameters string pokemon and List<Pokemon> p. These names do not convey the meaning of the method and its parameters. Better:

  public static Pokemon GetFirstPokemonNamed(string name, List<Pokemon> pokemons)
  

• Built-in methods: I understand that you are learning C#. Learning how to find an item in a list is a good exercise. Keep it like this. In a future project you may prefer to use built-in methods. You could use LINQ (Language INtegrated Query):

  Pokemon pokemon = pokemons.FirstOrDefault(p => p.Name == "Baxcalibur");
  

  The filterResults method (which should start with an upper case 'F') could be replaced by (in the context of WireUpList()):

  var filteredResults = pokemonNames
   .Where(name => String.Compare(SearchBox.Text, name,
   StringComparison.InvariantCultureIgnoreCase) == 0);
  

• The private method LoadPokemonData in the form adds no value an can be inlined. I.e., call pokemonData = Pokemon.LoadPokemonData(); directly.

Answer 2

continue on @Olivier answers

Stats class should be moved to it's File and should contain all necessary methods

public class Stats
{
 public int HP { get; set; }
 public int Attack { get; set; }
 public int Defense { get; set; }
 public int SpAttack { get; set; }
 public int SpDefense { get; set; }
 public int Speed { get; set; }
 public List<string> GetStateAsStrings()
 {
 return [
 $"{nameof(HP)} = {HP}",
 $"{nameof(Attack)} = {Attack}",
 $"{nameof(Defense)} = {Defense}",
 $"{nameof(SpAttack)} = {SpAttack}",
 $"{nameof(Speed)} = {Speed}",
 ]; 
 }
 public override string ToString()
 {
 return String.Join(Environment.NewLine, GetStateAsStrings());
 }
}

now you can use it as following

public class Pokemon
{
 public int Id { get; set; }
 public string Name { get; set; } = string.Empty;
 public List<string> Types { get; set; } = []; // Plural Names (i.e collections) should be Plural Name ('Types' insted of `type`)
 public Stats Stats { get; set; } = new();
 // ....
 public List<string> GetStatsList()
 {
 return Stats.GetStateAsStrings();
 }
}

actually, you can Eliminate this method GetStatsList and use Stats directly as following

private void ResultsWindow_DoubleClick(object sender, EventArgs e)
{
 Pokemon selectedPokemon = p.GetSelectedPokemonData(ResultsWindow.SelectedItem.ToString(), pokemonData); // variable names should be representative instead of 'result' better to use 'selectedPokemon' make more sense
 flowLayoutPanel1.Visible = true;
 TypeBox.Text = String.Join(Environment.NewLine, selectedPokemon.Types);
 PokemonStats.Text = selectedPokemon.Stats.ToString(); // Stats know how to render itself as string due to we override the ToString() method
}

Naming

Pay attention on how you Name properties, variables and Methods Name

• Properties and Variables should be descriptive noun and don't use generic or anonymous names, for example in your code BindingList<string> list for the reader it should search on your code what do you do with this magic list it would be better to name it FilteredPokemonNames
• Methods should be Verbs and you do good job in this point by choosing this names LoadPokemonData GetPokemonNames FilterResults

readonly modifier

fields and properties that you initialize and never need to reassign, it's better to mark it as readonly

public partial class Pokedex : Form
{
 readonly BindingList<string> filteredPokemonNames = [];
 readonly List<string> pokemonNames = [];
 readonly List<Pokemon> pokemonData = []; 
 
 // ...
}