Is it considered an anti pattern to write SQL in the source code?

Question

Is it considered an anti pattern to hardcode SQL into an application like this:

public List<int> getPersonIDs()
{    
    List<int> listPersonIDs = new List<int>();
    using (SqlConnection connection = new SqlConnection(
        ConfigurationManager.ConnectionStrings["Connection"].ConnectionString))
    using (SqlCommand command = new SqlCommand())
    {
        command.CommandText = "select id from Person";
        command.Connection = connection;
        connection.Open();
        SqlDataReader datareader = command.ExecuteReader();
        while (datareader.Read())
        {
            listPersonIDs.Add(Convert.ToInt32(datareader["ID"]));
        }
    }
    return listPersonIDs;
}

I would normally have a repository layer etc, but I have excluded it in the code above for simplicity.

I recently had some feedback from a colleague who complained that SQL was written in the source code. I did not get chance to ask why and he is now away for two weeks (maybe more). I assume that he meant either:

1. Use LINQ
   or
2. Use stored procedures for the SQL

Am I correct? Is it considered an anti pattern to write SQL in the source code? We are a small team working on this project. The benefit of stored procedures I think is that SQL Developers can get involved with the development process (writing stored procedures etc).

Edit The following link talks about hard coded SQL statements: https://docs.microsoft.com/en-us/sql/odbc/reference/develop-app/hard-coded-sql-statements. Is there any benefit of preparing an SQL statement?

Answer 1

You excluded the crucial part for simplicity. The repository is the abstraction layer for persistence. We separate out persistence into its own layer so that we can change the persistence technology more easily when we need to. Therefore, having SQL outside of the persistence layer completely foils the effort of having a separate persistence layer.

As a result: SQL is fine within the persistence layer that is specific to a SQL technology (e.g. SQL is fine in a SQLCustomerRepository but not in a MongoCustomerRepository). Outside of the persistence layer, SQL breaks your abstraction and thus is considered very bad practice (by me).

As for tools like LINQ or JPQL: Those can merely abstract the flavours of SQL out there. Having LINQ-Code or JPQL queries outside of an repository breaks the persistence abstraction just as much as raw SQL would.

---

Another huge advantage of a separate persistence layer is that it allows you to unittest your business logic code without having to set up a DB server.

You get low memory-profile, fast unit tests with reproducible results across all platforms your language supports.

In an MVC+Service architecture this is a simple task of mocking the repository instance, creating some mock-data in memory and define that the repository should return that mock data when a certain getter is called. You can then define test data per unittest and not worry about cleaning up the DB afterwards.

Testing writes to the DB is as simple: verify that the relevant update methods on the persistence layer have been called and assert that the entities were in the correct state when that happened.

Answer 2

Most standard business applications today use different layers with different responsibilities. However, which layers you use for your application, and which layer has which responsibility is up to you and your team. Before you can make a decision about if it is right or wrong to place SQL directly in the function you have shown us you need to know

• which layer in your application has which responsibility

• from which layer the function above is from

There is no "one-size-fits-all" solution to this. In some applications the designers prefer to use an ORM framework and let the framework generate all SQL. In some applications the designers prefer to store such SQL exclusively in stored procedures. For some applications there is a hand-written persistence (or repository) layer where the SQL lives, and for other applications it is ok to define exceptions under certain circumstances from strictly placing SQL in that persistence layer.

So what you need to think about: which layers do you want or need in your particular application, and how do you want the responsibilities? You wrote "I would normally have a repository layer", but what are the exact responsibilities you want to have inside that layer, and what responsibilities do you want to put somewhere else? Answer that first, then you can answer your question by yourself.

Answer 3

Marstato gives a good answer but I'd like to add some more commentary.

SQL in source is NOT an anti-pattern but it can cause problems. I can remember when you used to have to put SQL queries into the properties of components dropped onto every form. That made things really ugly really fast and you had to jump through hoops to locate queries. I became a strong advocate of centralising the Database access as much as possible within the limitations of the languages I was working with. Your colleague may be getting flashbacks to these dark days.

