6
\$\begingroup\$

this was my first thing i made on my own without any help. im sure theres a lot of improvement that could be made, i dont feel too confident about this. how can i make it shorter and more efficient? should this entire thing be rewritten in some other way? refactored? keep in mind if i wanted to add more birds to the list later. im also confused on when to use public, static, and void.

class Bird
    {
        public string name;
        public string size;
        public string color;
        public string habitat;
        public Bird(string name, string size, string color, string habitat)
        {
            this.name = name;
            this.size = size;
            this.color = color;
            this.habitat = habitat;
        }
    }
    class Program
    {
        public static List<Bird> birds = new List<Bird>();
        public static void CreateList()
        {
            birds.Add(new Bird("Bald Eagle", "large", "white", "America"));
            birds.Add(new Bird("American Kestrel", "small", "brown", "America"));
            birds.Add(new Bird("Mississippi Kite", "medium", "grey", "America"));
            birds.Add(new Bird("Red Kite", "medium", "brown", "Europe"));
            birds.Add(new Bird("Secretary Bird", "large", "grey", "Africa"));
            birds.Add(new Bird("Shoebill Stork", "large", "grey", "Africa"));
            birds.Add(new Bird("Cockatoo", "medium", "white", "Australia"));
        }
        public static void Start()
        {
            Console.WriteLine("Welcome to the Bird of prey identification helper.");
            Console.WriteLine("(1) List of all birds and their descriptions\n(2) Identification help.\n");

            string input;
            do
            {
                Console.Write("Enter 1 or 2: ");
                input = Console.ReadLine();
                if (input == "1")
                {
                    BirdList();
                }
                if (input == "2")
                {
                    BirdQuestions();
                }
            } while (input != "1" && input != "2");
        }
        public static void BirdList()
        {
            Console.Clear();
            foreach (var bird in birds)
                Console.WriteLine("Name: {0} | Size: {1} | Main Color: {2} | Habitat Location: {3}", bird.name, bird.size, bird.color, bird.habitat);
        }
        public static string colorInput;
        public static string sizeInput;
        public static string habitatInput;
        public static void BirdQuestions()
        {
            Console.Clear();
            Console.WriteLine("This process will help you through identifying a bird you have seen.");

            do
            {
                Console.WriteLine("\nWhat was the birds main color? Enter brown, grey, or white.");
                Console.Write("Enter: ");
                colorInput = Console.ReadLine();
            } while (colorInput != "brown" && colorInput != "grey" && colorInput != "white");

            do
            {
                Console.WriteLine("\nHow large or small was the bird? Enter small, medium, or large.");
                Console.Write("Enter: ");
                sizeInput = Console.ReadLine();
            } while (sizeInput != "small" && sizeInput != "medium" && sizeInput != "large");

            do
            {
                Console.WriteLine("\nWhere did you see the bird? Enter America, Australia, Europe, or Africa.");
                Console.Write("Enter: ");
                habitatInput = Console.ReadLine();
                
            } while (habitatInput != "America" && habitatInput != "Australia" && habitatInput != "Africa" && habitatInput != "Europe");

            BirdId();
        }
        public static void BirdId()
        {
            foreach (var bird in birds)
            {
                if (colorInput == bird.color && sizeInput == bird.size && habitatInput == bird.habitat)
                {
                    Console.WriteLine("\n" + bird.name); 
                }
                else
                {
                    Console.WriteLine("\nNo birds found.");
                    break;
                }
            }
        }
        static void Main(string[] args)
        {
            CreateList();
            Start();
        }
    }
