I have been taking some C# online courses the last couple of weeks in order to learn programming. Since I dont have any friends that are programming I found it hard to get feedback / reviews on the code I'm writing. Sure the code is running, but how about the code quality?

About the project

I have created a simple Address Book (console application) based on four classes:

• Main


• AdressBook


• Person


• WriteAndReadToFile

The application has the following abilities:

• Store friends


• Update friend information


• Remove user


• Show all users


• When adding/update/remove user it should be stored on a txt-file (which is loaded when the program start).

For simplicity I only added only name and adress in the application, there is also very bad data-input validation (focusing in learing OOP). Regarding the update function I am aware that it will update everything wich have a match on firstName.

Questions

• Am I using OOP in a correct way?


• I found it hard to keep the text-file in a correct "state" after updates & deletes and begun to flush the file after and rewrite the file instead. Is that bad practice?

Code

Main

class Program

 static void Main(string args)
 
 bool ProgramIsRunning = true;
 AdressBook ab = StartProgram();

 Console.WriteLine("--------- AdressBook 1.0 ---------");

 while (ProgramIsRunning)
 
 // Print user options
 PrintUserOptions();
 var userInput = Console.ReadLine();

 switch (userInput)
 
 case "1":
 ab.CreateUser();
 break;
 case "2":
 ab.UpdateUserInformation();
 break;
 case "3":
 ab.RemovePersonFromList();
 break;
 case "4":
 ab.ShowAllPersonsInList();
 break;
 case "x":
 ProgramIsRunning = false;
 break;
 
 
 

 private static void PrintUserOptions()
 
 Console.WriteLine("Choose one of the following options: ");
 Console.WriteLine("#1 Create new user");
 Console.WriteLine("#2 Edit user information");
 Console.WriteLine("#3 Delete existing user");
 Console.WriteLine("#4 Show all users in adressBook");
 

 private static AdressBook StartProgram()
 
 AdressBook ab = new AdressBook();

 //Start program by loading saved users from txt-file
 WriteAndReadToFile writer = new WriteAndReadToFile();
 writer.ReadFromFile(ab);
 return ab;
 

Person

 class Person
 
 public string FirstName get; set; 
 public string LastName get; set; 
 public string Adress get; set; 

 public Person(string firstName, string lastName, string adress)
 
 this.FirstName = firstName;
 this.LastName = lastName;
 this.Adress = adress;
 
 
}

AdressBook

class AdressBook

 private WriteAndReadToFile wtf;
 private List<Person> adressBookList = new List<Person>();
 public List<Person> AdressBookList
 
 get return adressBookList; 
 set this.adressBookList = value; 
 


 public AdressBook()
 
 AdressBookList = new List<Person>();
 wtf = new WriteAndReadToFile();
 

 // Create instance of Person-class and call AddPersonToList-method
 public void CreateUser()
 
 Console.WriteLine("Enter firstName:");
 var firstName = Console.ReadLine();

 Console.WriteLine("Enter lastName:");
 var lastName = Console.ReadLine();

 Console.WriteLine("Enter adress:");
 var adress = Console.ReadLine();

 Person person = new Person(firstName, lastName, adress);
 AddPersonToList(person);
 wtf.WriteUserToFile(person);

 

 // Add new person to AdressBookList
 private void AddPersonToList(Person person) => AdressBookList.Add(person);

 //Remove user from list where first and last name match
 public void RemovePersonFromList()
 
 Console.WriteLine("Enter firstName of the user you want to remove");
 var firstName = Console.ReadLine();

 Console.WriteLine("Enter lastname of the user you want to remove");
 var lastName = Console.ReadLine();

 AdressBookList.RemoveAll(item => item.FirstName == firstName && item.LastName == lastName);
 wtf.UpdateUserOnFile(adressBookList);
 

 //Show all Persons in AdressBookList
 public void ShowAllPersonsInList()
 
 foreach (var person in AdressBookList)
 
 Console.WriteLine("FirstName: 0, LastName: 1, Adress: 2", person.FirstName, person.LastName, person.Adress);
 
 

 public void UpdateUserInformation()
 
 Console.WriteLine("Which information do you want to update?");
 Console.WriteLine("#1: Firstname, #2: Lastname, 3# Adress");
 var userOption = Console.ReadLine();

 Console.WriteLine("Enter firstname on existing user to be updated");
 var oldFirstName = Console.ReadLine();
 UpdateUserFunction(userOption, oldFirstName);
 

 private void UpdateUserFunction(string userOption, string oldFirstName)
 
 var personsWithMatchingFirstName = AdressBookList.Where(person => person.FirstName == oldFirstName);
 string newValue;

 if(userOption == "1")
 
 Console.WriteLine("Enter a new first Name");
 newValue = Console.ReadLine();

 foreach (var person in personsWithMatchingFirstName)
 
 person.FirstName = newValue;
 wtf.UpdateUserOnFile(adressBookList);
 
 
 else if (userOption == "2")
 
 Console.WriteLine("Enter a new last name");
 newValue = Console.ReadLine();

 foreach (var person in personsWithMatchingFirstName)
 
 person.LastName = newValue;
 wtf.UpdateUserOnFile(adressBookList);
 
 
 else if(userOption == "3")
 
 Console.WriteLine("Enter a new adress");
 newValue = Console.ReadLine();

 foreach (var person in personsWithMatchingFirstName)
 
 person.Adress = newValue;
 wtf.UpdateUserOnFile(adressBookList);
 
 
 