Now, some of the comments are talking about vendor lock-in like it's automatically a bad thing. It isn't. If I'm signing a six figure cheque each year to use Oracle, you can bet that I want any application accessing that database to be using the extra Oracle syntax appropriately but to its fullest. I will not be happy if my shiny database is crippled by coders writing vanilla ANSI SQL badly when there is an "Oracle way" of writing the SQL that doesn't cripple the database. Yes, changing databases will be harder but I have only seen it done at a big client site a couple of times in over 20 years and one of those cases was moving from DB2 -> Oracle because the mainframe that hosted DB2 was obsolete and getting decommissioned. Yes that is vendor lock-in but for corporate customers it's actually desirable to pay for an expensive capable RDBMS like Oracle or Microsoft SQL Server and then utilize it to the fullest extent. You have a support agreement as your comfort blanket. If I'm paying for a database with a rich stored procedure implementation, I want it used for those cases where it makes sense to.

This leads to the next point, if you are writing an application that accesses a SQL Database you have to learn SQL as well as the other language and by learn, I mean query optimisation as well; I will get angry with you if you write SQL generation code that flushes the SQL cache with a barrage of nearly identical queries when you could have used one cleverly parameterised query.

No excuses, no hiding behind a wall of Hibernate. ORMs badly used can really cripple the performance of applications. I remember seeing a question on Stack Overflow a few years ago along the lines of:

In Hibernate I'm iterating through 250,000 records checking the values of a couple of properties and updating objects that match certain conditions. It's running a bit slow, what can I do to speed it up?

How about "UPDATE table SET field1 = <value> where field2 is True and Field3 >100". Creating and disposing of 250,000 objects may well be your problem...

i.e. Ignore Hibernate when it's not appropriate to use it. Understand the database.

So, in summary, embedding SQL in code can be bad practice but there are much worse things you can end up doing trying to avoid embedding SQL.

Answer 4

Yes, hard coding SQL strings into application code generally is an anti-pattern.

Let's try to set aside the tolerance we have developed from years of seeing this in production code. Mixing completely different languages with different syntax in the same file is generally not a desirable development technique. This is different than template languages like Razor which are designed to give contextual meaning to multiple languages. As Sava B. mentions in a comment below, SQL in your C# or other application language (Python, C++, etc.) is a string like any other and is semantically meaningless. The same applies when mixing more than one language in most cases, though there are obviously situations where doing so is acceptable, such as inline assembly in C, small and comprehensible snippets of CSS in HTML (noting that CSS is designed to be mixed with HTML), and others.

(Robert C. Martin on mixing languages, Clean Code, Chapter 17, "Code Smells and Heuristics", page 288)

For this response, I will focus SQL (as asked in the question). The following issues may occur when storing SQL as an à la carte set of disassociated strings:

• Database logic is difficult to locate. What do you search for to find all your SQL statements? Strings with "SELECT", "UPDATE", "MERGE", etc.?
• Refactoring uses of the same or similar SQL becomes difficult.
• Adding support for other databases is difficult. How would one accomplish this? Add if..then statements for each database and store all queries as strings in the method?
• Developers read a statement in another language and become distracted by the shift in focus from the method's purpose to the method's implementation details (how and from where data is retrieved).
• While one-liners may not be too much of a problem, inline SQL strings start to fall apart as statements become more complex. What do you do with a 113 line statement? Put all 113 lines in your method?
• How does the developer efficiently move queries back and forth between their SQL editor (SSMS, SQL Developer, etc.) and their source code? C#'s @ prefix makes this easier, but I have seen a lot of code that quotes each SQL line and escapes the newlines. "SELECT col1, col2...colN"\ "FROM painfulExample"\ "WHERE maintainability IS NULL"\ "AND modification.effort > @necessary"\
• Indentation characters used to align the SQL with surrounding application code are transmitted over the network with each execution. This is probably insignificant for small scale applications, but it can add up as the software's usage grows.

Full ORMs (Object-Relational mappers like Entity Framework or Hibernate) can eliminate randomly peppered SQL in application code. My use of SQL and resource files is but an example. ORMs, helper classes, etc. can all help accomplish the goal of cleaner code.

As Kevin said in an earlier answer, SQL in code can be acceptable in small projects, but large projects start out as small projects, and the probability most teams will go back and do it right is often inversely proportional to the code size.

