55502f40dc8b7c769880b10874abc9d0

I'm a newb, so please be nice?

Comments regarding what I am doing very wrong would be most appreciated.

This program works (mostly) the way I think it should. One part not working is the insert of a new set requires restarting the app to see the additions.

TIA

//
// frmMain
//
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Text;
using System.Windows.Forms;
using Owf.Controls; // Custom header panel from openwinforms.com
using System.Data.SQLite;

namespace FlashCards
{
    public partial class frmMain : Form
    {
        // Current directory - Used to find the database.
        private string _currentDirectory = Environment.CurrentDirectory;
        // Place to store the currentDatabase info once frmMain is initialized.
        private string _currentDatabase;
        // Place to store the connectionString info.
        private string _connectionString;

        private SQLiteConnection _sqLiteConnection;
        private SQLiteCommand _sqLiteCommand;
        private SQLiteDataAdapter _sqLiteDataAdapter;
        private DataSet _dataSet = new DataSet();
        private DataTable _dataTable = new DataTable();

        private Image _answerOff;
        private Image _answerOn;

        private bool _answerEnabled = false;
        private bool _btnPrevEnabled = false;
        private bool _btnNextEnabled = false;

        private int _answerIndexStart = 0;
        private int _answerIndexEnd = 0;
        private int _answerIndexCurrent;

        public int AnswerIndexEnd
        {
            get
            {
                return _answerIndexEnd;
            }
        }

        public frmMain()
        {
            InitializeComponent();
            _answerOff = new Bitmap(typeof(frmMain), "off.png");
            _answerOn = new Bitmap(typeof(frmMain), "on.png");
            _currentDatabase = _currentDirectory + @"\FlashCards.db";
            _connectionString = @"Data Source=" + _currentDatabase + ";New=True;UTF8Encoding=True;Version=3";
            _answerIndexCurrent = _answerIndexStart;
            GetRecordCount();
            LoadData();
            SetRtfText();
            StatusLabelUpdate();
            btnNextGetStatus();
            
        }

        private void newToolStripMenuItem_Click(object sender, EventArgs e)
        {
            frmNewSet _frmNewSet = new frmNewSet(this);
            _frmNewSet.Show();
        }

        public void LoadData()
        {
            string sqlCommandText = "SELECT * FROM  Cards";

            SetConnection();
            _sqLiteConnection.Open();
            _sqLiteDataAdapter = new SQLiteDataAdapter(sqlCommandText, _sqLiteConnection);
            _dataSet.Reset();
            _sqLiteDataAdapter.Fill(_dataSet);
            _dataTable.Reset();
            _dataTable = _dataSet.Tables[0];
            _sqLiteConnection.Close();
        }

        private void SetConnection()
        {
            _sqLiteConnection = new SQLiteConnection(_connectionString);
        }

        public void ExecuteQuery(string qryText)
        {
            SetConnection();
            _sqLiteConnection.Open();

            _sqLiteCommand = _sqLiteConnection.CreateCommand();
            _sqLiteCommand.CommandText = qryText;

            _sqLiteCommand.ExecuteNonQuery();
            _sqLiteConnection.Close();
        }

        private void AnswerButtonChangeImage()
        {
            switch (_answerEnabled)
            {
                case true:
                    _answerEnabled = false;
                    this.btnAnswerShow.Image = _answerOff;
                    this.rtfBoxAnswer.Visible = false;
                    break;

                case false:
                    _answerEnabled = true;
                    this.btnAnswerShow.Image = _answerOn;
                    this.rtfBoxAnswer.Visible = true;
                    break;

                default:
                    _answerEnabled = false;
                    this.btnAnswerShow.Image = _answerOff;
                    this.rtfBoxAnswer.Visible = false;
                    break;
            }
        }

        private void btnAnswerShow_Click(object sender, EventArgs e)
        {
            AnswerButtonChangeImage();
        }

