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();
}
}
