88

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?

  • 31
    "Use Linq" and "Use stored procedures" are not reasons; they are just suggestions. Wait two weeks, and ask him for reasons. – Robert Harvey May 14 '17 at 15:04
  • 47
    The Stack Exchange network uses a micro-ORM called Dapper. I think it's reasonable to say that the vast majority of Dapper code is "hardcoded SQL" (more or less). So if it's a bad practice, then it's a bad practice adopted by one of the most prominent web applications on the planet. – Robert Harvey May 14 '17 at 16:28
  • 16
    To answer your question about repositories, hard-coded SQL is still hard-coded SQL no matter where you put it. The difference is that the repository gives you a place to encapsulate the hard-coded SQL. It's a layer of abstraction that hides the details of the SQL from the rest of the program. – Robert Harvey May 14 '17 at 16:29
  • 26
    No, SQL within code is not an anti-pattern. But that's an awful lot of boiler plate code for a simple SQL query. – GrandmasterB May 14 '17 at 21:53
  • 44
    There's a difference between 'in the source code' and 'spread all over the source code' – tofro May 15 '17 at 7:18

13 Answers 13

112

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.

  • 80
    The idea that "we can change the persistence technology" is unrealistic and unnecessary in the vast majority of real-world projects. A SQL/relational DB is a completely different beast than NoSQL DBs such as MongoDB. See this detailed article about it. At most, a project which started using NoSQL would eventually realize their mistake and then switch to an RDBMS. In my experience, allowing direct use an object-oriented Query Language (such as JPA-QL) from business code provides the best results. – Rogério May 16 '17 at 3:24
  • 6
    What if there is a very little chance the persistence layer is changed? What if there isn't a one to one mapping when changing the persistence layer? What is the advantage of the extra level of abstraction? – pllee May 16 '17 at 3:46
  • 19
    @Rogério Migrating from a NoSQL store to an RDBMS, or vice versa, is as you say probably not realistic (assuming the technology choice was appropriate to begin with). However, I have been involved on a number of real-world projects where we migrated from one RDBMS to another; in that scenario, an encapsulated persistence layer is definitely an advantage. – David May 16 '17 at 13:37
  • 6
    "The idea that "we can change the persistence technology" is unrealistic and unnecessary in the vast majority of real-world projects. " My company previously wrote code along that assumption. We had to refactor the entire codebase to support not one, but three entirely different storage/query systems and are considering a third and fourth. Worse, even when we had only one database, the lack of consolidation led to all kinds of code sins. – NPSF3000 May 17 '17 at 1:22
  • 3
    @Rogério You know the "vast majority of real-world projects"? In my company, most applications can work with one of many common DBMS, and this is very useful, as mostly our clients do have nonfunctional requirements regarding that. – BartoszKP May 17 '17 at 11:27
55

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.

  • 1
    I have edited the question. Not sure if it sheds any light. – w0051977 May 14 '17 at 18:00
  • 18
    @w0051977: the link you posted does not tell us anything about your application, right? It seems you are looking for some simple, braindead rule where to place SQL or not which fits to every application. There is none. This is a design decision you have to make about your individual application (probably together with your team). – Doc Brown May 14 '17 at 18:37
  • 7
    +1. In particular I would note that there is no real difference between SQL in a method, and SQL in a stored procedure, in terms of how "hard coded" something is, or how reusable, maintainable, etc. Any abstraction you can do with a procedure can be done with a method. Of course procedures can be useful, but a rote rule "we can't have SQL in methods, so let's put it all in procedures" will probably just produce a lot of cruft with little benefit. – user82096 May 15 '17 at 16:48
  • 1
    @user82096 I would say generally speaking putting db access code in SP's has been detrimental in nearly every project I have seen. (MS stack) Aside from actual performance degradations (rigid execution plan caches), maintenance of the system was made harder by having logic split around, and SPs maintained by a different set of people than the app logic developers. Especially on Azure where code changes between Deployment Slots is like seconds of work, but non code-first / db-first schema migrations between slots are a bit of an operational headache. – Sentinel Apr 11 '18 at 7:21