        public void GetRecordCount()
        {
            string sqlCommandText = "SELECT COUNT(*) AS Total FROM Cards;";

            SetConnection();
            _sqLiteConnection.Open();
            _sqLiteDataAdapter = new SQLiteDataAdapter(sqlCommandText, _sqLiteConnection);
            _dataSet.Reset();
            _dataTable.Reset();
            _sqLiteDataAdapter.Fill(_dataSet);
            _dataTable = _dataSet.Tables[0];
            _sqLiteConnection.Close();
            _answerIndexEnd = Convert.ToInt32(_dataTable.Rows[_answerIndexStart]["Total"]);
        }

        private void SetRtfText()
        {
            if ((_answerIndexCurrent < _answerIndexEnd) && (_answerIndexCurrent >= _answerIndexStart))
            {
                this.rtfBoxQuestion.Text = _dataTable.Rows[_answerIndexCurrent]["card_question"].ToString();
                this.rtfBoxAnswer.Text = _dataTable.Rows[_answerIndexCurrent]["card_answer"].ToString();
            }
        }

        private void btnNextGetStatus()
        {            
            switch (_answerIndexCurrent < (_answerIndexEnd - 1))
            {
                case true:
                    this.btnNext.Enabled = true;
                    break;

                case false:
                    this.btnNext.Enabled = false;
                    break;

                default:
                    this.btnNext.Enabled = false;
                    break;
            }
        }

        private void btnPrevGetStatus()
        {
            switch (_answerIndexCurrent > _answerIndexStart)
            {
                case true:
                    this.btnPrev.Enabled = true;
                    break;

                case false:
                    this.btnPrev.Enabled = false;
                    break;

                default:
                    this.btnPrev.Enabled = false;
                    break;
            }
        }

        private void btnPrev_Click(object sender, EventArgs e)
        {
            _answerIndexCurrent = _answerIndexCurrent - 1;
            _answerEnabled = true;
            AnswerButtonChangeImage();
            SetRtfText();
            btnPrevGetStatus();
            btnNextGetStatus();
            StatusLabelUpdate();
        }

        private void btnNext_Click(object sender, EventArgs e)
        {
            _answerIndexCurrent = _answerIndexCurrent + 1;
            _answerEnabled = true;
            AnswerButtonChangeImage();
            SetRtfText();
            btnNextGetStatus();
            btnPrevGetStatus();
            StatusLabelUpdate();
        }

        public void StatusLabelUpdate()
        {
            this.tStripLabel1.Text = @"Record # " + (_answerIndexCurrent + 1).ToString();
        }
    }
}

//
// frmAddNew
//
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Text;
using System.Windows.Forms;
using System.Data.SQLite;

namespace FlashCards
{
    public partial class frmNewSet : Form
    {
        private frmMain _frmMain;
        private int _answerIndexNext;
        private RichTextBox _rtfCurrent;

        public frmNewSet(frmMain mainFrm)
        {
            InitializeComponent();
            _frmMain = mainFrm;
            _rtfCurrent = new RichTextBox();
            _answerIndexNext = _frmMain.AnswerIndexEnd + 1;
        }

        private void btnCancel_Click(object sender, EventArgs e)
        {
            this.Close();
        }

        private void btnSave_Click(object sender, EventArgs e)
        {
            Add();

            rtfNewQuestion.Text = string.Empty;
            rtfNewAnswer.Text = string.Empty;

            _frmMain.LoadData();
            this.Close();
        }

        private void Add()
        {
            string txtSQLQuery = "INSERT INTO Cards VALUES (" + _answerIndexNext + ",'" + rtfNewQuestion.Text + "','" + rtfNewAnswer.Text + "')";
            _frmMain.ExecuteQuery(txtSQLQuery);
        }

        private void rtfNewQuestion_Enter(object sender, EventArgs e)
        {
            _rtfCurrent = this.rtfNewQuestion;
        }

        private void rtfNewAnswer_Enter(object sender, EventArgs e)
        {
            _rtfCurrent = this.rtfNewAnswer;
        }
        
        //
        // This method sends special keys like áéíóúñ¿ to the richtextbox  when entering new card data
        //
        private void btnSpaKey_Click(object sender, EventArgs e)
        {
            string _btnSpaKeyPressed = (sender as Button).Text;
            _rtfCurrent.Focus();
            SendKeys.Send(_btnSpaKeyPressed);

        }
    }
}

Refactorings

No refactoring yet !

F9a9ba6663645458aa8630157ed5e71e

Ants

