I can not stress to people enough about how important it is to do a code review. This is especially true if you have a staff of young developers on your team. Unfortunately, in my company we tend not to do code reviews. It mainly stems from the fact that there are no "Lead Developers" to mentor the less experienced developers. Now even though code is written with the best intentions there is always a right and a wrong way to do things. However, presenting your arguments to others needs to be done in a positive manner. The goal is not to criticize! We are simply just trying to show them a better way of doing things. Code Reviews are a great platform for learning.
As a DBA I typically end up getting phone calls from end users who are complaining about performance. The phone call then precedes by a SQL trace. Once I review the results of a trace, I can typically get an idea of how the application was written. In order to fix the problems, I occasionally will ask the developers for source code. When I do this, I typically get a strange, confused look because most of the developers I work with do not think that DBAs can read code. After some hesitation they will give me the code but only because they think that I won't know enough to find any problems with it. Anyway, here is a stored procedure that I recently discovered. This procedure was used to populate a combo box on a form. This procedure would have been executed every time the form was displayed.
1: ALTER PROCEDURE uspGetMonths
2: AS
3:
4: CREATE TABLE #tmpMonth(
5: Ident INT ,
6: Months VARCHAR(15)
7: )
8:
9: INSERT INTO #tmpMonth(Ident, Months) VALUES(0, '(Select One)')
10: INSERT INTO #tmpMonth(Ident, Months) VALUES(1, 'January')
11: INSERT INTO #tmpMonth(Ident, Months) VALUES(2, 'February')
12: INSERT INTO #tmpMonth(Ident, Months) VALUES(3, 'March')
13: INSERT INTO #tmpMonth(Ident, Months) VALUES(4, 'April')
14: INSERT INTO #tmpMonth(Ident, Months) VALUES(5, 'May')
15: INSERT INTO #tmpMonth(Ident, Months) VALUES(6, 'June')
16: INSERT INTO #tmpMonth(Ident, Months) VALUES(7, 'July')
17: INSERT INTO #tmpMonth(Ident, Months) VALUES(8, 'August')
18: INSERT INTO #tmpMonth(Ident, Months) VALUES(9, 'September')
19: INSERT INTO #tmpMonth(Ident, Months) VALUES(10, 'October')
20: INSERT INTO #tmpMonth(Ident, Months) VALUES(11, 'November')
21: INSERT INTO #tmpMonth(Ident, Months) VALUES(12, 'December')
22:
23: SELECT
24: Ident ,
25: Months
26: FROM
27: #tmpMonth (NOLOCK)
28: ORDER BY
29: Ident ASC
30:
31: DROP TABLE #tmpMonth
Immediately after seeing this procedure I asked the developer the following questions:
- Why would you use a stored procedure for this?
- Why didn't you just hard code the month values in a combo box. It is not like the month names are going to change!
- Is there a reason to make a round trip to the server just to populate a combo box with a list of month names in it?
Here is how I would rewrite this in C# code:
1: public Form1() {
2: InitializeComponent();
3: comboBox1.DataSource = new BindingSource(GetMonths(), null);
4: comboBox1.DisplayMember = "Value";
5: comboBox1.ValueMember = "Key";
6: }
7:
8: private Dictionary<int,string> GetMonths() {
9: Dictionary<int, string> results = new Dictionary<int, string>();
10: results.Add(0, "(Select One)");
11: for (int i = 1; i <= 12; i++) {
12: DateTime myDate = new DateTime(2000,i,1);
13: results.Add(myDate.Month, myDate.ToString("MMMM"));
14: }
15: return results;
16: }
Now even though this particular procedure would probably not be considered a major performance killer it definitely does not help the cause. If this procedure was deployed to production it would have been executed repeatedly and caused a lot of unnecessary network I/O. In my opinion, this is a perfect example of how a code review can save everyone a lot of time and aggravation. If you are not doing code reviews with your team then start doing them now. No excuses!