33

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 = 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.

  • 5
    I didn't state "vendor lock-in" is bad thing. I stated It comes with trade-offs. In today's market with db as xaaS such old contracts and licences for running selfhosted db are going to be more and more rare. It's quite easier today to change the db from vendor A to B compared how It was 10 years ago. If you read the comments, I'm un favour of taking advantages of any feature of the data storage. So It's easy to put some specific vendor details into something (app) meant to be vendor agnostic. We strive to follow SOLiD just to the end lock the code to a single data storage. Not a big deal – Laiv May 15 '17 at 6:26
  • 2
    This is a great answer. Often the right approach is the one which leads to the least bad situation if the worst possible code is written. The worst possible ORM code might be lots worse than the worst possible embedded SQL. – jwg May 15 '17 at 9:19
  • @Laiv good points PaaS is a bit of a game changer - I'll have to think more about the implications. – mcottle May 16 '17 at 0:23
  • 3
    @jwg I'd be inclined to say that above average ORM code is worse than below average embedded SQL :) – mcottle May 16 '17 at 0:27
  • @macottle Assuming that the below average embedded SQL was written by ppl above the average of the one who write a ORM. What I doubt It very much. Many devs out there are not capable to write left JOINs whitout checking out S.O first. – Laiv May 17 '17 at 3:48
14

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.

Clean Code by Robert C. Martin, pg. 288 (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:

SQL in a resource

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.

  • 13
    I think this focuses much too much on a bad criticism of having SQL in source code. Having two different languages in the same file is not necessarily terrible. It depends on a lot of things like how they are related and how they are each presented. – jwg May 15 '17 at 9:00
  • 9
    "mixing completely different languages with different syntax in the same file" This is a terrible argument. It would also apply to the Razor view engine, and HTML, CSS and Javascript. Love your graphic novels! – Ian Newson May 15 '17 at 12:54
  • 4
    This just pushes the complexity somewhere else. Yes, your original code is cleaner, at the expense of adding an indirection that the developer now has to navigate to for maintenance purposes. The real reason you would do this is if each SQL statement was used in multiple places in the code. In that way, it's really no different from any other ordinary refactoring ("Extract method"). – Robert Harvey May 15 '17 at 16:25
  • 3
    To be clearer, this isn't an answer to the question "what do I do with my SQL strings," so much as it's an answer to the question "what do I do with any collection of sufficiently complex strings," a question which your answer eloquently addresses. – Robert Harvey May 15 '17 at 16:36
  • 2
    @RobertHarvey: I am sure our experiences differ, but in my own I have found the abstraction in question to be a win in most cases. I will surely refine my abstraction tradeoffs as I have done up and down for years, and may one day put SQL back in code. YMMV. :) I think microservices can be great, and they too generally separate your SQL details (if SQL is used at all) from core application code. I am less advocating "use my specific method:resources" and more advocating "Generally don't mix oil and water languages." – Charles Burns May 15 '17 at 19:29
13

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.

  • 2 as written essentially assumes a read only database. Stored procedures used to be not uncommon for queries that modify. – jpmc26 May 16 '17 at 0:32
  • @jpmc26: I cover writes in the parenthetical... did you have a specific recommendation for improving this answer? – Kevin May 16 '17 at 0:45
  • Hm. Apologies for commenting before completing the answer and then getting distracted. But updatable views make it rather difficult to track down where a table is being updated. Restricting updates to directly on the tables, regardless of mechanism, does have its advantages in terms of understanding the code logic. If you're constraining the logic to the DB, then, you can't enforce that without stored procedures. They also have the advantage of allowing multiple operations with a single call. Bottom line: I don't think you should be quite so hard on them. – jpmc26 May 16 '17 at 1:12
  • @jpmc26: That's fair. – Kevin May 16 '17 at 1:16
8

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.

  • 1
    Thanks. +1 for "Hardcoding SQL statements may lead you to coupling tightly your application to the db enginee.". I wander if he was referring to SQL Server rather than SQL i.e. using SQLConnection rather than dbConnection; SQLCommand rather than dbCommand and SQLDataReader rather than dbDataReader. – w0051977 May 14 '17 at 21:35
  • No. I was refering to the usage of expressions non-ANSI compliant. For instance: select GETDATE(), .... GETDATE() is SqlServer's date function. In other enginees, the function has different name, different expression, ... – Laiv May 15 '17 at 3:46
  • 14
    "Vendor lock-in"... How often do people/enterprises actually migrate their existing product's database to another RDBMS? Even in projects using exclusively ORM, I never ever saw that happen: usually the software is rewritten from scratch as well, because the needs have changed in between. Therefore, thinking about this seems pretty much like "premature optimization" to me. Yes, it's totally subjective. – Olivier Grégoire May 15 '17 at 16:01
  • 1
    I don't care how often happens. I do care It may happens. In greenfield projects, the cost of implementing a DAL abstracted from the db is almost 0. A possible migration is not. The arguments against ORM are quite weak. ORMs are not like 15 years ago when Hibernate was quite green. What I have seen is a lot of "by default" configurations and quite more bad data model designs. It's like many other tools, many people do the "getting started" tutorial and they don't care what happens under the hood. The tool is not the problema. The problem is at who doesn't know how and when to use it. – Laiv May 15 '17 at 17:54
  • On the other hand, vendor locked-in is not necessarily bad. If I were asked I would say I don't like. However, I'm already locked to Spring, Tomcat or JBoss or Websphere (still worse). Those I can avoid, I do. Those I can't I just live with It. It's matter of preferences. – Laiv May 15 '17 at 18:00
