Skip to main content
Code Review

Return to Answer

edited body
Source Link
Olivier Jacot-Descombes
Olivier Jacot-Descombes
  • 5.9k
  • 22
  • 27
private void ResultsWindow_DoubleClick(object sender, EventArgs e)
{
    Pokemon SelectedPokemonselectedPokemon = 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, SelectedPokemonselectedPokemon.Types);
    PokemonStats.Text = SelectedPokemonselectedPokemon.Stats.ToString(); // Stats know how to render itself as string due to we override the ToString() method
}

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
}

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
}
added 360 characters in body
Source Link
Ibram Reda
Ibram Reda
  • 566
  • 2
  • 10

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 = []; 
    
    // ...
}

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 = []; 
    
    // ...
}
Source Link
Ibram Reda
Ibram Reda
  • 566
  • 2
  • 10

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