c# Method not working as expected Address book project

Question

I'm new to c# and working on my first project - a console app. I am having some trouble understanding why my code will not return false when an entry in the address book already exists. The following methods are all part of the AddressBook class CheckEntry(), AddEntry(), RemoveEntry().

Ok, so the boolean method CheckEntry() is used by two other methods - AddEntry() and RemoveEntry() - both are looking to verify if the user entry exists before performing their respective duties. In AddEntry() it is supposed to see if the contact already exists before adding another contact and shouldn't create the contact if it does (but it's adding duplicates). RemoveEntry() is supposed to check if it exists and uses the updated value of the stored variable in CheckEntry() to remove the current contact (but does nothing). I know I'm probably either missing something simple or have way over thought the whole process. My assumption is that checkEntry() is not working correctly since both of the functions tied to it are faulting. Anyone have any ideas?? Let me know if I need to explain anything further.

NOTE: I know that using a list would be more efficient/easier to work with. It was my goal to use an array for learning purposes. Switching from learning Javascript to C# has been a bit of a challenge and I want to make sure I'm learning each thing before moving onto the next.

Here is all of my code. Each class is separated by "//--------" thank you in advance for your help.

namespace AddressBook {
    class Contact {
        public string Name;
        public string Address;

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

//------------------------------------------------------------------------

using System;

namespace AddressBook {
    class AddressBook {

        public readonly Contact[] contacts;

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

        public void AddEntry(string name, string address) {
            Contact AddContact = new Contact(name, address);
            if (CheckEntry(name)) {
                for (int i = 0; i < contacts.Length; i++) {
                    if (contacts[i] == null) {
                        contacts[i] = AddContact;
                        Console.WriteLine("Address Book updated. {0} has been added!", name);
                        break;
                    }
                }
            }
        }

        private string existingContact = "";

        private bool CheckEntry(string name) {
            foreach(Contact contact in contacts) {
                if (contact == null) {
                    break;
                }
                else if (contact != null && contact.ToString() != name) {
                    continue;
                }
                else if (contact.ToString() == name) {
                    existingContact = contact.ToString();
                    return false;
                }

            }
             return true;
        }

        public void RemoveEntry(string name) {
            if( !(CheckEntry(name)) ) {
                existingContact = null;
                Console.WriteLine("{0} removed from contacts", name);
            }
        }

         public string View() {
            string contactList = "";
            foreach(Contact contact in contacts) {
                if(contact == null) {
                    break;
                }
                contactList += String.Format("Name: {0} -- Address: {1}" + Environment.NewLine, contact.Name, contact.Address); 
            }
            return contactList;
        }
    }
}

//------------------------------------------------------------------------

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("'Remove' to select and remove a contact: ");
                Console.WriteLine("'Quit' to exit: ");
            }

            void UpdateAddressBook(string userInput) {
                string name = "";
                string address = "";
                switch ( userInput.ToLower() ) {
                    case "add":
                        Console.Write("Enter a name: ");
                        name = Console.ReadLine();
                        Console.Write("Enter an address: ");
                        address = Console.ReadLine();
                        addressBook.AddEntry(name, address);
                        break;
                    case "remove":
                        Console.Write("Enter a name to remove: ");
                        name = Console.ReadLine();
                        addressBook.RemoveEntry(name); 
                        break;
                    case "view":
                        Console.WriteLine(addressBook.View());
                        break;
                }
            }

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

Here is what I came up with - tested changes

Now I can't add duplicate names and I can remove entries.

public void AddEntry(string name, string address) {
        Contact AddContact = new Contact(); //changed
        AddContact.Name = name;  //changed
        AddContact.Address = address; //changed
        if (CheckEntry(name)) {
            for(int i = 0; i < contacts.Length; i++) {
                if (contacts[i] == null) {
                    contacts[i] = AddContact;
                    Console.WriteLine("Address Book updated. {0} has been added!", name);
                    break;
                }
            }
        }
    }
    //changed - removed variable and all instances of...
    private bool CheckEntry(string name) {
        foreach(Contact contact in contacts) {
            if (contact == null) {
                break;
            }
            else if (contact != null && contact.Name != name) {
                continue;
            }
            else if (contact.Name == name) {
                return false;
            }
        }
        return true;
    }