WriteToFile

class WriteAndReadToFile


 private readonly string UserTextFile = ConfigurationManager.AppSettings["textFileName"];

 public void WriteUserToFile(Person person)
 
 using (StreamWriter sw = new StreamWriter(UserTextFile, true))
 
 sw.WriteLine(person.FirstName + "," + person.LastName + "," + person.Adress + ",");
 
 

 public void ReadFromFile(AdressBook ab)
 
 string textLine;
 try
 
 using (StreamReader sr = new StreamReader(UserTextFile))
 
 while ((textLine = sr.ReadLine()) != null)
 
 string userInformation = textLine.Split(',');
 Person p = new Person(userInformation[0], userInformation[1], userInformation[2]);
 ab.AdressBookList.Add(p);
 
 
 
 catch (FileNotFoundException fnf)
 
 Console.WriteLine("File does not exist " + fnf);
 
 catch (Exception e)
 
 Console.WriteLine("Something went wrong" + e);
 
 

 public void UpdateUserOnFile(List<Person> adressBookList)
 
 // Remove old row
 using (StreamWriter sw = new StreamWriter(UserTextFile))
 
 sw.Flush();
 foreach (var person in adressBookList)
 
 sw.WriteLine(person.FirstName + "," + person.LastName + "," + person.Adress + ",");
 
 
 

That's a great beginner's code!

One important thing to keep in mind when writing code is extendability. If tomorrow you wanted to add more user options, let's say remove all existing users, you would have to modify at least 2 methods, with your project growing, more such functions might arise, which leads to potentially forgetting to edit some of those methods and if you remember to modify them all, you still need to modify them all.

There are multiple techniques that you can use to avoid that problem, I will try to keep it simple.

Let's start with your program's Main method. Currently you have a switch there, which is usually indicator of bad code maintainability (most of the time), one way to solve it would be to introduce an interface and apply the Strategy Pattern, a simple and perhaps more suitable solution for your project, would be to avoid the interface and just use a class with a Dictionary:

public class AdressBookAction

 public int ID get; 
 public string Description get; 
 public Action<AdressBook> UserAction get; 

 public AdressBookAction(int id, string description, Action<AdressBook> userAction)
 
 ID = id;
 Description = description;
 UserAction = userAction;
 

 public override string ToString()
 
 return $"Press ID to Description";
 

The ID property is used to signify to the user how to access this command.

