Simple person manager console-app

Question

The code below is for a coding challenge for a job. I have everything working as they requested, I've reviewed it myself and believe everything is good. I'm just looking to see if anyone see's anything that I may have done in a way that may be considered bad practice, or if there is any obvious improvements that could be made to clean up the code or increase performance. Any feedback is welcome.

Here are the instructions given to me by the company:

1. Present a menu to the user with 3 options [View Persons, Add Person, Exit]
2. If the user selects 'View Persons', the application should show a list of persons in the application.
3. If the user selects 'Add Person', they should be instructed to enter a name. The program should assign a unique id to the new person.
4. If a person selects 'Exit', the application should close. Otherwise, the application should stay running and/or indicate that the choice was invalid.
5. Optional: Create a 4th feature where you can search by name and the program indicates whether or not the name is in the system.
6. Use an in memory list to store person objects.
7. Use multiple classes and object oriented design to write your program (don't put all of your logic in the Program class).
8. What we look for:
   • Does the program meet the requirements/instructions and is it stable?
   • Is the code clean & appropriately commented?
   • Is there an understandable object oriented design?

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

namespace CSECodeSampleConsole
{


public class Program
{
    public static void Main(string[] args)
    {
        Menu menu = new Menu();
        menu.MainMenu();
    }
}

internal class Person
{
    public int Id { get; set; }
    public string Name { get; set; }
    public static int globalId = 1;

    public Person(string name)
    {
        this.Name = name;
        this.Id = globalId;
        globalId++;
    }

}

internal class Menu
{
    List<Person> people = new List<Person>();
    public void MainMenu() //Display and Navigate Menu.
    {
        Console.WriteLine("Please make a selection by entering the corresponding number:");
        Console.WriteLine("1.View Persons");
        Console.WriteLine("2.Add Person");
        Console.WriteLine("3. Search For Person");
        Console.WriteLine("4.Exit");
        var input = Console.ReadLine();

        switch (Convert.ToInt32(input))
        {
            case 1:
                DisplayNames();
                break;
            case 2:
                AddPerson();
                break;
            case 3:
                SearchList();
                break;
            case 4:
                Environment.Exit(0);
                break;
            default:
                Console.WriteLine("Invalid Input\n\n");
                MainMenu();
                break;
        }
        return;
    }

    public void DisplayNames() //Display all names and their Id's in list.
    {
        if(people.Count == 0)
        {
            Console.WriteLine("No People To Display Yet.\n\n");
            MainMenu();
        }
        else
        {
            Console.WriteLine("List of Current People:\n");
            foreach (Person person in people)
            {
                Console.WriteLine(person.Id + " - " + person.Name);
            }
            Console.WriteLine("\n\n");
            MainMenu();
        }
        return;
    }

    public void AddPerson() //Add a new person to the list.
    {
        Console.WriteLine("Please Enter The Person's Name: ");
        var result = Console.ReadLine();
        Person newPerson = new Person(result);
        people.Add(newPerson);
        Console.WriteLine(newPerson.Name + " added successfully.\n\n");
        MainMenu();
        return;
    }

    public void SearchList() //Find and display search results.
    {
        Console.WriteLine("Please Enter A Name To Search For: ");
        var result = Console.ReadLine();
        var searchResults = people.Where(n => n.Name.Contains(result, StringComparison.OrdinalIgnoreCase)).ToList();

        if (searchResults.Count == 0)
        {
            Console.WriteLine("No Results Were Found");
        }
        else
        {
            Console.WriteLine("The Following Results Were Found: \n");
            foreach (Person person in searchResults)
            {
                Console.WriteLine(person.Id + " - " + person.Name);
            }
        }
        Console.WriteLine("\n\n");
        MainMenu();
    }
}

public static class MyExtensionMethods 
{
    public static bool Contains(this string source, string toCheck, StringComparison comp)
    {   //Created this to take away the case sensitivity of the Contains method.
        return source != null && toCheck != null && source.IndexOf(toCheck, comp) >= 0;
    }
}
}

Answer 1

Feedack to the code:

this.Id = globalId;
globalId++;

Could be simplified as

this.Id = globalId++;

---

globalId should not be public

---

switch (Convert.ToInt32(input)) { ... }

That is not a robust way to parse the input. If the user enters a letter, your program crashs!

---

• MainMenu() can be moved to the end of the MainMenu() method (as recursive call).
• return is not needed at the end of a method.
• The Contains-Extension method is not needed because neither the name, nor the search input can be null.
• Use Environment.NewLine or Console.WriteLine instead of "\n".

---

Object oriented design

It is good, that there is a person class for storing information about the person.

However, because the interviewer wishs an "understandable object oriented design" I would also try to model the different menu cases as objects.

e.g.

public abstract class MenuEntry
{
    public MenuEntry(int id, string description)
    {
        this.Id = id;
        this.Description = description;
    }

    public int Id { get; }
    public string Description { get; }

    public abstract void Execut();
}

That allows to define each menu entry and its logic in a separate class and you get rid of the switch statement.

Further more it is simpler to extend the program with new menu items without touching existign logic ;).

