6
\$\begingroup\$

I'm in my second week/second section of learning C# through Treehouse/on my own. I recently switched from learning Javascript and am looking for a review of my code. I believe I've tested everything and have refactored to get it as concise as I can. The project is fully functional and does the following:

  • Adds a new contact
  • Updates existing contacts
  • Checks for duplicate contacts
    • if contact is a duplicate it asks the user if they want to update
  • returns(prints to the console) new/updated contact to view
  • Removes existing contacts
  • Formats contacts (upper/lower case) ("oH, bILly") //=> Oh, Billy
  • Allows a user to quit a process at anytime
  • Allows user to view the entire list of contacts

Some things I considered:

  • Using List
    • I wanted the practice with c# arrays since they differ from the javascript array literal
  • Adding a method that would allow a user to enter partial names and get a list of possible matches
  • Changing the switch(name) in Main() to an if/else to make it a bit more readable.

ADDITIONAL NOTE: I think it is fairly self-documenting, but I wrote it so, if you have any questions please ask. And, ContainsEntry() was written by someone else. I think that covers it..here's my code:

Contact class

namespace AddressBook {
    class Contact {
        public string Name { get; set; }
        public string Address { get; set; }

        public Contact(string name, string address) {
            Name = name;
            Address = address;
        }
    } 
}

AddressBook class

using System;

namespace AddressBook {
    class AddressBook {

        public readonly Contact[] contacts;

        public AddressBook() {
            contacts = new Contact[2]; ;
        }

        public bool AddEntry(string name, string address) {
            if (!ContainsEntry(name)) {
                name = FormatContact(name);
                address = FormatContact(address);
                Contact AddContact = new Contact(name, address);
                for (int i = 0; i < contacts.Length; i++) {
                    if (contacts[i] == null) {
                        contacts[i] = AddContact;
                        Console.WriteLine("Address Book updated. Name: {0} -- Address: {1} has been added!", name, address);
                        return true;
                    }
                }
                Console.WriteLine($"Cannot add ({name}) to Address Book since it is full!");
                return false;
            }
            else {
                Console.WriteLine($"({name}) already exists in Address Book!");
                UpdateContact(name);
            }
            return false;
        }

         public bool UpdateContact(string originalName) {
            Console.Write("Are you sure you would you like to update the Contact? -- Type 'Y' or 'N': ");
            string userResponse = Console.ReadLine().ToLower();
            if (userResponse == "y") {
                Console.Write($"Would you like to update {originalName}'s name or address? TYPE - 'Name' for name and 'Address' for address: ");
                string contactToUpdate = Console.ReadLine().ToLower();

                Console.Write($"Please enter changes to the {contactToUpdate} here: ");
                string updatedContact = Console.ReadLine().Trim();
                updatedContact = FormatContact(updatedContact);

                int index = GetEntryIndex(originalName);
                switch(contactToUpdate) {
                    case "name":
                        contacts[index].Name = updatedContact;
                        Console.WriteLine($"Contact {originalName} updated to {updatedContact}");
                        return true;
                    case "address":
                        contacts[index].Address = updatedContact;
                        Console.WriteLine($"Contact {originalName}'s {contactToUpdate} updated to {updatedContact}");
                        return true;
                }
            }
            return false;
        }

        private string FormatContact(string stringToFormat) {
            char[] arr = stringToFormat.ToCharArray();
            for (int i = 0; i < arr.Length; i++) {
                int num;
                if (i == 0 || arr[i - 1] == ' ' && !( int.TryParse(arr[i].ToString(), out num) ) ) { 
                    arr[i] = Convert.ToChar( arr[i].ToString().ToUpper() );
                }
                else {
                    arr[i] = Convert.ToChar(arr[i].ToString().ToLower());
                }
            }
            return String.Concat(arr);
        }

        private int GetEntryIndex(string name) {
            for (int i = 0; i < contacts.Length; i++) {
                if (contacts[i] != null && contacts[i].Name.ToLower() == name.ToLower())
                    return i;
            }
            return -1;
        }

        private bool ContainsEntry(string name) {
            return GetEntryIndex(name) != -1;
        }

        public void RemoveEntry(string name) {
            var index = GetEntryIndex(name);
            if (index != -1) {
                contacts[index] = null;
                Console.WriteLine("{0} removed from contacts", name);
            }
        }