There are many simple ways to keep SQL in a project. One of the methods that I often use is to put each SQL statement into a Visual Studio resource file, usually named "sql". A text file, JSON document, or other data source may be reasonable depending on your tools. In some cases, a separate class dedicated to ensconcing SQL strings may be the best option, but could have some of the issues described above.

SQL Example: Which looks more elegant?:

using(DbConnection connection = Database.SystemConnection()) {
    var eyesoreSql = @"
    SELECT
        Viewable.ViewId,
        Viewable.HelpText,
        PageSize.Width,
        PageSize.Height,
        Layout.CSSClass,
        PaginationType.GroupingText
    FROM Viewable
    LEFT JOIN PageSize
        ON PageSize.Id = Viewable.PageSizeId
    LEFT JOIN Layout
        ON Layout.Id = Viewable.LayoutId
    LEFT JOIN Theme
        ON Theme.Id = Viewable.ThemeId
    LEFT JOIN PaginationType
        ON PaginationType.Id = Viewable.PaginationTypeId
    LEFT JOIN PaginationMenu
        ON PaginationMenu.Id = Viewable.PaginationMenuId
    WHERE Viewable.Id = @Id
    ";
    var results = connection.Query<int>(eyesoreSql, new { Id });
}

Becomes

using(DbConnection connection = Database.SystemConnection()) {
    var results = connection.Query<int>(sql.GetViewable, new { Id });
}

The SQL is always in an easy-to-locate file or grouped set of files, each with a descriptive name that describes what it does rather than how it does it, each with space for a comment that will not interrupt the flow of application code:

This simple method executes a solitary query. In my experience, the benefit scales as use of the "foreign language" grows more sophisticated. My use of a resource file is just an example. Different methods may be more appropriate depending on the language (SQL in this case) and platform.

This and other methods resolve the list above in the following manner:

1. Database code is easy to locate because it is already centralized. In larger projects, group like-SQL into separate files, perhaps under a folder named SQL.
2. Support for a second, third, etc. databases is easier. Make an interface (or other language abstraction) that returns each database's unique statements. The implementation for each database becomes little more than statements similar to: return SqlResource.DoTheThing; True, these implementations can skip the resource and contain the SQL in a string, but some (not all) problems above would still surface.
3. Refactoring is simple -- just reuse the same resource. You can even use the same resource entry for different DBMS systems much of the time with a few format statements. I do this often.
4. Use of the secondary language can use descriptive names e.g. sql.GetOrdersForAccount rather than more obtuse SELECT ... FROM ... WHERE...
5. SQL statements are summoned with one line regardless of their size and complexity.
6. SQL can be copied and pasted between database tools like SSMS and SQL Developer without modification or careful copying. No quotation marks. No trailing backslashes. In the case of the Visual Studio resource editor specifically, one click highlights the SQL statement. CTRL+C and then paste it into the SQL editor.

Creation of SQL in a resource is quick, so there is little impetus to mix resource usage with SQL-in-code.

Regardless of chosen method, I have found that mixing languages usually reduces code quality. I hope that some issues and solutions described here help developers eliminate this code smell when appropriate.

Answer 5

It depends. There are a number of different approaches that can work:

1. Modularize the SQL, and isolate it into a separate set of classes, functions, or whatever unit of abstraction your paradigm uses, then call into it with application logic.
2. Move all complex SQL into views, and then only do very simple SQL in the application logic, so that you don't need to modularize anything.
3. Use an object-relational mapping library.
4. YAGNI, just write SQL directly in the application logic.

As is often the case, if your project has already chosen one of these techniques, you should be consistent with the rest of the project.

(1) and (3) are both relatively good at maintaining independence between the application logic and the database, in the sense that the application will continue to compile and pass basic smoke tests if you replace the database with a different vendor. However, most vendors do not fully conform to the SQL standard, so replacing any vendor with any other vendor is likely to require extensive testing and bug hunting regardless of which technique you use. I'm skeptical that this is as big of a deal as people make it out to be. Changing databases is basically a last resort when you can't get the current database to meet your needs. If that happens, you probably chose the database poorly.

