1
\$\begingroup\$

I have created a simple C# console app to display an Address Book using Dictionary. I would like to present it in a interview but I am not sure if the code is "interview" worthy. Given below is the Main() method that calls the methods as required.

public static void Main(string[] args) {
          while (true)
            {  
                Console.WriteLine(
                    "+++++++++++************+++++++++++++++****************" +
                    "\n\n This is an application to view list of contacts \n" +
                    "Choose from the options below to go ahead: \n"+
                    "1. Create a new contact \n" +
                    "2. View list of contacts \n" +
                    "3. Update a contact \n" +
                    "4. Delete a contact \n" +
                    "5. Quit the current application \n");

                string UserInput = Console.ReadLine();
                int ChosenValue;
                bool ChosenValueValid = int.TryParse(UserInput, out ChosenValue);                               

                if (ChosenValueValid) {
                    Console.WriteLine("You have chosen option: {0}", UserInput);
                    switch (ChosenValue)
                    {
                        case 1:
                            ConsoleAddressBook.CreateContact();
                            break;
                        case 2:
                            ConsoleAddressBook.GetContactList();
                            break;
                        case 3:
                            ConsoleAddressBook.UpdateContact();
                            break;
                        case 4:
                            ConsoleAddressBook.DeleteContact();
                            break;
                        case 5:
                            Console.WriteLine("Application will be exited");
                            return;
                        default:
                            Console.WriteLine("Enter a valid number between 1 and 6");
                            break;
                    }
                }               
                else
                {
                    Console.WriteLine("Please enter a number and not a string");
                }
            }
        }

Below is the code that describes the various methods that are being called from the initial screen:

public class ConsoleAddressBook
    {
        class Contact
        {
            string Name { get; set; }
            int PhoneNumber { get; set; }

        }
        static Dictionary<int, string> ContactsList = new Dictionary<int, string>();
        public static void GetContactList() {
            if (ContactsList.Count > 0)
            {
                foreach (var contact in ContactsList)
                    Console.WriteLine("\n Name: " + contact.Value +                                          
                                        " Number: " + contact.Key + " \n ");
            }
            else
            {
                Console.WriteLine("\n No contacts found in the ContactList. Add a new contact to proceed further: \n");
            }
        }
        public static void CreateContact() {
            while (true) { 
                bool Valid= false;

                Console.WriteLine("Enter the Name of the contact");
                string Name = Console.ReadLine();
                Valid = IsValueValid(Name, 0);            

                Console.WriteLine("Enter the contact PhoneNumber");
                int PhoneNumber;
                bool PhoneNumberValid = int.TryParse(Console.ReadLine(), out PhoneNumber);
                Valid = IsValueValid("", PhoneNumber);            

                if (!Valid)
                {
                    Console.WriteLine("Enter valid string for Name Or integer for PhoneNumber and try again");
                    return;
                }
                else if(ContactsList.ContainsKey(PhoneNumber))
                {
                    Console.WriteLine("Phone number already found in the list. \n" + 
                        "Choose option 3 to update the contact name if required");
                    return;
                }
                else
                {
                    ContactsList.Add(PhoneNumber, Name);
                    Console.WriteLine("Contact added. Choose option 2 to view list of contacts");
                    return;
                }
            }
        }
        public static void UpdateContact() {

            Console.WriteLine("Enter the phone number of the contact you wish to update");

            int PhoneNumber;
            bool PhoneNumberValid = int.TryParse(Console.ReadLine(), out PhoneNumber);
            bool Valid = IsValueValid("", PhoneNumber);

            if (!Valid)
            {
                Console.WriteLine("Enter a valid phone number to update");
            }
            else if (!ContactsList.ContainsKey(PhoneNumber))
            {
                Console.WriteLine("Phone number not found in the list. \n" +
                    "Choose option 1 to view the list of contacts");
                return;
            }
            else
            {
                Console.WriteLine("Enter the new name for the contact");
                string newName = Console.ReadLine();

                ContactsList[PhoneNumber] = newName;
                Console.WriteLine("Contact {0} updated", PhoneNumber);
            }
        }
        public static void DeleteContact()
        {
            Console.WriteLine("Enter the phone number of the contact you wish to delete");

            int PhoneNumber;
            bool PhoneNumberValid = int.TryParse(Console.ReadLine(), out PhoneNumber);
            bool Valid = IsValueValid("", PhoneNumber);

            if (!Valid)
            {
                Console.WriteLine("Enter a valid phone number to delete");
            }
            else if (!ContactsList.ContainsKey(PhoneNumber))
            {
                Console.WriteLine("Phone number not found in the list. \n" +
                    "Choose option 1 to view the list of contacts");
                return;
            }
            else
            {
                ContactsList.Remove(PhoneNumber);
                Console.WriteLine("Contact {0} deleted", PhoneNumber);
            }            
        }
        public static bool IsValueValid(string name = "", int phoneNumber = 0)
        {
            if (!string.IsNullOrWhiteSpace(name))
            {
                return Regex.IsMatch(name, @"^[a-zA-Z]+$");
            }

            if (phoneNumber > 0)
            {
                return Regex.IsMatch(Convert.ToString(phoneNumber), @"^[0-9]+$");
            }
            return false;
        }
    }