    //changed - instead of passing checkentry() as a check I just took care of it here
    public void RemoveEntry(string name) {
        for(int i = 0; i < contacts.Length; i++) {
            if(contacts[i].Name == name) {
                contacts[i] = null;
                break;
            }
        }
        Console.WriteLine("{0} removed from contacts", name);
    }

Answer 1

First of, I am assuming your strategy is to find the first empty slot in the array and insert a new Contact in it for AddEntry. To remove an entry, you want to simply mark that array location as empty. As you realize, this means that the array does not grow dynamically with the request i.e. you can have an ArrayFull situation that you need to handle. Also, you are doing linear search a.k.a. scan of the array - I assume you don't want to focus on that aspect in this sample.

Below are my comments for your existing code:

1. Shouldn't AddEntry update the Address if Address is different even though the Name matches?
2. Also returning a bool to indicate if the address was added/updated (true) versus nothing was done (false)
3. You should also handle the 'ArrayFull` condition
4. I do not understand why you need the variable existingContact
5. Don't use functions named CheckXXX if you plan to return bool. ContainxXXX is better understood method name to use with a bool return value.
6. You should not break from your foreach loop in CheckEntry. What if 2 entries were added, then the first one removed and then request to add the second one comes again? You would just break out since first entry is null
7. You seem to have 3 different ifs to check in your foreach where only one suffices.

Below is my relevant code with comments. I tried to keep them similar to what you have. You can use LINQ in multiple places instead of looping.

class Contact {
    public string Name { get; private set; }        // use a property with a private setter, instead of a public member
    public string Address { get; private set; }     // use a property with a private setter, instead of a public member

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

}

//------------------------------------------------------------------------


class AddressBook {

    public readonly Contact[] contacts;

    public AddressBook() {
        contacts = new Contact[2]; // I am assuming you kept the size 2 for testing
    }

    public bool AddEntry(string name, string address) {
        if (!ContainsEntry(name)) {
            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. {0} has been added!", name);
                    return true;
                }
            }
            Console.WriteLine($"Cannot add name ({name}) to Address Book since it is full!");
            // TODO: Throw some exception or specific return values to indicate the same to the caller
        } else {
            Console.WriteLine($"Name ({name}) already exists in Address Book!");
            // TODO: Update the address?
        }

        return false;
    }

    private int GetEntryIndex(string name) {
        for (int i = 0; i < contacts.Length; i++) {
            if (contacts[i] != null && contacts[i].Name == name)
                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 View() {
        string contactList = "";
        foreach (Contact contact in contacts) {
            if (contact == null) {
                continue;   // Don't break, but simply continue to look further
            }
            contactList += String.Format("Name: {0} -- Address: {1}" + Environment.NewLine, contact.Name, contact.Address);
        }
        return contactList;
    }
}

EDIT: Answering some of the questions the OP has

Q: What should I do with the return value of AddEntry
A: Right now, you are writing code for practice, but once you start writing industry standard code, you will soon realize that you wouldn't know who call's your function. Never assume (unless the method is private) that only you will call this function. Currently, you don't need to do anything with this return value, but time might come when you want to know if your call did indeed modify some values or just returned without changes. It is for that time. It is a pretty standard practice. Some folks even return an enum {NoChange, Added, Updated, Deleted} instead of a bool

Q: How to reduce the CheckEntry function.

A: Below I have written your function with a slightly different formatting

private bool CheckEntry(string name) {
    foreach (Contact contact in contacts) {
        if (contact == null) { // First if
            continue;
        } else {
            if (contact != null && contact.Name != name) { // Second if
                continue;
            } else {
                if (contact.Name == name) { // Third if
                    return false;
                }
            }
        }
    }
    return true;
}

For the first if statement, the else part is redudant. The second if statement will hit only when contact is not null due to the continue statement, cancelling the need for the else keyword.

Since contact is not null when we hit the second if statement, the check contact != null is pretty redundant in the second if. You can reduce the if statement's as below

if (contact.Name != name) { // Second if
    continue;
} else {
    if (contact.Name == name) { // Third if
        return false;
    }
}

Similarly you will notice that the the third if will hit only when contact.Name is the same as name (otherwise it would have continued). So there is no need to check again. This will reduce our checks as below

if (contact == null)
    continue;

if (contact.Name != name)
    continue;
else
    return false;

This can be further reduced by combining the conditions in the two if statements as below

if (contact == null || contact.Name != name)
    continue;
else
    return false;

Which is the same as (negating the condition)

if (contact != null && contact.Name == name)
    return false;

So your function will look like

private bool CheckEntry(string name) {
    foreach (Contact contact in contacts)
        if (contact != null && contact.Name == name)
            return false;
    return true;
}

Hope you follow the deductions here

Q: I may have misunderstood from the course but when you write the properties in a class with getters and setters, which I did change with my updated version, I was under the impression that it wasn't necessary, even redundant, to create a Constructor - that the class (not sure if this is correct terminology) has a default constructor built in for cases where you would want to add the property.
A: Correct. Just different ways of doing it.

Q: I like what you did with GetEntryIndex() and ContainsEntry() - I'm curious, is GetEntryIndex() not the same as writing your own Array.IndexOf(), though? Someone, either me or a method, has to scan the array. Both versions are linear so either way, this is O(n), correct? (just getting into some theory so please correct me if I'm wrong). So, just for my understanding, this is the same as: return Array.IndexOf(contacts, name); this returns -1 if it doesn't exist or whatever the index is(I'm assuming)