```
\$\endgroup\$
6
\$\begingroup\$

Welcome there new programmer :-)

For the first piece of code it's looking good and of course a lot can be rewritten or done in a different way, but a lot of it also depends on someone's programming style/experience. Don't worry about it too much since this will improve more and more the longer you craft software.

What I did is rewrite some parts of your code and put in some comments for you to learn from. Take a look at it and if you have questions just ask.

What editor do you use? If you take a close look on what hints it gives you can also learn/improve your code!

using System;
using System.Collections.Generic;
using System.Linq;

class Bird
{
    // Use properties instead of members. This is safer. Also start publics with a capital.
    // Most of the times the properties are public in (POCO) classes like this
    //
    // set; is also availble, but since we set it from the constructor we want this to be controlled like this.
    public string Name { get; }
    public string Size { get; }
    public string Color { get; }
    public string Habitat { get; }

    public Bird(string name, string size, string color, string habitat)
    {
        Name = name;
        Size = size;
        Color = color;
        Habitat = habitat;
    }
}

class Program
{
    // Use a collection initializer - it is cleaner code
    // Mark it as readonly so Birds cannot be overwritten somewhere (except in constructor)
    static readonly List<Bird> Birds = new List<Bird>
    {
        new Bird("Bald Eagle", "large", "white", "America"),
        new Bird("American Kestrel", "small", "brown", "America"),
        new Bird("Mississippi Kite", "medium", "grey", "America"),
        new Bird("Red Kite", "medium", "brown", "Europe"),
        new Bird("Secretary Bird", "large", "grey", "Africa"),
        new Bird("Shoebill Stork", "large", "grey", "Africa"),
        new Bird("Cockatoo", "medium", "white", "Australia")
    };

    // The list of possible actions I've put in a dictionary. This eliminates the manual writing of a lot of ifs
    static readonly Dictionary<string, Action> Actions = new Dictionary<string, Action>
    {
        {"1", BirdList}, 
        {"2", BirdQuestions},
    };

    // Use private access modifier when only the containing class is using the method or property
    // Since this all is initiated in a static Main method you are bound to the static context
    // unless you instantiate a new object (class) inside that Main method; this can have plain public and private methods.
    // Check https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/access-modifiers
    // and https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/static-classes-and-static-class-members
    // for more info on this topic.
    private static void Start()
    {
        Console.WriteLine("Welcome to the Bird of prey identification helper.");
        Console.WriteLine("(1) List of all birds and their descriptions\n(2) Identification help.\n");

        string input;
        do
        {
            Console.Write("Enter 1 or 2: ");
            input = Console.ReadLine();
            if (Actions.ContainsKey(input))
            {
                Actions[input](); // call the action that belongs to the input (in Dictionary)
            }
        } while (!Actions.ContainsKey(input));
    }

    static void BirdList()
    {
        Console.Clear();
        foreach (var bird in Birds)
            Console.WriteLine(
                // Use string interpolation  (with the $ before the ") so you can directly put in the variables in the string
                $"Name: {bird.Name} | Size: {bird.Size} | Main Color: {bird.Color} | Habitat Location: {bird.Habitat}");
    }

    static void BirdQuestions()
    {
        // Pass local variables to the next method - makes it more clear how the code flow is, instead of magically setted
        // values in some method and reads in other methods.
        string colorInput;
        string sizeInput;
        string habitatInput;

        Console.Clear();
        Console.WriteLine("This process will help you through identifying a bird you have seen.");

        // Prevent a lot of 'hard coded variables' - the options are based on the Birds list I assume
        // Let's get a distinct list of colors from the birds collection
        var colors = Birds.Select(s => s.Color).Distinct();
        
        // create a comma separated string of all the colors
        var colorsString = string.Join(", ", colors);
        do
        {
            Console.WriteLine($"\nWhat was the birds main color? Enter {colorsString}.");
            Console.Write("Enter: ");
            colorInput = Console.ReadLine();
        } while (!colors.Contains(colorInput));

        // Now all the possible colors hard codings have been eliminated 
        
        
        var sizes = Birds.Select(s => s.Size).Distinct();
        var sizesString = string.Join(", ", sizes);
        do
        {
            Console.WriteLine($"\nHow large or small was the bird? Enter {sizesString}.");
            Console.Write("Enter: ");
            sizeInput = Console.ReadLine();
        } while (!sizes.Contains(sizeInput));

        var habitats = Birds.Select(s => s.Habitat).Distinct();
        var habitatsString = string.Join(", ", habitats);
        do
        {
            Console.WriteLine($"\nWhere did you see the bird? Enter {habitatsString}.");
            Console.Write("Enter: ");
            habitatInput = Console.ReadLine();
        } while (!habitats.Contains(habitatInput));

        BirdId(colorInput, sizeInput, habitatInput);
    }

    static void BirdId(string colorInput, string sizeInput, string habitatInput)
    {
        // Use LINQ to iterate over a collection - increases readability in cases you have a big if condition on properties
        var foundBirds = Birds.Where(bird =>
            bird.Color == colorInput &&
            bird.Size == sizeInput &&
            bird.Habitat == habitatInput).Select(bird => bird.Name);

        if (foundBirds.Any())
        {
            Console.WriteLine("\n" + string.Join('\n', foundBirds));
        }
        else
        {
            Console.WriteLine("\nNo birds found.");
        }
    }

    static void Main(string[] args)
    {
        Start();
    }
}
```
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ +1 but small nit - that's a collection initializer not an object initializer. \$\endgroup\$ – RobH Aug 14 at 17:51
  • \$\begingroup\$ @RobH you’re absolutely right! I’ll change the comment, even though it’s an object in the end ;-) \$\endgroup\$ – 321X Aug 14 at 17:58
  • \$\begingroup\$ Awesome review. Another small nit, the foundBirds enumeration is being enumerated twice (partially first time). Perhaps by design but worth noting. Doing a ToList or ToArray would avoid it, especially given the known small resultset. \$\endgroup\$ – TheSoftwareJedi Aug 14 at 21:29
  • \$\begingroup\$ Thanks :-) I know, that’s why I asked him what editor he uses. Some give hints, like Rider. It’s a review to challenge him to learn more about C#, and since he didn’t use LINQ in his code I assume he doesn’t know (much) about it (yet). Going into all details would have been too much imho. \$\endgroup\$ – 321X Aug 14 at 22:33
