I am writing a program that takes user input and writes it to a CSV file. I am also checking to see if what they have input is a duplicate of whatever is in that CSV file. So far, the flow of this code snippet is:
- Get user input ->
- Iterate through the CSV file and assign it to a list ->
- If the list is empty (null) or if it doesn't contain a username ->
- Write data to CSV.
- If user input is a duplicate -> don't write
I'm a bit new to OOP and C# in general, so any tips would be appreciated.
private string writeUser()
{
string filePath = "usernames.csv";
List<string> users = new List<string>();
try
{
using (StreamReader sr = new StreamReader(filePath))
{
while (sr.Peek() >= 0) // iterate through the csv and add it to our users list
{
users.Add(sr.ReadLine());
}
sr.Close();
}
using (StreamWriter sw = new StreamWriter(filePath, true))
{
if (users == null | !users.Contains(this.username)) // write to csv if users is empty, or if it doesnt contain the typed username
{
sw.WriteLine(this.username);
sw.Close();
}
else if (users.Contains(this.username))
{
Console.WriteLine("username already exists");
sw.Close();
}
}
}
catch (Exception e)
{
Console.WriteLine("Some Error Occured: \n" + e.Message);
Console.ReadLine();
}
return this.username;
}
How can I simplify this/make what I have better?
1 Answer 1
We can rewrite this to be more declarative. Not to mention StreamReader
and StreamWriter
are fairly low level types, in most cases there is a different call to make to express your intent better.
private string writeUser()
{
string filePath = "usernames.csv";
var found = false;
//we don't modify the existing list, don't store it
//if storing, I would use HastSet<> for better duplicate checking
if(File.Exists(filePath))
foreach(var user in File.ReadLines(filePath))
//declare you want to process each line, but not necessarily
//read the whole file like below
{
if(user == username)
{
found = true;
break;//found it, stop looping
}
}
if(!found)
{
//AppendText will also Create the file for you too if not there
//skip boolean values that you need to check what the parameter affect
using (StreamWriter sw = File.AppendText(filePath))
{
sw.WriteLine(username);
}
}
return username;
}
-
\$\begingroup\$ The file can be huge! It is not a good idea to read all file (ReadAllLines method reads whole file to the memory)... Otherwise, I completely agree \$\endgroup\$Alexey Nis– Alexey Nis2021年09月05日 05:24:14 +00:00Commented Sep 5, 2021 at 5:24
-
\$\begingroup\$ @AlexeyNis updated for better line by line handling \$\endgroup\$Michael Rieger– Michael Rieger2021年09月07日 20:46:19 +00:00Commented Sep 7, 2021 at 20:46
using
blocks will take care of theClose
method calls, you don't have to callsr.Close
andsw.Close
explicitly. \$\endgroup\$