From time to time our clients ask us to look at software that was developed for them and assess its quality. This potentially leads to a refactor process where we might clean up code to reduce redundancies, make it more maintainable or to put it back on track to meet the end clients need. One of the observations that I have made and seems to resonate is with the way Adhoc SQL queries are written in code. I know that generally its not the greatest Idea in all cases and other solutions exist like stored procedures, ORM frameworks and the like but they are not always a viable solution to implement.
Back to my point.
Queries from what I notice are a concatenation of string parts and for the most part very difficult to easily understand because of this fragmentation. Core to the refactoring process is identifying the duplicate functionality and moving it into a shared component following a more DRY approach and increasing the overall maintainability of the project.
To this end I convert most of the adhoc sql statements to string.format( method calls that provide a string that looks as close to the requisite SQL as possible. That way through simpler inspection it is possible to see where the duplicate calls exist and identify proper patterns viable for a code refactor.
Write only enough code to do the job Right
Another thing that is really important is the KISS factor where doing only what is required for the current context is important. For example if you are using a constant value such as “T” do not call a format function as you are unnecessarily creating extra string objects and doing more work this is required. Set the value to “‘T’, ” if it is required and reduce the work.
Some Helper Functions can be Evil
We all write them, and use them extensively in our development, but helper functions need to be understood and applied in the most relevant places. For example a function that retrieves a single value from a database table can come in handy in a pinch. However, when the function needs to be called several times within a calling chain it is better to retrieve an object that is a composition of all of the needed values, reducing the round tripping to the database. For me one of the most evil functions that has come out of the SalesLogix scripting world and into the .net world is the get field call. This method makes it too easy go get values from the database and also too easy to write inefficient code.