2
\$\begingroup\$

Well 321X has a good review, I'm just adding some other points that would benefit you since you're new. I'll give you some points, then I'll link it to your code so you get the full picture.

First, designing your application MUST be flexible for any future changes. Not all applications needs to be flexible as some applications might be fixed (do it once and forget it), however, it is important as a programmer to consider changes in the long-term even for fixed applications (or as I call them static applications). This means, you need to separate your logic from the application environment (in your case Console). Which would add more flexibility to your application and easier to maintain and adapt or convert to another environment (such as converting from Console to Windows Form) .

Constants MUST be declared as const or enum or readonly objects. That is, if you have some options in your application (like colors, size, and habitats) always initiate them as const or enum or readonly and keep them inside their related class (or struct if so).

Global variables (public or private) always declare them at the top of the class for better readability.

Always use the correct datatype, and use string only for output. So, in your case, user input will be string always, you need to parse that input to the correct datatype. For instance, at start you list 2 options to the user, and user needs to input either 1 or 2. You should try parse it to an actual int which is the correct data type for this input.

Always use access modifiers (private, public, protected, internal ..etc). Keep it visible, as it would increase your code readability.

Always use PascalCase in class, struct, method, Functions and Properties. And camelCase for the rest.

Now to the actual code :

class Bird
    {
        public string name;
        public string size;
        public string color;
        public string habitat;
        public Bird(string name, string size, string color, string habitat)
        {
            this.name = name;
            this.size = size;
            this.color = color;
            this.habitat = habitat;
        }
    }

Color, Size, and Habitat are constants strings, you should use enum instead.

public enum BirdColor
{
    Default,
    Brown,
    Grey,
    White
}

public enum BirdSize
{
    Default,
    Small,
    Medium,
    Large
}

public enum BirdHabitat
{
    Default,
    America,
    Australia,
    Africa,
    Europe
}

Then your model would be :

public class Bird
{
    public string Name { get; }
    public BirdSize Size { get; }
    public BirdColor Color { get; }
    public BirdHabitat Habitat { get; }

    /// .. rest 
}

This :