3

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.

  • Allowing DBAs to change your SQL without updating the application is effectively splitting your application into two separately developed, separately deployed entities. You now need to manage the interface contract between those, synchronize the release of interdependent changes, etc. This may be a good thing or a bad thing, but it's a lot more than "putting all your SQL in one place", it's a far-reaching architectural decision. – IMSoP Sep 20 '17 at 17:29
2

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";
  • 7
    The most glaring problem with this is the issue of where that value 42 comes from. Are you concatenating that value into the string? If so, where did it come from? How has it been scrubbed to mitigate SQL injection attacks? Why doesn't this example show a parameterized query? – Craig May 15 '17 at 17:19
  • I just meant to show the formatting. Sorry, I forgot to parameterize it. Now fixed after referring to msdn.microsoft.com/library/bb738521(v=vs.100).aspx – rleir May 16 '17 at 6:49
  • +1 for importance of formatting, though I maintain that SQL strings are a code smell regardless of formatting. – Charles Burns May 17 '17 at 16:45
  • 1
    Do you insist on the use of an ORM? When an ORM is not in use, for a smaller project, I use a 'shim' class containing embedded SQL. – rleir May 21 '17 at 5:18
  • SQL strings are most definitely not a code smell. Without an ORM, as a manager and architect I would encourage them over SPs for both maintenance and sometimes performance reasons. – Sentinel Apr 12 '18 at 6:17
1

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 ...

  • To be clear: Using prepared statements does not prevent SQL injection. Using prepared statements with bound variables does. It is possible to use prepared statements that are built from untrusted data. – Andy Lester May 18 '17 at 18:13
1

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.

  • +1 Good point. Supportability is more important than most other considerations so don't be too clever :) That said, if you're an ORM shop don't forget that there are instances where ORM is not the answer and you do need to write direct SQL. Don't optimize prematurely but there will be times in any non-trivial application when you will have to go down this route. – mcottle Sep 20 '17 at 3:55
0

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.

0

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

  • It may be you just using shorthand but it's a really bad idea to blindly use "SELECT *" because you will bring back fields you don't need (bandwidth!!) Also although I don't have a problem with the conditional Admin clause it leads down a very slippery slope to a conditional "WHERE" clause and that's a road that leads straight to performance hell. – mcottle Sep 18 '17 at 8:33
0

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.

  • Wrong........... – Sentinel Apr 11 '18 at 6:01
  • Do you think that your comment is helpful to anybody? – bikeman868 Apr 11 '18 at 6:03
  • This is just plain incorrect. Let's say we have a Web API2 project with EF as the data access and Azure Sql as the storage. No, let's get rid of EF and use handcrafted SQL.The db schema is stored in an SSDT SQL Server project.Db schema migration involves making sure the codebase is synched to the db schema and fully tested, before pushing out the change to a test db in Azure.A code change involves pushing out to an App Service deployment slot, which takes 2 seconds. Plus handcrafted SQL have performance plusses over stored procedures, generally speaking, despite the 'wisdom' to the contrary. – Sentinel Apr 11 '18 at 7:16
  • In your case deployment might take seconds, but this is not the case for many people. I did not say that stored procedures are better, just that they have some advantages under some circumstances. My final statement is that if these circumstances don’t apply then putting SQL in the application is very valid. How does this contradict what you are saying? – bikeman868 Apr 11 '18 at 7:23
  • Stored procedures are hand crafted SQL??? – bikeman868 Apr 12 '18 at 5:48

protected by gnat May 15 '17 at 15:39

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.