Login and registration functions

Question

I'm currently learning C# as first programming language and I've made a simple program to exercise the basic concepts. It's nothing fancy especially compared to advanced stuff you normally talk about, but I would like to know if I've made some common mistakes or I missed some good practices.

Basically I created a log-in and register functions that, once logged in, gives access to some simple apps like a calculator. I have to work out a way to store the registration data and eventually add more apps, but as I said it doesn't have a real utility outside making some practice with coding so for now it's just fine that it works as I intended.

    class MainClass
{
    public static List<string> nomi = new List<string>();
    public static List<string> password = new List<string> ();
    public static bool startup = true;
    public static bool logged = false;



    public static void Main (string[] args)
    {

        Start ();

    }


    //Starting loop
    public static void Start()
    {
        string choice;

        while (startup == true) 
        {

            Console.WriteLine ("Do you want to login or register?");

            choice = Console.ReadLine ();

            switch (choice) 
            {
            case "register":
                Register ();
                break;
            case "login":
                Login (ref logged, out startup);
                break;
            default:
                Console.WriteLine ("Error, please repeat.");
                Console.WriteLine ();
                break;
            }   

        } 

    }

    //Logged in loop
    public static void Run()
    {

        string app;

        while (logged == true) 
        {
            Console.WriteLine ("Choose an app: Calculator, Logout, Exit.");
            app = Console.ReadLine ();

            if (app == "calculator") {
                Calc ();
            } else if (app == "check") {
                Check();
            }else if (app == "exit") {
                Console.WriteLine ("Goodbye.");
                logged = false;
            } else if (app == "logout") {
                logged = false;
                startup = true;
                Console.WriteLine ();
                Start ();
            } else {
                Console.WriteLine ("Invalid app");
                Console.WriteLine ();
            }

        }

    }


    //Registration and login methods
    public static void Register()
    {
        Console.WriteLine ("Choose a Name: ");
        nomi.Add (Console.ReadLine ());
        Console.WriteLine ("Choose a password: ");
        password.Add (Console.ReadLine ());
        Console.WriteLine ();

    }


    public static bool Login(ref bool logged, out bool startup)
    {
        string name;
        string pwd;


        Console.WriteLine ("Write your username");
        name = Console.ReadLine ();

        foreach (string nome in nomi) {                                 

            if (nome == name) {
                Console.WriteLine ("Write your Password");
                pwd = Console.ReadLine ();

                foreach (string pword in password) {
                    if (pword == pwd) {
                        Console.WriteLine ("You are logged in");
                        Console.WriteLine ();
                        logged = true;
                        startup = false;
                        Run ();
                        return logged;
                    }

                }
                Console.WriteLine ("Incorrect Password");
                Console.WriteLine ();
                startup = true;
                return logged;
            } 
        }       

        Console.WriteLine ("Invalid username");
        Console.WriteLine ();
        startup = true;
        return logged;


    }

    //APPS

    public static void Calc()
    {
        float num1;
        float num2;
        string oper;


        Console.WriteLine ("Write the first number: ");
        num1 = float.Parse (Console.ReadLine ());
        Console.WriteLine ("Write the second number:");
        num2 = float.Parse (Console.ReadLine ());
        Console.WriteLine ("Select the operator: +, -, *, /");
        oper = Console.ReadLine ();
        switch (oper)
        {
        case "+":
            Console.WriteLine ("The result is: " + num1 + num2);
            Console.WriteLine ();
            break;
        case "-":
            Console.WriteLine ("The result is: " + (num1 - num2));
            Console.WriteLine ();
            break;
        case "*":
            Console.WriteLine ("The result is: " + num1 * num2);
            Console.WriteLine ();
            break;
        case "/":
            Console.WriteLine ("The result is: " + num1 / num2);
            Console.WriteLine ();
            break;
        default:
            Console.WriteLine ("Invalid Operator");
            Console.WriteLine ();
            break;
        }
    }


    public static void Check()
    {
        Console.WriteLine ("Names:");
        foreach (string name in nomi)
            Console.WriteLine (name);
        Console.WriteLine ("Passwords:");
        foreach (string pwd in password)
            Console.WriteLine (pwd);


    }


}

Answer 1

I have a few observations to make, which hopefully will help you out

1. In the start method you have a switch on the value of choice, now string comparison in c# by default is case sensitive and so you will only get the desired behaviour if the user types in lower case. The easiest way round this is to make use of the ToUpper or ToLower methods on the string which will give you a guaranteed case. Or if comparing strings follow the example shown here.

2. If you have defined logged as globally accessible in the class, why are you passing it from methods to other methods by reference. As its defined with a global scope it is not necessary to pass it to make use of it.

   Some people prefer not to declare variables with global scope and would instead pass it round. In this case I would say a global scope is the best way though.

3. In the run method I would have used a switch rather than the multiple nested if and else if as it produces more easily readable code.

Hope those observations proove useful.

Answer 2

A few extra observations that may or may not be helpful:

• I always like my method next to my brackets so it would be

Console.WriteLine("Invalid Operator");

rather than

Console.WriteLine ("Invalid Operator");

• Try and keep consistency with brackets, it's C#, so I tend to go with starting the opening parenthesis on a separate line, anyone who doesn't should go write Java or JavaScript instead (only joking!) e.g.

  if (app == "calculator")
  {
      Calc();
  }
  else if (app == "check")
  {
      Check();
  }
  

• Create a class, stick these methods in it, remove the static tags (note: Main remains where it is in the MainClass) then instantiate it in your Main method and call Start() e.g.

  public static void Main (string[] args)
  {
      var inst = new MyNewClass(); // I'm rubbish at naming things!
      inst.Start();
  }
  

• Have a look at enum and try to replace your dependency on string values like "calculator" with enum values instead

Have fun.

Answer 3

Let's talk about data structures. Right now you've got the usernames and passwords each in their own List<string>. Ideally, all the information for one user should be contained in a single class or struct:

struct UserInfo
{
  string name;
  string password;
}

Instead of having a List<> of these, use a data structure that allows searching by name, such as Dictionary<string,UserInfo>. This way you don't have to iterate through the whole list to find the right one.