Skip to content

Commit 8badfc2

Browse files
dpaquettedpaquette
committed
Merge pull request dpaquette#3 from dpaquette/blog2
Filling out blog number two
2 parents 8dbb4f8 + 13eb441 commit 8badfc2

File tree

1 file changed

+115
-1
lines changed

1 file changed

+115
-1
lines changed

‎Blog 2 - Evaluating the Code.md

+115-1
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,118 @@ In the previous blog post in this series we blindly imported the entire project
22

33
Now that we have the source control set up we can start figuring out what our next steps are going to be. I won't worry too much about outlining what the solutions are in this post but I will attempt to identify problems. The remaining posts in this series will cover what actions we can take to improve our situation.
44

5-
Opening the project up in Visual Studio the first thing I note is that this is an old school website project. This sort of project tends to encourage higitypigilty coding with source files just thrown anywhere. Indeed there are image files mixed in with .html files mixed in with JavaScript and CSS files. This could use a real good organization. So we would like to move to a web project and add some organiztion.
5+
Opening the project up in Visual Studio the first thing I note is that this is an old school website project. This sort of project tends to encourage higitypigilty coding with source files just thrown anywhere. Indeed there are image files mixed in with .html files mixed in with JavaScript and CSS files. This could use a real good organization. So we would like to move to a web project and add some organiztion.
6+
7+
#Third Party Components
8+
9+
It looks like there might be some third party dependencies in the project. Just looking at the included DLLs I found:
10+
11+
- log4net
12+
- Lucene
13+
- SharpMimeTools
14+
15+
The version numbers on these suggest that they're pretty old. There is nothing wrong as a whole with old packages but I'm always keen on updating packages because of potential security holes. Hopefully these packages will just be drop in upgrades without any need for fixes. I do seem to remember that there was a breaking change to the log4net API that caused some complaints the time. That may need more work. We'll add it to the list of tasks.
16+
17+
There also seems to be a copy of jQuery and jQuery UI as well as a couple of other jQuery plugins. There are, in fact, 3 copies of various versions of jQuery, I'm not sure which is used so we should try to reduce that to just one version - probably a recent one.
18+
19+
There are a few other JavaScript libraries and components as well. CKEditor is the largest of these. Googling it we find that it is an open source text editor for the web. We also find that the version we have is from 2010 and that the project is very active. There must have been [40 releases](http://ckeditor.com/download/releases) since version 3.4.2. We'll put updating it on the list of tasks.
20+
21+
#Code Duplication
22+
23+
One of the best features of ASP.net is master pages. A master page can contain all of the common elements for a site. Things like navigation menus as well as JavaScript includes and CSS includes can live within a master page.
24+
25+
There are no master pages in BugTracker.net. If we open up a couple of files we can see that there are great similarities between them. They all have html elements and body elements as well as some very similar looking JavaScript. If we wanted to change the style of the pages or add a menu it would be quite an ordeal. The change would have to be made in every single page on the site.
26+
27+
We'll put adding a master page and hooking it into all the other pages as a task.
28+
29+
#Code Organization
30+
31+
If you start a new WebForms project you'll see that the pages that come out of the box have three parts:
32+
33+
- .aspx - This is the HTML portion of the page. I like to think of it as containing all the presentation code.
34+
- .aspx.designer - This automatically generated file contains the definitions of any controls from the .aspx page that have runat=server set on them.
35+
- .aspx.cs - The other half of the partial class defined in the designer file. This file contains any of your own logic. I like to think of is as a controller. (Everything in my brain is related to MVC, MVP or MVVM.)
36+
37+
This division of logic and presentation is pretty loose in WebForms. The ease of mixing up presentation and business logic between these files is one of the reasons I don't like WebForms. You _can_ write good, well separated, testable WebForms but, by Thor's nostril hair, it is hard.
38+
39+
The pages in BugTracker.Net contain all these parts in a single file. To be honest I didn't even know you could do that. In my mind this is a big problem. We need to, at the very least, split up the single files into the standard triple of files. This will make testing easier as well as refactoring and even distribution.
40+
41+
There are limited namespaces in the project. Namespaces are used to organize code into logical groups. They make it easier to reference one part of code from another and, shoudl it be needed, provide logical places to split the project into several projects.
42+
43+
On to the list with you, code reorganization.
44+
45+
#Database Access
46+
47+
I figure an application like this must have some database behind it. So I set out to figure out how it works. I randomly selected a .aspx file and looked through for anything resembling data access. I found something pretty quickly
48+
49+
```
50+
sql = "update bugs set
51+
bg_short_desc = N'$sd',
52+
bg_tags = N'$tags',
53+
bg_project = $pj,
54+
bg_org = $og,
55+
bg_category = $ct,
56+
bg_priority = $pr,
57+
bg_assigned_to_user = $au,
58+
bg_status = $st,
59+
bg_last_updated_user = $lu,
60+
bg_last_updated_date = @now,
61+
bg_user_defined_attribute = $udf
62+
$pcd_placeholder
63+
$custom_cols_placeholder
64+
where bg_id = $id
65+
end
66+
select @now";
67+
68+
sql = sql.Replace("$sd", short_desc.Value.Replace("'","''"));
69+
sql = sql.Replace("$tags", tags.Value.Replace("'", "''"));
70+
sql = sql.Replace("$lu", Convert.ToString(security.user.usid));
71+
sql = sql.Replace("$id", Convert.ToString(id));
72+
sql = sql.Replace("$pj", new_project);
73+
sql = sql.Replace("$og", org.SelectedItem.Value);
74+
sql = sql.Replace("$ct", category.SelectedItem.Value);
75+
sql = sql.Replace("$pr", priority.SelectedItem.Value);
76+
sql = sql.Replace("$au", assigned_to.SelectedItem.Value);
77+
sql = sql.Replace("$st", status.SelectedItem.Value);
78+
sql = sql.Replace("$udf", udf.SelectedItem.Value);
79+
sql = sql.Replace("$snapshot_datetime", snapshot_timestamp.Value);
80+
81+
...
82+
DateTime last_update_date = (DateTime) btnet.DbUtil.execute_scalar(sql);
83+
...
84+
foreach (DataRow drcc in ds_custom_cols.Tables[0].Rows)
85+
86+
```
87+
So three things here: the first is that we're using SQL strings in the application instead of any sort of object relational mapper. That's probably okay, I do basically the same thing for applicaitons written using very lighweight ORMs like [Dapper](https://github.com/StackExchange/dapper-dot-net). We can put using an ORM on the list but kind of low down.
88+
89+
The second thing is that we're using data tables. I had some bad experiences with data tables years ago and have avoided them like the plague since them. You can see just by looking at the usage of them that they add a lot of layers of indirection that are unnecessary. I would much rather that we use simple IEnumerables with a strongly typed object. IEnumerable<Bug> here would be perfect.
90+
91+
I left the most important thing for last: sql string replacements. The parameters for the SQL are simply being inserted without sanitizing them or using parameters. This is a huge deal as it opens up the application to SQL injection attacks. With a well crafted entry into a field on the page a user could wipe out our whole database. Some attempts are being made to sanitize the strings doing a replace on single quotes. That isn't good enough. It is very easy to forget to escape something and not every field is escaped. Failing to use parameters is insecure programming and high on the list of common exploits. It is actually number 1 on the [OWASP top 10 list for 2013](https://www.owasp.org/index.php/Top_10_2013-Top_10). Fixing these strings should be a high priority.
92+
93+
#Styling and Various Other Things
94+
95+
The style of the website is a big outdated and could use a face lift. It may seem odd that we would put a styling change on the list but sometimes a slight improvment in a visual area of the program makes people forget about the problems elsewhere.
96+
97+
I once worked on a project which had a lot of problems. It was old code and difficult to maintain. People hated using it because it didn't match their needs and was slow. In the first new release of it I changed the background color from beige to blue. Nothing else. However for the next week I had a series of complements about how much faster and better the new version was. I had literally changed nothing but the color but people were so delighted to see a style change that they thought other improvements had been made.
98+
99+
This story isn't unique, there is a similarly [great story](http://thedailywtf.com/Articles/The-Cool-Cam.aspx) about saving a game through the use of UI tweak. UI might not seem like it is important but it changes people's perception of the application.
100+
101+
There are some other minor things that jumped out at me as I was going through the application. Inconsistent naming was one of the things that bugged me. By convention C# variables and methods don't use underscores but there lots of places that is done and even some places where variables are named with the "my" prefix: My_Lucene, My_Mime. These aren't a big deal but it will make lives easier for future developers if we fix them.
102+
103+
#Priorities
104+
105+
We've got quite a list of tasks to address now. However some of them are more important than others. We need to make sure that we're addressing the issues that are most important first. This is a discussion that needs to be had with the business. Fixing things like the code organization will save time later on.
106+
107+
As we're writing a blog and not running a business we'll just pretend we've had a good discussion with the business. The priority list we've established looks like
108+
- Code organization
109+
- Third party libraries
110+
- Database access
111+
- Styling
112+
113+
As we move through the application we'll be making code style and structure improvements. The code we're living by is
114+
115+
Leave things better than we found them
116+
117+
Small incremental fixes start adding up over time. It is like paying down a mortgage: the more you manage to pay down the more money you have to roll into the principle. We'll outline these changes as we make them.
118+
119+
We've now got a series of tasks on our todo list to improve BugTracker.Net. The rest of the posts in this series will outline our solutions to the issues. By the time the series is over it is our hope that BugTracker.Net will be much improved and easier to maintain.

0 commit comments

Comments
 (0)