The choice between (1) and (3) is mostly a matter of how much you like ORMs. In my opinion they are overused. They are a poor representation of the relational data model, because rows do not have identity in the way that objects have identity. You are likely to encounter pain points around unique constraints, joins, and you may have difficulty expressing some more complicated queries depending on the power of the ORM. On the other hand, (1) will probably require significantly more code than an ORM would.

(2) is rarely seen, in my experience. The problem is that many shops prohibit SWEs from directly modifying the database schema (because "that's the DBA's job"). This is not necessarily a Bad Thing in and of itself; schema changes have a significant potential for breaking things and may need to be rolled out carefully. However, in order for (2) to work, SWEs should at least be able to introduce new views and modify the backing queries of existing views with minimal or no bureaucracy. If this is not the case at your place of employment, (2) probably won't work for you.

On the other hand, if you can get (2) to work, it's much better than most other solutions because it keeps relational logic in SQL instead of application code. Unlike general purpose programming languages, SQL is specifically designed for the relational data model, and is accordingly better at expressing complicated data queries and transformations. Views can also be ported along with the rest of your schema when changing databases, but they will make such moves more complicated.

For reads, stored procedures are basically just a crappier version of (2). I don't recommend them in that capacity, but you might still want them for writes, if your database doesn't support updatable views, or if you need to do something more complex than inserting or updating a single row at a time (e.g. transactions, read-then-write, etc.). You can couple your stored procedure to a view using a trigger (i.e. CREATE TRIGGER trigger_name INSTEAD OF INSERT ON view_name FOR EACH ROW EXECUTE PROCEDURE procedure_name;), but opinions vary considerably as to whether this is actually a Good Idea. Proponents will tell you that it keeps the SQL that your application executes as simple as possible. Detractors will tell you that this is an unacceptable level of "magic" and that you should just execute the procedure directly from your application. I'd say this is a better idea if your stored procedure looks or acts a lot like an INSERT, UPDATE, or DELETE, and a worse idea if it is doing something else. Ultimately you'll have to decide for yourself which style makes more sense.

(4) is the non-solution. It may be worth it for small projects, or large ones which only sporadically interact with the database. But for projects with a lot of SQL, it's not a good idea because you may have duplicates or variations of the same query scattered around your application haphazardly, which interferes with readability and refactoring.

Answer 6

Is it considered an anti-pattern to write SQL in the source code?

Not necessarily. If you read all the comments here you will find valid arguments for hardcoding SQL statements in the source code.

The problem comes with where you place the statements. If you place the SQL statements all over the project, everywhere, you will be probably ignoring some of the SOLID principles we usually strive to follow.

assume that he meant; either:

1) Use LINQ

or

2) Use stored procedures for the SQL

We can't say what he meant. However, we can guess. For instance, the first that comes to my mind is vendor lock-in. Hardcoding SQL statements may lead you to couple tightly your application to the DB engine. For instance, using specific functions of the vendor that are not ANSI compliant.

This is not necessarily wrong nor bad. I'm just pointing to the fact.

Ignoring SOLID principles and vendor locks have possible adverse consequences you may be ignoring. That's why is usually good to sit with the team and expose your doubts.

The benefit of stored procedures I think is that SQL Developers can get involved with the development process (writing stored procedures etc)

I think It has nothing to do with the advantages of stored procedures. Moreover, if your colleague dislikes hardcoded SQL, It's likely moving the business to stored procedures will dislike him too.

Edit: The following link talks about hard-coded SQL statements: https://docs.microsoft.com/en-us/sql/odbc/reference/develop-app/hard-coded-sql-statements. Is there any benefit of preparing an SQL statement?

Yes. The post enums the advantages of prepared statements. It's a sort of SQL templating. More secure than strings concatenation. But the post is not encouraging you to go this way nor confirming that you are right. It just explains how can we use hardcoded SQL in a safe and efficient way.

Summarising, try asking your colleague first. Send a mail, phone him, ... Whether he answers or not, sit with the team and expose your doubts. Find the solution that best suites your requirements. Don't make false assumptions based on what you read out there.

Answer 7