November 1, 2009, November 01, 2009 09:10, permalink

No rating. Login to rate!

DANGER! SQL Injection vulnerability in frmNewSet.Add() method. Use parameterized queries instead of building SOL queries straight from the user.

http://xkcd.com/327/

Below is a quick first pass refactoring without attempting to fix the vulnerability, nor fix the failure to load the new questions and answers. Changes are:
- Replace switch statements on bools with if-else statements.
- Coalesce shared logic between true and false handling within if-else statements.
- Replace if statements with ? : operators for setting the button image.
- Replaced btnSpaKey_Click() logic that used SendKeys with setting the RichTextBox.SelectedText.
- Replace frmNewSet.Show() call with frmNewSet.ShowDialog().
- Set DialogResult on frmNewSet.btnCancel.
- Removed frmNewSet.btnCancel_Click() handler.
- Moved call to frmMain.LoadData() from frmNewSet.btnSave_Click() to frmMain.newToolStripMenuItem_Click().
- Added call to btnNextGetStatus() to frmMain.newToolStripMenuItem_Click().
- Made frmMain.LoadData() private again.
- Set frmMain._answerIndexEnd in frmMain.LoadData() by getting the row count.
- Got rid of (mis-used) _sqLitConnetion, _sqLiteCommand, _sqLiteDataAdapter, and _dataTable fields.
- Changed SetRtfText() to grab a reference to the data row directly.
- Renamed SetConnection() to GetConnection().
- Return new instance of SQLiteConnection from GetConnection().
- Change usage of SQLiteConnection's to take advantage of IDisposable and using statement.
- Got rid of GetRecordCount() method.
- Get rid of unused _btnNextEnabled and _btnPrevEnabled.

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Text;
using System.Windows.Forms;
using Owf.Controls; // Custom header panel from openwinforms.com
using System.Data.SQLite;

namespace FlashCards
{
    public partial class frmMain : Form
    {
        // Current directory - Used to find the database.
        private string _currentDirectory = Environment.CurrentDirectory;
        // Place to store the currentDatabase info once frmMain is initialized.
        private string _currentDatabase;
        // Place to store the connectionString info.
        private string _connectionString;

        private DataSet _dataSet;

        private Image _answerOff;
        private Image _answerOn;

        private bool _answerEnabled = false;

        private int _answerIndexStart = 0;
        private int _answerIndexEnd = 0;
        private int _answerIndexCurrent;

        public int AnswerIndexEnd
        {
            get
            {
                return _answerIndexEnd;
            }
        }

        public frmMain()
        {
            InitializeComponent();
            _answerOff = new Bitmap(typeof(frmMain), "off.png");
            _answerOn = new Bitmap(typeof(frmMain), "on.png");
            _currentDatabase = _currentDirectory + @"\FlashCards.db";
            _connectionString = @"Data Source=" + _currentDatabase + ";New=True;UTF8Encoding=True;Version=3";
            _answerIndexCurrent = _answerIndexStart;
            LoadData();
            SetRtfText();
            StatusLabelUpdate();
            btnNextGetStatus();
            
        }

        private void newToolStripMenuItem_Click(object sender, EventArgs e)
        {
            using(frmNewSet frmNewSet = new frmNewSet(this))
            {
                if(frmNewSet.ShowDialog() == DialogResult.OK)
                {
                    LoadData();
                    btnNextGetStatus();
                }
            }
        }

        private void LoadData()
        {
            string sqlCommandText = "SELECT * FROM  Cards";

            using(SQLiteConnection sqLiteConnection = GetConnection())
            {
                sqLiteConnection.Open();
                SQLiteDataAdapter sqLiteDataAdapter = new SQLiteDataAdapter(sqlCommandText, sqLiteConnection);
                _dataSet = new DataSet();
                sqLiteDataAdapter.Fill(_dataSet);
                _answerIndexEnd = _dataSet.Tables[0].Rows.Count;
            }
        }

        private SQLiteConnection GetConnection()
        {
            return new SQLiteConnection(_connectionString);
        }