public static void BirdList()
{
    Console.Clear();
    foreach (var bird in birds)
        Console.WriteLine("Name: {0} | Size: {1} | Main Color: {2} | Habitat Location: {3}", bird.name, bird.size, bird.color, bird.habitat);
}

there is ToString() method, you should use it instead. override the Bird class ToString() to return the string you need. Like this :

public class Bird
{
    //code ...
    public override string ToString()
    {
        return $"Name: {Name} | Size: {Size} | Main Color: {Color} | Habitat Location: {Habitat}";
    }
}


public static void BirdList()
{
    Console.Clear();
    foreach (var bird in birds)
        Console.WriteLine(bird.ToString());
}

For the datatype point, see this line :

do
{
    Console.Write("Enter 1 or 2: ");
    input = Console.ReadLine();
    if (input == "1")
    {
        BirdList();
    }
    if (input == "2")
    {
        BirdQuestions();
    }
} while (input != "1" && input != "2");

The input is string integer, I know it's not an issue in your current code, however, with this code, it would opens some security risks. It might be not important here, but it would give you heads-up to avoid that in real applications.

your loop will process every user-input, and then validate that input, while you're expecting only integers (1 or 2). In other word, you're saying to the application, keep processing user inputs until the results either 1 or 2. Even if it's simple application. The concept itself to process every input until your condition is met it would be a huge risk in real applications. Avoid doing that, and always narrow down the inputs processing to the task requirements only. In this case, you need to restrict the input to integers only, then validate the given integer.

instead use this :

// restricting the input to only integers this way is a guaranteed that you will only process a valid integer. 
// also, it would be flixable for adding more options in the future. 
while(!int.TryParse(Console.ReadLine() , out int result))
{
    // code here ... 
}

no matter how many inputs the user enters, it'll only get inside the loop if it's integer, otherwise it'll skip and re-evaluate the next input. This change would make it difficult to the user to do any hacky inputs (either bugs that would stops the application, or some security exploits).

Your code is really good for a beginner, however, you made everything in one place, which throw away the prettiness of OOP. Your application has a model (this is a plus), and needs to have a class to manage the model collections (Business Layer). Then, Another class to work with the environment itself (Presentation Layer).

Let's say we have created BirdApplication and BirdConsole.

BirdApplication would contain the List<Bird> data, along with some methods that would be reused to get the data and parse them.

BirdConsole would contain the business logic for the Console Environment and would used BirdApplication internally.

If we implement them like that, we would endup doing this :

public static class Program 
{
    static void Main(string[] args)
    {
        BirdConsole.Start();
    }
}

This means, you moved all your application logic into independent classes, which would be easier to maintain, and also to convert to another environment. with minimum changes possible. For instance, if you wanted to move to Windows Form, you only need to create another class BirdForm for instance, and then convert BirdConsole to the approperate objects for Windows Forms.

These are some of the points that I see, I have re-written your code applying the points that I've mentioned above to give you a better picture on them. I hope it would be useful to you.

public class Bird
{
    public string Name { get; }
    public BirdSize Size { get; }
    public BirdColor Color { get; }
    public BirdHabitat Habitat { get; }

    public Bird(string name , BirdSize size , BirdColor color , BirdHabitat habitat)
    {
        Name = name;
        Size = size;
        Color = color;
        Habitat = habitat;
    }

    public override string ToString()
    {
        return $"Name: {Name} | Size: {Size} | Main Color: {Color} | Habitat Location: {Habitat}";
    }

}

public enum BirdColor
{
    Default,
    Brown,
    Grey,
    White
}

public enum BirdSize
{
    Default,
    Small,
    Medium,
    Large
}

public enum BirdHabitat
{
    Default,
    America,
    Australia,
    Africa,
    Europe
}

public class BirdApplication
{
    private readonly List<Bird> _birds;

    public BirdApplication()
    {
        _birds = InitiateList();
    }

