5
\$\begingroup\$

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);


    }


}
\$\endgroup\$
  • \$\begingroup\$ Pretty good, except you don't hash your passwords. Even though its just practice, you should be practicing it right. \$\endgroup\$ – AJMansfield Mar 31 '15 at 1:22
  • \$\begingroup\$ @AJMansfield That's some more advanced stuff that i decided i'm going to look after i have mastered the basics. I know the importance of hashing and how in depth this subject can go, but i don't see myself writing something that actually stores sensitive data in the foreseeable future so i decided to focus on other stuff for now. Thank you for the advice anyway, that article is extremely informative. \$\endgroup\$ – seesharp Mar 31 '15 at 15:22
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your feedback. 1) That was some information i really missed, but i noticed the problem when i misspelled something during some testing, thanks. 2) Good point. I used to pass the variables in an "older" version of the script in which the loops were both in Main and the variables where declared there too. Then i decided to put everything in separate functions using global variables, but for some reason i thought i still had to pass it. \$\endgroup\$ – seesharp Mar 30 '15 at 16:37
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Actually that's funny: when i started i preferred to write brackets like you said, but then i've seen that my ide (Xamarin) always changed it in the other way so i thought that was some sort of standard and i just dealt with it. I'm looking at enum values, nice advice it could make the code a little more clean and organized. \$\endgroup\$ – seesharp Mar 31 '15 at 14:20
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the input, so i should create a struct for each new user containing his information? I'm going to have a deeper look at this matter, i must admit it's not entirely clear. Probably i have to understand better the "object oriented" concepts of coding. \$\endgroup\$ – seesharp Mar 31 '15 at 14:34
  • \$\begingroup\$ You need to define the struct once, then create instances of it for each user. Related data should be kept together. It's one of the principles of object-oriented coding but it goes far beyond that. Basically, if you have two List<>s how can anyone be sure that (a) they have the same length and (b) the elements correspond? By having a List<UserInfo> those are guaranteed. \$\endgroup\$ – Snowbody Apr 2 '15 at 13:27

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.