I think it is a bad practice, yes. Others have pointed out the advantages of keeping all your data access code in its own layer. You don't have to hunt around for it, it's easier to optimize and test... But even within that layer, you have a few choices: use an ORM, use sprocs, or embed SQL queries as strings. I would say SQL query strings are by far the worst option.

With an ORM, development becomes a lot easier, and less error-prone. With EF, you define your schema just by creating your model classes (you would have needed to create these classes anyway). Querying with LINQ is a breeze - you often get away with 2 lines of c# where you'd otherwise need to write and maintain a sproc. IMO, this has a huge advantage when it comes to productivity and maintainability - less code, less problems. But there is a performance overhead, even if you know what you're doing.

Sprocs (or functions) are the other option. Here, you write your SQL queries manually. But at least you get some guarantee that they are correct. If you're working in .NET, Visual Studio will even throw compiler errors if the SQL is invalid. This is great. If you change or remove some column, and some of your queries become invalid, at least you're likely to find out about it during compile time. It's also way easier to maintain sprocs in their own files - you'll probably get syntax highlighting, autocomplete..etc.

If your queries are stored as sprocs, you can also change them without re-compiling and re-deploying the application. If you notice that something in your SQL is broken, a DBA can just fix it without needing to have access to your app code. In general, if your queries are in string literals, you dbas can't do their job nearly as easily.

SQL queries as string literals will also make your data access c# code less readable.

Just as a rule of thumb, magic constants are bad. That includes string literals.

Answer 8

It is not an anti-pattern (this answer is dangerously close to opinion). But code formatting is important, and the SQL string should be formatted so it is clearly separate from the code which uses it. For example

    string query = 
    @"SELECT foo, bar
      FROM table
      WHERE id = @tn";

Answer 9

I can not tell you what you colleague meant. But I can answer this:

Is there any benefit of preparing an SQL statement?

YES. Using prepared statements with bound variables is the recommended defense against SQL injection, which remains the single biggest security risk for web applications for over a decade. This attack is so common it was featured in web comics nearly ten years ago, and it is high time effective defenses were deployed.

... and the most effective defense to bad concatenation of query strings is not representing queries as strings in the first place, but to use a type-safe query API to build queries. For instance, here is how I'd write your query in Java with QueryDSL:

List<UUID> ids = select(person.id).from(person).fetch();

As you can see, there is not a single string literal here, making SQL injection neigh impossible. Moreover, I had code completion when writing this, and my IDE can refactor it should I choose to ever rename the id column. Also, I can easily find all queries for the person table by asking my IDE where the person variable is accessed. Oh, and did you notice it's quite a bit shorter and easier on the eyes than your code?

I can only assume that something like this is available for C# as well. For instance, I hear great things about LINQ.

In summary, representing queries as SQL strings makes it hard for the IDE to meaningfully assist in writing and refactoring queries, defers detection of syntax and type errors from compile time to run time, and is a contributing cause for SQL injection vulnerabilities [1]. So yes, there are valid reasons why one might not want SQL strings in source code.

[1]: Yes, correct use of prepared statements also prevents SQL injection. But that another answer in this thread was vulnerable to an SQL injection until a commenter pointed this out does not inspire confidence in junior programmers correctly using them whenever necessary ...

Answer 10

A lot of great answers here. Apart from what already has been said, I'd like to add one other thing.

I don't consider using inline SQL an anti-pattern. What I do however consider an anti-pattern is every programmer doing their own thing. You have to decide as a team on a common standard. If everyone is using linq, then you use linq, if everyone is using views and stored procedures then you do that.

Always keep an open mind though and don't fall for dogma's. If your shop is a "linq" shop you use that. Except for that one place where linq absolutely doesn't work. (But you shouldn't 99.9% of the time.)

So what I would do is research the code in the code base and check out how your colleagues work.

Answer 11

The code looks like from the database-interacting layer that is between the database and the rest of the code. If really so, this is not anti-pattern.

This method IS the part of the layer, even if OP thinks differently (plain database access, no mixing with anything else). It is maybe just badly placed inside the code. This method belongs to the separate class, "database interfacing layer". It would be anti-pattern to place it inside the ordinary class next to the methods that implement non-database activities.

