Avatar

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

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
//
// 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.

frmMain.cs

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
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();
        }
    }
}

frmAddNew.cs

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
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;
        }
    }
}
Avatar

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