---

Example implementation for single class entries:

    internal class Person
    {
        public int Id { get; }
        public string Name { get; }
        private static int globalId = 1;

        public Person(string name)
        {
            this.Name = name;
            this.Id = globalId++;
        }

    }

    internal abstract class MenuEntry
    {
        public MenuEntry(int id, string description)
        {
            this.Id = id;
            this.Description = description;
        }

        public int Id { get; }
        public string Description { get; }

        public abstract void Execut();
    }

    internal class DisplayNames : MenuEntry
    {
        private readonly List<Person> persons;

        internal DisplayNames(List<Person> persons) : base(1, "View Persons")
        {
            this.persons = persons;
        }

        public override void Execut()
        {
            if (persons.Count == 0)
            {
                Console.WriteLine("No People To Display Yet.");
            }
            else
            {
                Console.WriteLine("List of Current People:");
                persons.ForEach(p => Console.WriteLine(p.Id + " - " + p.Name));
            }
        }
    }

    internal class AddPerson : MenuEntry
    {
        private readonly List<Person> persons;

        internal AddPerson(List<Person> persons) : base(2, "Add Person")
        {
            this.persons = persons;
        }

        public override void Execut()
        {
            Console.WriteLine("Please Enter The Person's Name: ");
            var result = Console.ReadLine();
            Person newPerson = new Person(result);
            persons.Add(newPerson);
            Console.WriteLine(newPerson.Name + " added successfully.");
        }
    }

    internal class Exit : MenuEntry
    {
        internal Exit() : base(9, "Exit")
        {
        }

        public override void Execut()
        {
            Environment.Exit(0);
        }
    }

    internal class Menu
    {
        private readonly List<Person> persons = new List<Person>();
        private readonly List<MenuEntry> entries = new List<MenuEntry>();

        public Menu()
        {
            this.entries.Add(new DisplayNames(this.persons));
            this.entries.Add(new AddPerson(this.persons));
            // ... other entries
            this.entries.Add(new Exit());
        }

        public void Show()
        {
            Console.WriteLine("Please make a selection by entering the corresponding number:");
            this.entries.ForEach(p => Console.WriteLine($"{p.Id}. {p.Description}"));

            var input = Console.ReadLine();

            int entryId = -1;
            MenuEntry entry = null;
            if (int.TryParse(input, out entryId))
            {
                entry = this.entries.FirstOrDefault(e => e.Id == entryId);
                entry?.Execut();
            }

            if (entry == null)
            {
                Console.WriteLine("Invalid Input.");
            }

            Console.WriteLine();
            Console.WriteLine();

            this.Show();
        }
    }

Answer 2

I am not a C# programmer, but here is some general feedback:

You call menu.MainMenu() but there's no verb! Objects are nouns, methods should be verbs. Maybe name the method DisplayMenu or DispatchUserAction or something that indicates activity is happening?

Your MainMenu method doesn't appear to handle invalid input. What happens if the user hits return? EOF? "Abcde?"

The failure mode that I see just calls itself recursively. So can I crash your program by exhausting the stack with a series of 9's as input? Put in a while true loop, or something similar. Maybe loop and call an inner method, for small clear code.

The Person constructor also updates the id counter. Change that so the id is managed separately. Maybe take an optional parameter and force the id counter to be above that value if provided?

You have some awkward code to implement your search. Why is your comparison logic in the Menu class, or some 3rd party class? I think asking if a person's name matches a string is something that the Person class should handle:

foreach (Person guy in people)  
     if (guy.isThisYou(target))
          etc.

I just noticed you handle all your returns to the main menu by recursion. Don't do that. Return and write a loop in the main menu.

You have a lot of patterns of interacting with the console for input and output. Make those into dedicated helper functions/methods. Examples: printing two newlines to separate "paragraphs" could be NewParagraph(); and writing a line then reading a line could be GetInput (String prompt)

If possible, separate input and output streams into variables, and read and write against those stream variables. This will make it possible to code some test cases: construct the menu with string streams and compare the output against an expected value for the input.