A little bug in my Login system

Question

Hi, I have a little problem with my simple login system. this is the code

   static class Program
   {
      [STAThread]
         static void Main()
         {
            Application.EnableVisualStyles();
            Application.SetCompatibleTextRenderingDefault(false);

            bool loginSuccessful;
            bool loginRetry;
            using (Login login = new Login())
            {
               loginSuccessful = (login.ShowDialog() == DialogResult.OK);
               loginRetry = (login.ShowDialog() == DialogResult.Retry);
               if (loginSuccessful)
               {
                  Application.Run(new Form1());
               }
               if (loginRetry)
               {
                  Application.Run(new Login());
               }
            }

         }
 }

It works but a little problem starts with these two lines :

               loginSuccessful = (login.ShowDialog() == DialogResult.OK);
               loginRetry = (login.ShowDialog() == DialogResult.Retry);

At firts the programm reaches the 'loginSuccessful-line', but when it reaches the next line the windows forms application starts to move from its position and waits for a new click on the login button before it decides to close itself and to move on the the next forms application or to stay at its place because of a wrong usercode/password combination.

how can I fix this ? Btw. this is .net, C# I dont want the forms application to move 1 position from left to right and ask for a new click action.

Answer 1

You're calling ShowDialog() twice. That can't be good.

Don't store the result in two variables (loginSuccessful, loginRetry), because then you have the same information stored in two places. Try instead:

switch (login.ShowDialog())
{
    case DialogResult.OK:
        Application.Run(new Form1());
        return;

    case DialogResult.Retry:
        Application.Run(new Login());
        return;

    default:
        throw new Exception("unexpected dialog result");
}

I don't think you should be calling Application.Run() in a nested context, but I don't have hard data on that. Maybe looping on login.ShowDialog() until it returns OK.

It doesn't seem right to have an authentication system return DialogResult to indicate success of authentication. these values are about which buttons were pressed.

You haven't shown us the context of this code, but I would bet that it's not secure. Most experienced programmers struggle with security (myself included). Having an experienced programmer do it is a recipe for disaster.

While I'm at it, I would name a dialog class with Dialog, as in LoginDialog. Having it be called Login will lead to name clashes soon.

Answer 2

A better aproach will be using FormClosing event in Login dialog, and if DialogResult is DialogResult.OK and user could is not authenticated, set the e.Cancel property to true, this way you don't have to create new instances of the Login dialog neither call ShowDialog twice since it's disposed when closed.

in Login dialog:

private void Login_FormClosing(object sender, FormClosingEventArgs e)
{
    if (this.DialogResult == DialogResult.OK) {
        // authenticate user 
        // if fails assign e.Cancel = true; to prevent login dialog to close
    }
}

Main body:

using (Login login = new Login())
{
    if (login.ShowDialog() == DialogResult.OK) {
        Application.Run(new Form1());
    }
}

Answer 3

Cople of changes. As previously mentioned you are calling ShowDialog twice, which is why you see the login screen twice. To support redo. you need some sort of loop that repeatedly shows the login form until the user enters the correct credentials. What you really need is something like the following:

static class Program 
{ 
    [STAThread] 
    static void Main() 
    { 
        Application.EnableVisualStyles(); 
        Application.SetCompatibleTextRenderingDefault(false); 

        DialogResult rc;

        do
        {
            using (Login login = new Login()) 
            { 
               rc = login.ShowDialog(); 
               if (rc == DialogResult.OK) 
               { 
                  Application.Run(new Form1()); 
               }  
            } 
         }
         while (rc == DialogResult.Retry) 
     }
 }