        public void ExecuteQuery(string qryText)
        {
            using(SQLiteConnection sqLiteConnection = GetConnection())
            {
                sqLiteConnection.Open();

                SQLiteCommand sqLiteCommand = sqLiteConnection.CreateCommand();
                sqLiteCommand.CommandText = qryText;

                sqLiteCommand.ExecuteNonQuery();
            }
        }

        private void AnswerButtonChangeImage()
        {
            _answerEnabled = !_answerEnabled;
            this.rtfBoxAnswer.Visible = _answerEnabled;
            this.btnAnswerShow.Image = _answerEnabled ? _answerOn : _answerOff;
        }

        private void btnAnswerShow_Click(object sender, EventArgs e)
        {
            AnswerButtonChangeImage();
        }

        private void SetRtfText()
        {
            if ((_answerIndexCurrent < _answerIndexEnd) && (_answerIndexCurrent >= _answerIndexStart))
            {
                DataRow dataRow = _dataSet.Tables[0].Rows[_answerIndexCurrent];
                this.rtfBoxQuestion.Text = dataRow["card_question"].ToString();
                this.rtfBoxAnswer.Text = dataRow["card_answer"].ToString();
            }
        }

        private void btnNextGetStatus()
        {            
            this.btnNext.Enabled = _answerIndexCurrent < (_answerIndexEnd - 1);
        }

        private void btnPrevGetStatus()
        {
            this.btnPrev.Enabled = _answerIndexCurrent > _answerIndexStart;
        }

        private void btnPrev_Click(object sender, EventArgs e)
        {
            _answerIndexCurrent = _answerIndexCurrent - 1;
            _answerEnabled = true;
            AnswerButtonChangeImage();
            SetRtfText();
            btnPrevGetStatus();
            btnNextGetStatus();
            StatusLabelUpdate();
        }

        private void btnNext_Click(object sender, EventArgs e)
        {
            _answerIndexCurrent = _answerIndexCurrent + 1;
            _answerEnabled = true;
            AnswerButtonChangeImage();
            SetRtfText();
            btnNextGetStatus();
            btnPrevGetStatus();
            StatusLabelUpdate();
        }

        public void StatusLabelUpdate()
        {
            this.tStripLabel1.Text = @"Record # " + (_answerIndexCurrent + 1).ToString();
        }
    }
}
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Text;
using System.Windows.Forms;
using System.Data.SQLite;

namespace FlashCards
{
    public partial class frmNewSet : Form
    {
        private frmMain _frmMain;
        private int _answerIndexNext;
        private RichTextBox _rtfCurrent;

        public frmNewSet(frmMain mainFrm)
        {
            InitializeComponent();
            btnCancel.DialogResult = DialogResult.Cancel;
            _frmMain = mainFrm;
            _rtfCurrent = new RichTextBox();
            _answerIndexNext = _frmMain.AnswerIndexEnd + 1;
        }

        private void btnSave_Click(object sender, EventArgs e)
        {
            Add();

            btnSave.DialogResult = DialogResult.OK;
        }

        private void Add()
        {
            //
            // DANGER! Here be dragons!
            // SQL injection vulnerability.
            //
            string txtSQLQuery = "INSERT INTO Cards VALUES (" + _answerIndexNext + ",'" + rtfNewQuestion.Text + "','" + rtfNewAnswer.Text + "')";
            _frmMain.ExecuteQuery(txtSQLQuery);
        }

        private void rtfNewQuestion_Enter(object sender, EventArgs e)
        {
            _rtfCurrent = this.rtfNewQuestion;
        }

        private void rtfNewAnswer_Enter(object sender, EventArgs e)
        {
            _rtfCurrent = this.rtfNewAnswer;
        }
        
        //
        // This method sends special keys like áéíóúñ¿ to the richtextbox  when entering new card data
        //
        private void btnSpaKey_Click(object sender, EventArgs e)
        {
            _rtfCurrent.SelectedText = (sender as Button).Text;
        }
    }
}
55502f40dc8b7c769880b10874abc9d0

etherealmonkey.myopenid.com

November 2, 2009, November 02, 2009 01:05, permalink

No rating. Login to rate!

Thank you for the review! I haven't got time to digest the changes fully at this moment, but I will definitely take another look at the SQL parameter suggestion and follow up.

Your refactoring





Format Copy from initial code

or Cancel