33
You have a lot of if statements in your SQL. This screams at me that you are doing something in your SQL that should be done in the application, and that this SQL should be separated into two distinct stored procedures.
The benefits of doing this:
Faster stored procedures
Use a coding language instead of a query language to do the logic where it was ...
33
SQL Injection alarm
Don't use user passed text in your sql statements if you aren't using parameters.
From: https://stackoverflow.com/a/7505842/2655508
Using parameters helps prevent SQL Injection attacks when the database is used in conjunction with a program interface such as a desktop program or web site.
In your example, a user can directly ...
30
In the case of both if your while loops, you could easily make them for loops instead, which tends to be the best practice.
int i = 1;
while (i <= 100000)
{
// main while code
i++
}
Should be rewritten as:
for (int i = 1; i <= 100000; i++)
{
// main while code
}
Likewise:
i = 1;
while (i <= 1000)
{...
27
The shorter query I can think of is
SELECT COALESCE(MAX(Data), 'Default Value')
FROM Configuration
WHERE Name = 'NameOfConfiguration';
If the configuration is present it'll take it.
If the configuration is not present the MAX will generate a NULL value (the MAX of nothing is NULL) that will be coalesced to the default value
There is a more generic ...
18
Cleartext passwords
Not a code-specific issue exactly but it would appear that you are storing the user's passwords in clear text. This means that anyone gaining access to the LOGIN_Tab would be handed a list of valid usernames and passwords for your application. This is especially bad when combined with SQL injection vulnerabilities!
Consider instead ...
17
This seems extra complexity with no purpose.
You take any type variable and automatically convert it to a parameter (this is good).
But then something strange happens, you look at the type of the variable and convert that to a string so you can call a function named after the type to do a standard set of options that only change based on the type.
Why ...
16
You could use the SET NOEXEC statement, which controls whether the subsequent statements are executed (OFF) or not (ON). Use it in conjunction with your EXISTS test like this:
IF NOT EXISTS (SELECT * FROM sys.assemblies WHERE name = 'SQL_CLR_Functions')
SET NOEXEC ON
GO
CREATE FUNCTION dbo.CLR_CharList_Split(@list nvarchar(MAX), @delim nchar(1) = N',')
...
16
There are a few things that are off in this query. Going through them in some sense of order:
I am still a fan of 'old style' joins, but, the CTE concept is a real winner in SQL Server, so, instead of creating the table variable, just use the CTE.
I can tell that your code went through some iterations, and as a result, you have some 'cruft' that can be ...
16
Code Review Stack Exchange advice
It really helps if you explain, in plain English, what your code does or is supposed to do. As it stands, nobody has any idea what your code is supposed to do, and the best we can do is guess. With that in mind...
Nitpicks
Your aliases make no sense. Your table names are cryptic enough (you probably can't change that) but ...
16
A few things, some nitpicks, some UX, some probably-bugs. What Lyle's Mug has already stated is also part of my answer, but first things first:
UX
This query is quite unwieldy to use, because all the things you might want to play with are hardcoded.
SEDE allows using parameters, with a rather simple syntax:
##name:type[?if optional, default value]##
...
16
In addition to Heslacher's answer I would also look at the using statement.
It is very important to keep a handle on resources like Sql Connections to ensure they are closed and cleaned up.
Link to the MSDN about SqlConnection with an example with the using statement.
A SqlConnection object represents a unique session to a SQL Server
data source. ...
15
I am unfortunately not able to test your code, but I am able to read your code. Very well. Your code is overall very well written and you seem to adhere to most of the Delphi coding conventions that I know of. (Yes, non-Delphi users, using F for field name, T for a type, and a few letters before each enum constant is Delphi conventions)
It's been a while ...
15
Documentation
I think that what the query is doing is looking at your reputation rank, then rounding up to the nearest 50. It would be nice to hit "edit description" and document that for the benefit of anyone else using Stack Exchange Data Explorer.
Generality
Data Explorer allows parameterization. Use it.
Style
You shouldn't have to declare ...
14
The use of the Tags field on the Post is a poor choice for the query. It relies on a table scan, which is slow, because it has to check each question. Note, you can use the < and > characters to identify tag start and end values in the Tags column, but, as I say, don't use that column.
Instead, you should do a join with the PostTags and Tags tables. ...
14
SqlConnection implements the IDisposable interface, and so does the SqlDataAdapter class - any type that implements IDisposable should have its Dispose method called; by disposing the connection you don't need to explicitly close it.
The best way to ensure Dispose gets called is to wrap the object in a using block, which is basically compiler magic for a ...
13
The query feels tortured because SQL is designed to work with sets of data. You're using SQL like an ordinary programming language, trying to turn tables into scalars, and embedding special values in the query rather than storing it as data.
I recommend that you create and populate a ConfigurationDefaults table that is analogous to your Configuration table....
13
I find this pretty nicely done and really hard to pick on.
But here's a little something.
Instead of writing this condition this way:
WHERE Posts.CreationDate <= @today
AND Posts.CreationDate > @target_week
This is shorter and simpler:
WHERE Posts.CreationDate BETWEEN @target_week AND @today
Keep in mind though that's not exactly the same,
...
13
If you write the query where you first order by rank and then by age, you will get a better result, and you should be able to get rid of part of the where statement, which I would think would make the query a little faster.
SELECT TOP 150
ROW_NUMBER() OVER (ORDER BY Users.Reputation DESC) AS Rank,
Users.Id as [User Link],
Users.Age,
Users.Reputation ...
13
Your query is well structured, and consistent, but there is one issue you have failed to incorporate.... not all bad questions have a user. When questions are migrated, or there's a user deleted, the question may have no link back to the Users table.
This requires an Outer Join to Users.
Additionally, I have found that CTE expressions are fantastic for ...
13
There are two problems with this that are immediately apparent:
You are executing arbitrary SQL. This is a SQL Injection flaw. You'll want to use parameterized queries.
You are catching an exception and doing nothing with it. Malformed SQL, integrity constraint violations, or database connectivity issues will be swallowed, causing failures in your ...
12
I think overall your code is good! Nothing stands out as being particularly inefficient.
Nitpicking
I find your keyword formatting to be inconsistent: I see declare and Select and DATEDIFF etc. While it is true that SQL is not case-sensitive, it is a good practice to be consistent. Just pick your favorite one and stick to it.
Old-style join
From Posts ...
12
Naming
Constants in .NET should be named using PascalCase. See c-sharp-naming-convention-for-constants
Your const string selectSatement = @"INSERT INTO Payment..." should be better named InsertStatement to also reflect that it is an INSERT and not a SELECT.
var first = 0;
var payInfo = list[first];
As the value of var first won't be changed, you ...
12
Security issues are covered, so just a bit on the code itself.
You have three nearly identical pieces of code, inside a switch-statement. That seems to be a code-smell. Apart from that, I'm missing a default case. What happens if a user logs in before you have assigned a role? He gets an empty screen, or an ugly error message?
How about creating a(n ...
12
Not utterly wrong, but
q.PostTypeId = 1
and
AND a.PostTypeId = 2
are superfluous. The only post types with a non-null value in ParentId are answers (see the Database schema documentation), and their values are the ids of questions, so the join already takes care of this.
I'd slightly misunderstood the objective of the query at first, because questions ...
11
What you've got is pretty readable, but there are a few points to make:
Your sub-query is written with a different indentation style from the main query.
I prefer to see the keywords in upper-case.
Personally, I dislike spaces before commas intensely (and you aren't 100% consistent about adding them). However, I loathe even more the style where the comma ...
11
I have looked at this query/report, and from the beginning I figured it must be missing something. I looked through the SQL, and can't identify it off-hand, so I figured I would build my own query, and see how they compare. The results I got are very different... :(
Edit: Found the problem
You cannot chain two outer joins.... Consider the query for the ...
11
AdoConverter
For better extensibility, the methods in that class shouldn't be calling each others the way ToLongParameter is calling ToIntegerParameter. Also instead of hard-coding the type
Private Type TypeMappings
BooleanMap As ADODB.DataTypeEnum
ByteMap As ADODB.DataTypeEnum
CurrencyMap As ADODB.DataTypeEnum
DateMap As ADODB.DataTypeEnum
...
11
Logical correctness
c is not c++; java is not javascript. Using CHARINDEX(…, Q.Tags) to inspect a comma-delimited string is a bad idea. Join against the PostTags table instead.
Keep in mind that this query only gives the time to first answer for questions that received an answer. Questions that never received an answer have an infinite response time, in ...
11
-- Number of weeks must not go into the future, hence the following:
SET @weeks_ago = (CASE WHEN @weeks_ago >0 THEN -@weeks_ago ELSE @weeks_ago END);
This boils down to taking the absolute value of the given parameter, so it could be simplified to:
SET @weeks_ago = abs(@weeks_ago)
I would expect the query to return 0 rows for invalid parameter values ...
10
Going through this code again, it appears there are a few things that could be improved.
Use #Temp Table for User Selection
Instead of doing a cheap-shot UNION select to get the set of users to process, the right way would be to create a temp table, and then conditionally populate it:
create table #TopUsers (
UserId int not null,
Reputation int ...
Only top voted, non community-wiki answers of a minimum length are eligible