The UserAction is a delegate, which means that it holds a reference to a method, more specifically it's Action<AdressBook> meaning that any method with no return type and a single parameter of type AdressBook will be suitable, this will allow us to use this single class in various cases and declare all of the necessary actions using only this class:

private static Dictionary<string, AdressBookAction> _userActions = new Dictionary<string, AdressBookAction>

 ["1"] = new AdressBookAction(1, "Create new user", book => book.CreateUser()),
 ["2"] = new AdressBookAction(2, "Edit user information", book => book.UpdateUserInformation()),
 ["3"] = new AdressBookAction(3, "Delete existing user", book => book.RemovePersonFromList()),
 ["4"] = new AdressBookAction(4, "Show all users in adressBook", book => book.ShowAllPersonsInList()),
;

You could write another class to wrap the Dictionary there, which would remove the potential discrepancy between the Key and the ID of the AdressBookAction, but I will leave it as it is.

With your Dictionary declared your Main method's while loop will look like this:

while (ProgramIsRunning)

 // Print user options
 PrintUserOptions();
 var userInput = Console.ReadLine();

 if (userInput == "x")
 
 ProgramIsRunning = false;
 
 else if (_userActions.TryGetValue(userInput, out var action))
 
 action.UserAction.Invoke(ab);
 

Also PrintUserOptions, currently requires manual change, because your options are not stored in any collection, with the Dictionary, we can simply do:

private static void PrintUserOptions()

 Console.WriteLine("Choose one of the following options: ");
 Console.WriteLine(string.Join(Environment.NewLine, _userActions.Values));

This will utilize the overridden ToString() method in the AdressBookAction class.

Moving on to your AdressBook class.

You can apply similar technique to your UpdateUserFunction, I will leave that up to you.

You shouldn't not use the backing field of a property outside of the property's body.

wtf.UpdateUserOnFile(adressBookList);

Methods are very useful, they allow you to avoid repetitive code and they reduce the need for comments, multiple related instructions unified under 1 descriptive name. The reason I'm bringing this up is this part:

AddPersonToList(person);
wtf.WriteUserToFile(person);

I can think of few cases where you would want those separated, but in your project it seems that they would be better off in a single method.

Oh I forgot to mention that they usually serve 1 purpose, take a look at those 2 functions:

// Create instance of Person-class and call AddPersonToList-method
public void CreateUser()

 Console.WriteLine("Enter firstName:");
 var firstName = Console.ReadLine();

 Console.WriteLine("Enter lastName:");
 var lastName = Console.ReadLine();

 Console.WriteLine("Enter adress:");
 var adress = Console.ReadLine();

 Person person = new Person(firstName, lastName, adress);
 AddPersonToList(person);


// Add new person to AdressBookList
private void AddPersonToList(Person person)

 AdressBookList.Add(person);
 wtf.WriteUserToFile(person);

Now take a look at this one:

//Remove user from list where first and last name match
public void RemovePersonFromList()

 Console.WriteLine("Enter firstName of the user you want to remove");
 var firstName = Console.ReadLine();

 Console.WriteLine("Enter lastname of the user you want to remove");
 var lastName = Console.ReadLine();

 AdressBookList.RemoveAll(item => item.FirstName == firstName && item.LastName == lastName);
 wtf.UpdateUserOnFile(adressBookList);

Did you spot the difference? (besides the fact that they serve 2 different roles).

Spelling

This isn't that big of a deal but it was the first thing I noticed, Address has two d's. The reason this doesn't matter that much is that you're consistent; however, it might confuse someone having it spelt wrong so take that into consideration. There are very handy tools in visual studio for renaming variables.

Repetition

Your UpdateUserFunction is very repetitive. Personally, I would have wrote it like this to avoid large repeated chucks of code; however, sometimes simple is better than not repeating yourself.

private void UpdateUserFunction(string userOption, string oldFirstName)

 var personsWithMatchingFirstName = AdressBookList.Where(person => person.FirstName == oldFirstName);
 string newValue;

 switch (userOption)
 
 case "1":
 UpdateUser("first name", person => person.FirstName = newValue);
 break;
 case "2":
 UpdateUser("last name", person => person.LastName = newValue);
 break;
 case "3":
 UpdateUser("adress", person => person.Adress = newValue);
 break;
 default:
 Console.WriteLine("Unknown user option!");
 break;
 