         public string ViewContactsList() {
            string contactList = "";
            foreach (Contact contact in contacts) {
                if (contact == null) {
                    continue;
                }
                contactList += String.Format("Name: {0} -- Address: {1}" + Environment.NewLine, contact.Name, contact.Address);
            }
            return (contactList != String.Empty) ? contactList : "Your Address Book is empty.";
        }
    }
}

Program class

using System;

namespace AddressBook {
    class Program {
        static void Main(string[] args) {

            AddressBook addressBook = new AddressBook();

            PromptUser();

            void Menu() {
                Console.WriteLine("TYPE:");
                Console.WriteLine("'Add' to add a contact: ");
                Console.WriteLine("'View' to view the list of contacts: ");
                Console.WriteLine("'Remove' to select and remove a contact: ");
                Console.WriteLine("'Update' to select and update a contact: ");
                Console.WriteLine("'Quit' at anytime to exit: ");
            }

            void UpdateAddressBook(string userInput) {
                string name = "";
                string address = "";
                switch ( userInput.ToLower() ) {
                    case "add":
                        Console.Write("Enter a name: ");
                        name = Console.ReadLine().Trim();
                        switch(name) {
                            case "quit":
                                break;
                            default:
                                Console.Write("Enter an address: ");
                                address = Console.ReadLine().Trim();
                                switch (address) {
                                    case "quit":
                                        break;
                                    default:
                                        addressBook.AddEntry(name, address);
                                        break;
                                }
                                break;
                        }
                        break;
                    case "remove":
                        Console.Write("Enter a name to remove: ");
                        name = Console.ReadLine();
                        switch (name) {
                            case "quit":
                                break;
                            default:
                                addressBook.RemoveEntry(name);
                                break;
                        }
                        break;
                    case "view":
                        Console.WriteLine(addressBook.ViewContactsList());
                        break;
                    case "update":
                        Console.WriteLine("Please enter the name of the Contact you wish to update");
                        name = Console.ReadLine();
                        addressBook.UpdateContact(name);
                        break;
                }
            }

            void PromptUser() {
                Menu();
                string userInput = "";
                while (userInput != "quit") {
                    Console.WriteLine("What would you like to do?");
                    userInput = Console.ReadLine().Trim();
                    UpdateAddressBook(userInput);
                }
            }
        }
     }
 }
\$\endgroup\$
2
\$\begingroup\$

A few things that caught my eye:

Keep things private as much as possible, especially fields/properties. This restricts how someone else can use your class. They should only be able to access or manipulate any data, through methods you've created. This applies as well to the class Contact, keep it inside the AddressBook class and private. The user doesn't need to know what a Contact object is, only the information required to create/access one.

Since the menu(es) are specific to the AddressBook, have another class handle the collection and use AddressBook to handle the interface. Possible something like this:

class AddressBook
{
    class ContactCollection

    {
        class Contact
        {

        }
    }    
}

Since you want incremental search capabilities, I would suggest a Dictionary<string,List<Contact>> and institute an indexed collection of possible search terms

Here's a little more about how the structure can look:

public class AddressBook
{
    class Contact
    {
        public string Name { get; private set; }
        public string Address { get; private set; }

        public Contact(string name, string address)
        {
            Name = name;
            Address = address;
        }
        public void EditInfo(string address = "")
        {

        }
        public override string ToString()
        {
            return $"{Name}\n{Address}\n";
        }
    }
    class ContactCollection
    {
        Dictionary<string,Contact> contacts = new Dictionary<string, Contact>();
        Dictionary<string, List<Contact>> index = new Dictionary<string, List<Contact>>();
        public bool AddEntry(string name, string address)
        {

        }
        public bool EditEntry(string name)
        {

        }
        public void ShowEntry(string name)
        {

        }
        public void ShowAll()
        {

        }
    }
    ContactCollection cc = new ContactCollection();

    void ShowMenu()
    {

    }
}

You'll notice how I've used a combination public and private in the Contact class. And in the ContactCollection the collections are private but the methods are public.

