Should I leave this static variable, or refactor it?

Question

A previous developer placed a static string called "Qry" in a "god" class in a project I inherited.

The developer then used this static variable in every single place throughout the program that a single db query string is built and used.

For instance:

SomeGodClass.Qry = "select count(1) from AddressBook where Name = '" + txtName.Text.Trim(' ') +
                            "' and Group_Name = '" + txtGroupName.Text.Trim(' ') + "'";

int count = sqlHelper.ExecuteScalar(SomeGodClass.Qry);

Thus, this variable is referenced exactly 626 times, most with a different query being assigned. There are other static variables like this that he used - probably 50 of them - but this is the most predominant.

My first instinct is to remove this static string and rework all 626 usages. However, I don't know if this is a bad enough practice to take the time to do it.

Thus, my question is: Is this an acceptable use of a static string, especially when taking into consideration the amount of work refactoring would take?

score 3 · Accepted Answer · edited May 23 '17 at 11:45

3

Don't see any problem of using static here, at least judging from the code I see. But do not use TextBox concatenation !

Use Parameters instead.

That is most important refactoring I can think of while looking on the code provided.

Why do we always prefer using parameters in SQL statements?

edited May 23 '17 at 11:45

Community

1
1

answered Apr 04 '16 at 21:37

Tigran

61,654
8
86
123

In my time as a .Net developer, I have worked with one employer who already did things "the right way," and that's all I've really done. The method the developers I've replaced at my new job did it a very different way, and your links have helped me to understand a number of changes I need to make. – Kiel Apr 05 '16 at 13:08

score 1 · Answer 2 · answered Apr 05 '16 at 06:53

You should definitely refactor this. It looks like SomeGodClass.Qry was ment to be a constant of some kind, but what good is a constant if you keep reassigning it? Now you never know what's its value, unless you have just overwritten it. Just use a local variable query instead, and use parameters (like Tigran said)

score 1 · Answer 3 · answered Apr 07 '16 at 19:43

One of the main issues with using a static member for code like this is that at first glance, it looks fine (if a little redundant).

SomeGodClass.Qry = /* Some Query */;

int count = sqlHelper.ExecuteScalar(SomeGodClass.Qry);

It assigns a value, then uses it, great. The problem comes if you start introducing threads into your application. Suddenly, the static is being assigned and used from multiple threads and there's no guarantee that the value used in the Execute is the same one that was assigned higher up the method.

How big an issue this is, obviously depends on your application, if the code's in a library etc...

Very good point! Though there is no threading happening in this application currently, the time will come when I will likely have to implement it for the sake of responsiveness. — Kiel, Apr 08 '16 at 18:45

Should I leave this static variable, or refactor it?

3 Answers3