The anti-pattern would be to call SQL from some other random place of code. You need to define a layer between the database and the rest of the code, but it is not that you must absolutely use some popular tool that could assist you there.

Answer 12

In my opinion no isn't an anti pattern but you will find yourself dealing with string formatting spacing, especially for big queries that cannot fit in one line.

another pros is it get much harder when dealing with dynamic queries that should built according to certain conditions.

var query = "select * from accounts";

if(users.IsInRole('admin'))
{
   query += " join secret_balances";
}

I suggest using a sql query builder

Answer 13

For many modern organizations with rapid deployment pipelines where code changes can be compiled, tested and pushed to production in minutes, it totally makes sense to embed SQL statements into the application code. This is not necessarily an anti-pattern depending on how you go about it.

A valid case for using stored procedures today is in organizations where there is a lot of process around production deployments that can involve weeks of QA cycles etc. In this scenario the DBA team can resolve performance issues that emerge only after the initial production deployment by optimizing the queries in the stored procedures.

Unless you are in this situation I recommend that you embed your SQL into your application code. Read other answers in this thread for advice on the best way to go about it.

---

Original text below for context

The main advantage of stored procedures is that you can optimize database performance without recompiling and redeploying your application.

In many organizations code can only be pushed to production after a QA cycle etc which can take days or weeks. If your system develops a database performance issue because of changing usage patterns or increases in table sizes etc then it can be quite difficult to track down the problem with hard coded queries, whereas the database will have tools/reports etc that will identify problem stored procedures. With stored procedures solutions can be tested/benchmarked in production and the issue can be fixed quickly without having to push a new version of the application software.

If this issue is not important in your system then it's a perfectly valid decision to hard code SQL statements into the application.

Answer 14

Compared to the use of an SQL View, yes it is an anti-pattern.

In the past, I have been against SQL anything. I have used ORMs and been an advocate for LINQ everywhere. I was wrong.

The case for SQL Views:

• Some people half-correctly discuss the need for a separate persistence layer. But the RDBMS is already the persistence layer, and SQL is fairly standardised. (I have seen projects where there is a database [DML], ORM [POCOs], Repository Pattern, Inversion of Control [Interfaces], Data Service - so 6 layers of "persistence layer". Less is more).
• When you define a VIEW in the RDBMS, it remains adjacent to the schema (and that version of the schema). This is a good thing.
• Version control for Views is the same as Version control for tables. If your ORM doesn't support migration of Views, get a better one, or don't use ORM migrations, but instead maintain your own master-script (eg: if not exists - add column to table; replace view).
• Just like any "reuse" scenario, another web server in the future can be written to leverage the same database and views. But also, another system on the same network may also integrate and leverage the same Views, and reporting engines can use the Views.
• The Web Service SHOULD be there as an adaptor only, so that the client application in the browser can query the database. (In practice, people like to also implement extra login inline with such RPCs - but that's another story)
• But what about mockability? The logic should be in the View - unit test that. If you want more integration testing, then by definition that includes the "database" too: have a testing environment complete with database.

With the above in mind, having SQL in code is worse:

• With an ORM, you can specify SQL and provide a POCO to materials; that way you do get type-safety. However, it's better to have a VIEW and tell the ORM to rebuild the Pocos for you. For a View, the ORM can see the schema and derive the correct POCO.
• If you want to slightly adjust the logic of one Web Service, if it's in code, you need to compile the code and upgrade. If it's monolithic, this is tedious and risky. If it's a microservice, this is still tedious and with dependencies still risky. However, if it's a View, you can easily deploy and be sure that it's only affecting just that View itself.
• Literal SQL in code is fine for development by the way. But prototyping it in a View should still be encouraged because it's so much easier to "test" that functionality in a query tool - that's right, squarely in the persistence layer

SQL in source code isn't an anti-pattern; that's a symptom of the actual anti-patterns:

• "Don't put it in the RDBMS" - this is just a distaste from younger coders that isn't rational. If you are a full-stack developer or backend-developer that includes the "database".
• "Do it all in code" - this is probably a result of the success of git. When a full-stack coder is making this kind of a decision, a coder will put it in code.