开发者

What is wrong with this class? (design wise)

Following up from this question: designing application classes

What is wrong (from a design point of view) with this class:

I'm trying to refactor this class and it's abstract base class (Logon) and the fact it's actually horrible design. I wrote it myself (pretty much when I was a newbie). I'm finding it hard to refactor and want some input on it?

 class NewUserLogon : Logon, ILogonNewUser, IDisposable
    {
        #region Member Variables

        System.Windows.Forms.Form _frm = new MainWindow();

        SQLDatabase.SQLDynamicDatabase sql;
        SQLDatabase.DatabaseLogin dblogin;
        LogonData lgndata;
        System.Security.SecureString securepassword;
        PasswordEncrypt.Collections.CreatedItems items;

        LogonEventArgs e = new LogonEventArgs();

        #endregion

        #region Constructors
        // for DI
        public NewUserLogon(PasswordEncrypt.Collections.CreatedItems items) : base (items)
        {
            this.items = items;
        }
        #endregion

        #region Public Methods
        public new void Dispose()
        {
        }

        public  bool? ReadFromRegistry(HashedUsername username, HashedPassword hashedpassword)
        {
            return RegistryEdit.ReadFromRegistry(username, hashedpassword);
        }

        public  bool WriteToRegistry(HashedUsername username, HashedPassword hashedpassword)
        {
            return RegistryEdit.WriteToRegistry(username, hashedpassword);
        }

        public override void Login(TextBox username, TextBox password)
        {
            base.Login(username, password);
            Login(username.Text, password.Text);
        }
        #endregion

        #region Protected Methods
        protected override void Login(string username, string password) // IS INSECURE!!! ONLY USE HASHES 
        {
            base.Login(username, password);

            if (_user is NewUserLogon) // new user
            {
                sql = new PasswordEncrypt.SQLDatabase.SQLDynamicDatabase();
                dblogin = new SQLDatabase.DatabaseLogin();
                lgndata = base._logondata;
                securepassword = new System.Security.SecureString();

                // Set Object for eventhandler
                items.SetDatabaseLogin = dblogin;
                items.SetSQLDynamicDatabase = sql; // recreates L
                items.Items = items;

                string generatedusername;

                // write new logondata to registry
                if (this.WriteToRegistry(lgndata.HahsedUserName, lgndata.HashedPsw))
                {
                    try
                    {
                        // Generate DB Password...
                        dblogin.GenerateDBPassword();

                        // get generated password into securestring
                        securepassword = dblogin.Password;

                        //generate database username
                        generatedusername = dblogin.GenerateDBUserName(username);

                        if (generatedusername == "Already Exists")
                        {
                            throw new Exception("Username Already Exists");
                        }

                        //create SQL Server database
                        try
                        {
                            sql.CreateSQLDatabase(dblogin, username);
                        }
    开发者_开发知识库                    catch (Exception ex)
                        {
                            //System.Windows.Forms.MessageBox.Show(ex.Message);
                            e.ErrorMessage = ex.Message;
                            e.Success = false;
                            OnError(this, e);
                        }
                    }
                    catch (Exception exc)
                    {
                        e.Success = false;
                        e.ErrorMessage = exc.Message;
                        OnError(this, e);
                    }
                    OnNewUserLoggedIn(this, e); // tell UI class to start loading... 
                }
                else
                {
                    System.Windows.Forms.MessageBox.Show("Unable to write to Registry!", "Registry Error", System.Windows.Forms.MessageBoxButtons.OK, System.Windows.Forms.MessageBoxIcon.Exclamation);
                }
            }

            else if (_user is ExistingUserLogon) // exising user
            {

               bool? compare = base._regRead;
               lgndata = base._logondata;

                if (compare == true)
                {
                   //Tell GUI to quit the 'busydialog' thread
                    OnMessage(this, e);
                    LogonFrm frm = LogonFrm.LogonFormInstance;

                   // tell user he already exists and just needs to login
                    // ask if user wants to logon straight away
                    System.Windows.Forms.DialogResult dlgres; 
                    dlgres = System.Windows.Forms.MessageBox.Show("Your login already exists, do you wan to login now?", "Login Exists", System.Windows.Forms.MessageBoxButtons.YesNo, System.Windows.Forms.MessageBoxIcon.Question);

                    if (dlgres == System.Windows.Forms.DialogResult.Yes)
                    {
                        ExistingUserLogon existinguser = new ExistingUserLogon(compare, lgndata);
                        existinguser.Error += new ErrorStatus(frm._newuser_Error);
                        existinguser.loginname = username;
                        existinguser.LoginNewUser();

                        ///TELL GUI THAT USER LOGIN SUCCEEDED, THROUGH EVENT
                        e.Success = true;
                        OnNewUserLoggedIn(this, e);

                    }
                    else
                    {  
                        e.Success = false;
                        e.ErrorMessage = "Failed";
                        OnError(this, e);

                    }
                }

            }


        }
        #endregion
    }


Your class tries to do too many things. Try to separate different responsibilities into separate classes (eg database access and UI stuff).
And why are you instantiating a new Form at the beginning of your class and don't seem to use it further on?


Your protected Login is way too long.


Security should be a cross cutting concern, not a base class. I don't know if you have aspect oriented programming techniques available to you, but extending a base class with security built into it seems like an abuse of inheritance to me.

0

上一篇:

下一篇:

精彩评论

暂无评论...
验证码 换一张
取 消

最新问答

问答排行榜