A: It is not the same, but similar in essence. IndexOf has a set of rules to figure out if given two objects are equal or not. You haven't defined these rules on your custom object, hence it will always return -1. There are simple LINQ statements to do these, but I would let you explore that on your own.

Answer 2

In CheckEntry you are testing an object of type Contact with your param of type string. This cannot work as you are testing 2 different types. It could work the way you wrote it if you override the ToString method in your Contact class (so that it gives the attribute contact.name).

You can modify your code like this (add using System.Linq):

private bool CheckEntry(string name)
{
    return contacts.Any(a => a.Name == name);            
}

Answer 3

First, thank you for taking the time to make such a thorough response. Second, I should probably note that I am teaching myself/taking a course on Treehouse and I'm on my second week/second section of the course - just to give you an idea of where I'm coming from. That being said, I want to walk through what you have given me and where I was heading with this project so I can learn.

1. I agree the user should be able to update and it was a feature that I had considered but had not quite gotten there.
2. if returning a bool when added/updated would you then remove the update string and place it with the caller in main()? This way it only prints that it's been updated if return true else print that the contact was unchanged. It kind of feels like it might be advantageous to make a class that is suitable just checking values - which would also make my code more reusable?? If I was going to use it in Main() maybe something like this would work..

if(!addEntry()) {Console.WriteLine("Contact {0} was not updated", name);} //do something else{Console.WriteLine("Address Book updated. {0} has been added!", name);}

Furthermore, if the user is just updating the contact it could print that it's been updated. So the Console output could be a ternary operation - if new name print contact added otherwise contact updated??

3. I agree and had run into the situation where nothing was done because the array was full and that was something I had planned on working on.
4. I absolutely agree about the existing contact variable. I was so stuck when it came to what to do that was the best I could come up with. I should have walked away for a bit and thought about it more. I don't know if you saw my updated portion, but I was able to eliminate it.
5. Contains for a bool method seems logical, I will be sure to follow this rule.
6. I had not considered this in such terms before you said that. It makes perfect sense though. If the first entry is null and the second contains the name that the user is trying to enter then you would end up with a duplicate since I was breaking from the loop at the first instance of null instead of making sure the name didn't exist in the entire array, first.
7. I'm not sure, without your methods, how I would have been able to eliminate my three conditionals. I see what you did to make the one work, but am I possibly missing something? Or, was that more of a reference saying, ok here's a better way - use these methods to check and then eliminate the if/else chain, thus making your code more concise.

for your code:

1. I may have misunderstood from the course but when you write the properties in a class with getters and setters, which I did change with my updated version, I was under the impression that it wasn't necessary, even redundant, to create a Constructor - that the class (not sure if this is correct terminology) has a default constructor built in for cases where you would want to add the property.

2. Correct, I did set the size to 2 for testing purposes.

3. I like what you did with GetEntryIndex() and ContainsEntry() - I'm curious, is GetEntryIndex() not the same as writing your own Array.IndexOf(), though? Someone, either me or a method, has to scan the array. Both versions are linear so either way, this is O(n), correct? (just getting into some theory so please correct me if I'm wrong). So, just for my understanding, this is the same as:

return Array.IndexOf(contacts, name);

this returns -1 if it doesn't exist or whatever the index is(I'm assuming)

4. That's a good idea with the continue in View(). That will make sure to print out any contact with an index higher than an index with a null value. By some magic, this was working in this way with the break(it would print out index 1 even if index 0 was empty), but I do realize why it shouldn't and how using continue is better.