\$\endgroup\$
  • \$\begingroup\$ I have gone from private to public with the Contact.cs properties a couple of times. I had trouble figuring out how to work with them without them being public. I had no idea you could set up multiple classes in one file like this and have it be considered good practice. It is good to know but adds another layer of needing to understand how that works. It would cover the issue with access to the Contact.cs properties, maybe. I assume this because Contact, in your example, is nested within the AddressBook class. But, what about scope? \$\endgroup\$ – Nibble15 May 3 '17 at 22:14
  • \$\begingroup\$ If items declared are part of the Contact class wouldn't they need to be public to be accessed from the outer scope that AddressBook() has? Or, does scope not apply in the same manner in this case? eg: the 'i' variable declared in a for loop's statement is within a method, but not accessible outside of the loop, therefore only in the scope of the loop. Either way, all of this, to be clear, would bring my files down to two - Program.cs with Main() and then my AddressBook and Contact classes rolled into one with the collection added to it, instead of the three that I have now. \$\endgroup\$ – Nibble15 May 3 '17 at 22:15
  • \$\begingroup\$ Continuing with my assumptions, by collection you're referring to the Dictionary<s, Contact> that you mentioned. So, public AddressBook(){} and the AddressBook field that I have now would then be a part of the ContactCollection class and possibly GetEntryIndex() ContainsEntry() and RemoveEntry(), as well, but using Dictionary(which I need to look up what that is). The methods I just mentioned don't really have anything to do specifically with the interface, that is why I am thinking this. \$\endgroup\$ – Nibble15 May 3 '17 at 22:15
  • \$\begingroup\$ Sorry for the search typo that should have been Dictionary<string,List<Contact>>. I've added more about the structure to perhaps make it a little more clear. \$\endgroup\$ – tinstaafl May 3 '17 at 22:51
  • 1
    \$\begingroup\$ Three nested classes... isn't it too much? ;-] \$\endgroup\$ – t3chb0t May 4 '17 at 4:26
1
\$\begingroup\$

You are writing a lot of collection stuff from scratch when a lot of that structure is built into custom collections

You aqre doing a lot of searches that are one line in LINQ

I suggest that Contact implements iComparable so all comparison is in one spot.

And implement Indexer on the collection

Use a List so can add and remove

Sample for for the collection part

public class TestContacts
{
    public TestContacts()
    {
        Program.ContactCollection<Program.Contact> contactCollection = new Program.ContactCollection<Program.Contact>();
        contactCollection.AddContact("zeke", "blvd");
        contactCollection.AddContact("john", "street");                
        contactCollection.AddContact("john", "street");
        foreach (Program.Contact c in contactCollection.ContactsSorted)
        {
            Debug.WriteLine(c.Name);
        }
        Debug.WriteLine(contactCollection[1].Name);
        Debug.WriteLine(contactCollection["zeke"].Address);
    }
}
public class Contact : IComparable
{
    public int CompareTo(object obj)
    {
        if (obj == null) return 1;

        Contact other = obj as Contact;
        if (other != null)
            return this.Name.CompareTo(other.Name);
        else
            throw new ArgumentException("Object is not a Contact");
    }
    public override bool Equals(Object obj)
    {
        Contact other = obj as Contact;
        if (other == null)
            return false;
        else
            return this.Name.Equals(other.Name);
    }
    public override int GetHashCode() { return base.GetHashCode(); }
    public string Name { get; private set; }
    public string Address { get; private set; }            
    public Contact(string name, string address)
    {
        Name = name;
        Address = address;
    }
}
public class ContactCollection<Contact> : IReadOnlyCollection<Program.Contact> 
{
    private List<Program.Contact> contacts = new List<Program.Contact>();
    public int Count { get { return contacts.Count; } }
    public bool Contains(Program.Contact item) { return contacts.Contains(item); }
    public IEnumerator<Program.Contact> GetEnumerator() { return contacts.AsReadOnly().GetEnumerator(); }
    IEnumerator IEnumerable.GetEnumerator() { return contacts.AsReadOnly().GetEnumerator(); }
    public Program.Contact this[int i] { get { return contacts[i]; } }
    public Program.Contact this[string name] { get { return contacts.FirstOrDefault(x => x.Name.Equals(name)); } }
    public IEnumerable<Program.Contact> Contacts  { get { return contacts.AsReadOnly(); } }
    public IEnumerable<Program.Contact> ContactsSorted { get { return contacts.AsReadOnly().OrderBy(x => x.Name); } }
    public bool AddContact(string name, string address)
    {
        Program.Contact contact = new Program.Contact(name, address);
        if (this.Contains(contact))
            return false;
        contacts.Add(contact);
        return true;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thank you for the information. This too, I believe, will be useful in the future. I do, however, think this is well beyond my scope at the moment. Even more so than the previous answer. I will be sure to refer back to this later as I learn more. Hopefully, when the time comes you won't mind I ask you some questions about it. \$\endgroup\$ – Nibble15 May 5 '17 at 17:42

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.