private void UpdateUser(string label, Action<Person, string> updateAction)

 Console.WriteLine($"Enter a new label");
 var newValue = Console.ReadLine();

 foreach (var person in personsWithMatchingFirstName)
 
 updateAction(person, newValue);
 wtf.UpdateUserOnFile(adressBookList);
 

Inconsistency

I noticed that in the first part of your program you use a switch statement for handling userOption but then in the UpdateUserFunction you used an if-statement. The switch statement is a nice use in this case since you're comparing the same variable and you could end up adding more userOption values later.

var

You can use the keyword var instead of typing the variables type twice (e.g. StreamWriter sw = new StreamWriter(UserTextFile) can become var sw = new StreamWriter(UserTextFile). This is mostly preference, but you can imagine how convenient this will be. It has no effect on performance, the compiler is just deducing the type at compile-time.

Conclusion

That's all the nit-picking I could manage, haha. Overall, your code looks pretty good, especially for a beginner!

Separation

First thing which I notice is that your code is very tightly coupled with the console. I can imagine that you, in version 2.0, want to create a UI for your users.

An improvement could be to separate the code which involves getting the input for the firstname, lastname and address and the code that involves validating and storing the input.

So let the Console part be in charge of getting the required input and make your own code accept and process that input.

Example:

Responsibilities

Next thing is try to assign the appropriate responsibilities to the different objects that you have. Currently the AddressBook class has a method for creating a User (Or Person). Try to think about the real life situation, an it's not the responsibility of the address book to create a new person, it only has the option to add a new person entry.
The address book has too much responsibilities now.

Example:

class Program
{
 static void Main(string args)
 
 bool ProgramIsRunning = true;
 AdressBook ab = StartProgram();

 Console.WriteLine("--------- AdressBook 1.0 ---------");

 while (ProgramIsRunning)
 
 // Print user options
 PrintUserOptions();
 var userInput = Console.ReadLine();

 switch (userInput)
 
 case "1":
 
 Console.WriteLine("Enter firstName:");
 var firstName = Console.ReadLine();

 Console.WriteLine("Enter lastName:");
 var lastName = Console.ReadLine();

 Console.WriteLine("Enter adress:");
 var adress = Console.ReadLine();

 var person = new Person(firstName, lastName, adress);
 ab.AddEntry(person);
 
 break;
 
 
 


public class AdressBook

 private WriteAndReadToFile wtf;
 private List<Person> adressBookList = new List<Person>();
 public List<Person> AdressBookList
 
 get return adressBookList; 
 set this.adressBookList = value; 
 


 public AdressBook()
 
 AdressBookList = new List<Person>();
 wtf = new WriteAndReadToFile();
 


 public void AddEntry(Person person)
 
 AddPersonToList(person);
 wtf.WriteUserToFile(person);
 

In my example you'll see that I've moved the Console.Readlines to the Main method, when you now want to move away from using the console as input, you are able to do so. Just replace the Console.Readlines with input you got from a javascript frontend or WPF UI.

You'll also see that I moved the creating of the person to the main method, the address book was doing two things, creating a person and adding it to the addressbooklist.

Try to follow the Single Responsibility principle this will help maintaining and re-usability of the code (https://stackoverflow.com/questions/10620022/what-is-an-example-of-the-single-responsibility-principle)

Question answers:

I found it hard to keep the text-file in a correct "state" after updates & deletes and begun to flush the file after and rewrite the file instead. Is that bad practice?

Apply the single responsibility principle here. In your code right now every method is also responsible for writing the create/update/delete actions to the file.

You could choose to only "update" the file once by writing the latest state of the address book to file.

My suggestion would be to have a method which writes you entire address book to a file and you would call this when the application is stopped (in your case when 'x' is pressed)