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 run SQL code ...
31
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)
{...
29
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
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
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 ...
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 ...
14
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,
...
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
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 ...
13
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 ...
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 Q, ...
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
-- 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 ...
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 ...
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
A couple of things to think about while writing your query....
Variable Naming
ALTER PROCEDURE [stimulator].[GetLastMessages2]
@serviceId bigint,
@stimulatorId int,
@from datetime,
@to datetime,
@atLeast int = null,
@lessThan int = null,
@lastHourLessThan int = null,
@hourFactor int = 24,
@lastXHourLessThan int = null,
...
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
Stylistically I'd declare variables just like in program code: Each on a single line.
Also your nested Select seems a little forced and cramped, as well as the calculation code. You maybe want to add a little whitespace there ;)
Well now now, I actually wouldn't want you to get a Mug, but I'd want one myself...
slightly jesting
How many mugs would have to ...
11
Comments
-- NumberWeeks: Number of weeks to go back
-- DATETIME VARIABLES
DECLARE @today DATETIME;
SET @today = CURRENT_TIMESTAMP;
DECLARE @weeks_ago INT;
SET @weeks_ago = ##NumberWeeks:int?4##;
The comment about the number of weeks should be much closer to the declaration.
A better variable name would eliminate the need for this variable entirely.
...
Only top voted, non community-wiki answers of a minimum length are eligible
