0
\$\begingroup\$

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?

\$\endgroup\$
3
  • \$\begingroup\$ This question has nothing to do with OOP. You have a single function which utilizes built-in classes and functionalities. \$\endgroup\$ Sep 3 at 9:48
  • 1
    \$\begingroup\$ BTW using blocks will take care of the Close method calls, you don't have to call sr.Close and sw.Close explicitly. \$\endgroup\$ Sep 3 at 9:50
  • 1
    \$\begingroup\$ Why you store usernames in the list? In your code you just check on this list if the user is exists. You do work twice. Better just read the username form the file and check if current username exists. No need to open stream writer when user exists because it is pointless operation. \$\endgroup\$
    – Alexey Nis
    Sep 3 at 11:47
2
\$\begingroup\$

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;
}
\$\endgroup\$
2
  • \$\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
    Sep 5 at 5:24
  • \$\begingroup\$ @AlexeyNis updated for better line by line handling \$\endgroup\$ Sep 7 at 20:46

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.