I am not sure if this is too wordy or too lengthy and if I can make this more efficient. Looking forward to your inputs.

\$\endgroup\$
  • \$\begingroup\$ I don't like the many static methods. I get it - you probably designed this thinking you have ONE contact list. But you could be more flexible by allowing different lists, perhaps business and personal. There could be a static method for GetDefaultContactList but it everything else works off an instance. \$\endgroup\$ – Rick Davin Mar 25 at 12:57
  • \$\begingroup\$ In addition to @RickDavin's comment: non static methods easier to test. \$\endgroup\$ – MaLiN2223 Mar 25 at 12:59
5
\$\begingroup\$

Don't use \n, use Environment.NewLine.


Please follow Microsoft's guidelines/conventions, e.g. with regard to capitalization,...


Be careful when naming things. The "Console" part of ConsoleAddressBook is IMHO pointless. ContactsList isn't a List but a Dictionary, and thus the name doesn't correspond with the type -- why not simply name it Contacts or ContactsByPhoneNumber?


A phone number isn't an int. See also this list of Falsehoods Programmers Believe About Phone Numbers.


Separate your UI from your back-end code. ConsoleAddressBook mixes the two and thus becomes hard to maintain. What would have impressed me is a somewhat mature application where there is such a separation, whereas I'd regard your code as a typical "whipped this up in an hour" stopgap solution.

\$\endgroup\$
  • \$\begingroup\$ I didn't notice that the variables do not have a good names, nice catch. To add to your comment about a new line: documentation states that it is A string containing "\r\n" for non-Unix platforms, or a string containing "\n" for Unix platforms. So the reason for this suggestion is portability. \$\endgroup\$ – MaLiN2223 Mar 25 at 13:19
2
\$\begingroup\$

Note: below is my opinion and should not be taken as a source of truth.

  1. Main should be clear of any logic except starting the program or parsing the arguments (args not user input). For me this comes from SRP.

  2. Try to abstract reading/writing to some injectable read and write classes. This gives you both possibility of further extension (away from console) and testability.

  3. As mentioned in comments, drop static methods when they are not absolutely needed advantage of doing that is the fact that your code becomes more testable (without moles).

  4. 'Menu' from which user selects the data should be generated dynamically for possible extensions or getting the options from config/db.

  5. Phone number should be a string. It's validation should be extracted to some helper class/function because you are repeating yourself. For validation regex see this link

  6. There are many duplicates in the code, maybe you could try to abstract some parts to methods.

  7. IsValueValid method is not nice, it does not follow SRP. Try to split it into two.

Bonus - things that may not be required but would show an effort:

  1. Dictionary storage (some kind of database abstraction would be nice)
  2. Logging - I haven't seen any production app without logging
  3. Tests - it's always good to show some test evidence.
\$\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.