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);
}
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:
AddEntry update the Address if Address is different even though the Name matches? bool to indicate if the address was added/updated (true) versus nothing was done (false)existingContactCheckXXX if you plan to return bool. ContainxXXX is better understood method name to use with a bool return value. 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 nullifs 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.