    private List<Bird> InitiateList()
    {
        return new List<Bird>
        {
            new Bird("Bald Eagle", BirdSize.Large, BirdColor.White, BirdHabitat.America),
            new Bird("American Kestrel", BirdSize.Small, BirdColor.Brown, BirdHabitat.America),
            new Bird("Mississippi Kite", BirdSize.Medium, BirdColor.Grey, BirdHabitat.America),
            new Bird("Red Kite", BirdSize.Medium, BirdColor.Brown, BirdHabitat.Europe),
            new Bird("Secretary Bird", BirdSize.Large, BirdColor.Grey, BirdHabitat.Africa),
            new Bird("Shoebill Stork", BirdSize.Large, BirdColor.Grey, BirdHabitat.Africa),
            new Bird("Cockatoo", BirdSize.Medium, BirdColor.White, BirdHabitat.Australia)
        };
    }

    public List<Bird> GetBirds()
    {
        return _birds;
    }

    public static bool TryParseColor(string color , out BirdColor result)
    {
        return Enum.TryParse(color , true , out result);
    }

    public static bool TryParseSize(string size , out BirdSize result)
    {
        return Enum.TryParse(size , true , out result);
    }

    public static bool TryParseHabitat(string size , out BirdHabitat result)
    {
        return Enum.TryParse(size , true , out result);
    }

    public Bird GetBird(BirdColor color , BirdSize size , BirdHabitat habitat)
    {
        return _birds.Find(x => x.Color == color && x.Size == size && x.Habitat == habitat);
    }
}

public static class BirdConsole
{
    // always keep global variabls at the top of the class 
    public static BirdApplication _birdApp = new BirdApplication();

    public static void Start()
    {
        // Console.WriteLine will add the message into a new line, so no need to \n 
        // it would be more readable this way. 
        Console.WriteLine("Welcome to the Bird of prey identification helper.");
        Console.WriteLine("(1) List of all birds and their descriptions.");
        Console.WriteLine("(2) Identification help.");
        Console.WriteLine();

        // restricting the input to only integers this way is a gurantee that you will get a vaild integer. 
        // also, it would be flixable for adding more options in the future. 
        while(!int.TryParse(Console.ReadLine() , out int result))
        {
            switch(result)
            {
                case 1:
                    Console.Clear();
                    GetBirdsList();
                    break;
                case 2:
                    Console.Clear();
                    GetBirdQuestions();
                    break;
                default:
                    Console.WriteLine("Please choose between 1 or 2");
                    break;
            }
        }

    }

    private static void GetBirdsList()
    {
        var str = string.Join(Environment.NewLine , _birdApp.GetBirds());
        Console.WriteLine(str);
    }
    // this is not important, but It would be better if questions has its own implementation with a collection, so you loop over them, and help you to manage them easier.
    private static void GetQuestionAnswer(string question , bool condition)
    {
        do
        {
            Console.WriteLine(question);
            Console.Write("Enter: ");
        } while(!condition);
    }

    private static void GetBirdQuestions()
    {

        Console.Clear();
        Console.WriteLine("This process will help you through identifying a bird you have seen.");
        // questions are constants, keep that way to keep them unchanged. 
        const string question1 = "\nWhat was the birds main color? Enter brown, grey, or white.";

        const string question2 = "\nHow large or small was the bird? Enter small, medium, or large.";

        const string question3 = "\nWhere did you see the bird? Enter America, Australia, Europe, or Africa.";

        GetQuestionAnswer(question1 , BirdApplication.TryParseColor(Console.ReadLine() , out BirdColor color));

        GetQuestionAnswer(question2 , BirdApplication.TryParseSize(Console.ReadLine() , out BirdSize size));

        GetQuestionAnswer(question3 , BirdApplication.TryParseHabitat(Console.ReadLine() , out BirdHabitat habitat));

        var getBird = _birdApp.GetBird(color , size , habitat);

        if(getBird != null)
        {
            Console.WriteLine("\n" + getBird.Name);
        }
        else
        {
            Console.WriteLine("\nNo birds found.");
        }

    }

}

public static class Program 
{
    static void Main(string[] args)
    {
        BirdConsole.Start();
    }
}
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.