30

Proliferation of tables Database schemas should not be designed such that you need to create more tables to accommodate more data. The same principle applies here. Instead of separate tables Fizz, Buzz, Woof, Ping, and Plop, you should have a single NoiseDefs table. A classic unparameterized FizzBuzz should be much simpler… WITH NoiseDefs(multiple, ...


18

I think your scripts could use a bit more vertical whitespace; maybe it's the uppercasing, but I find this: BEGIN SET SEARCH_PATH = PhrancisFizzBuzz; -- call populate function with input parameters PERFORM fncPopulateFizzBuzz( prmMaxNumber, prmFizz, prmBuzz, prmWoof, prmPing, prmPlop ); DROP TABLE IF EXISTS tmpFizzBuzzResults; CREATE ...


12

It is a maxim in computer science that there are really "only three numbers": zero, one, and many. You have a case of many, and you should treat it as such. Otherwise, someday a customer might want to buy a hundred of some food item, and performance will be poor. Therefore, I recommend… Option four Instead of having the table represent current supply ...


10

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


8

Great answer by @Malachi. Let's talk about performance. Variables You have lots of variables and this can slow it down. I see at least two that could be completely eliminated: SET @Now = getdate() SET @Today = DATEADD(dd, 0, DATEDIFF(dd, 0, @Now)) There is not really a point in setting a variable as a place holder for a built-in SQL function. You could ...


8

You started out with all your keywords being capitalized and then you abandoned your capitalization haphazardly throughout the script, this makes it a little hard to read because your brain says that keywords are capitalized and then all of a sudden you see a begin and your brain says, "hold up a second, no that's a keyword, never mind" make sure that ...


7

Did not use EF, stored procedures already available and VS2008 is limited to EF 3/4. A quick Google search for "SqlDatabase class" yields an outdated MSDN page about an obscure class in an obscure Microsoft.Practices.EnterpriseLibrary.Data.Sql namespace: Retired Content This content is outdated and is no longer being maintained. It is provided as a ...


7

Sometimes, comments aren't the clearest way to make a script readable: IF ( (--is it a monday? SELECT DATEPART(dw,@now) ) = 2 Consider: declare @MONDAY int; set @MONDAY = 2; if (datepart(weekday,@now) = @MONDAY) Does one need a comment to figure out what's going on? I hope not! This part however: DECLARE @now DATETIME = ...


7

Does this open the door to destructive injection attacks? It depends, but potentially, yes it can, especially with dynamic SQL, which you should try to avoid unless it is absolutely necessary. You should be able to sanitize the input before passing it to the database server, how to do it really depends on what technologies you are using. NOTE: DO NOT TRY ...


6

These are really the only local variables that you need SET @lSortCol = LTRIM(RTRIM(@SortColumn)) SET @lFirstRec = ( @lPageNbr - 1 ) * @lPageSize SET @lLastRec = ( @lPageNbr * @lPageSize + 1 ) SET @lTotalRows = @lFirstRec - @lLastRec + 1 and you don't really need some of them SET @SortColumn = LTRIM(RTRIM(@SortColumn)) SET @FirstRec = (@PageNo - 1) * @...


6

General Remarks First off, you want to make this a dynamic SQL statement, or use OPTION(RECOMPILE): SQL Server has issues with the null-or-provided-parameter pattern (that is, stuff like ((@atLeast IS NULL) OR (d.[inCount] >= @atLeast))). I also feel a disconnect about your parameters - you're provided the @from and @to, but you're also using the ...


6

I am building a store. When a user decides to make a purchase I need to store it in the database but since the purchase might be of more than 1 item I'm wondering which way is the best to insert multiple rows, here are my 3 options. Who said you had to insert multiple rows? If I buy 10 apples, that's one row: user_id = 42, food_id = 103, quantity = 10. ...


6

Problem Statement Tiered Architecture This database is the first part of the project, I couldn’t really create a friendly user interface until the database is working. Due to feature creep this database can now handle other kinds of books besides science fiction and fantasy. The most important step when taking on a project as you have, is deciding ...


5

Don't do this: catch (Exception ex) { throw ex; } From MSDN Once an exception is thrown, part of the information it carries is the stack trace ... If an exception is re-thrown by specifying the exception in the throw statement, the stack trace is restarted at the current method and the list of method calls between the original method that ...


5

This is first refactored version of your code. I have changed following thing If any object provide dispose method , prefer using statement. Use dictionary rather than list of parameter as you have a possibility of specifying param names also Current implementation sticks only to datatable , you might need to think about the datareader. naming of variable ...


5

I may be pointing out the obvious here, but the ValidateEntity Method returns a string. I would expect it to return a Boolean. There are a number of one and two letter variable names. We've not been restricted in the size of names for many years now. It would be good to give them more descriptive names and we can take advantage of C#'s case sensitivity. ...


5

Create PROCEDURE [dbo].[spChanger] AS BEGIN Execute ('UPDATE [TblActions] SET Complete = 2 WHERE DateToComplete < Date.Now.AddDays(14) AND Complete = 3' ) END Your casing is inconsistent. If you prefer UPPERCASE keywords, stick to uppercase :) CREATE PROCEDURE [dbo].[spChanger] AS BEGIN EXECUTE ('UPDATE [TblActions] SET Complete = 2 WHERE ...


5

Aside from Malachi's remarks about capitalization of keywords and indentation of the code, I would add two points about variable naming: DON'T use variable names that start with @@, as they are reserved for T-SQL functions. This would now give a conflict between the parameter @DiscludeTables and the private table variable @@DiscludeTables, but I would ...


5

The DbConnection class implements the IDbConnection interface, so unless you need something exposed by the Component base class, you could use the interface instead of the concrete type. This will make your code more flexible and testable. If you're using C#6, you can use the nameof operator to refactor proof your argument exceptions. ... throw new ...


5

Overall, I think your procedure is very good. It's consistent in layout, capitalisation and style, and well commented. I had no problems reading through it and understanding what it was doing (and how) without actually running it. I really like the way you distinguish global constants with @ALL_CAPS, from regular "true" @variables. I'd not seen this before ...


5

Considering that your subselect is basically the same as your main select, chances are that this subselect is indeed a bit of overkill. Looking at your logic, what you want to achieve is to get the Price of the SalesOrderDetail linked to the SalesOrderHeader with the most recent OrderDate, so let's use an ORDER BY DESC and select only the first answer. I ...


4

I'm afraid you current solution is a prime example of inappropriate use of PL/SQL when plain SQL provides much more elegant solution: create table t1 ( id number, status varchar2(10) ); create table t2 ( id number ); create table t3 ( id number ); insert into t1 values(1,'A'); insert into t2 values(2); insert into t3 values(3); insert into t1 ...


4

I am not sure there is a way to reduce the number of if-statements and nesting. What is possible though is to improve the performance of the conditions you have that check those conditions. The count(*) is inefficient. Consider adding a 'ROWNUM = 1' condition to your where clause in a way that makes the following conditions possible: SELECT COUNT(...


4

Be consistent with your naming and Capitalization of Keywords. Some places you write null and others you write NULL, some places Declare other places DECLARE. Personally I write them all in uppercase, it's a pain, but is so much easier to read in my opinion, especially when it is in plain text and not in an IDE. You do some weird things with variables ...


4

It's possible to omit the {} in the using statements, much like with an if statement. If you're not doing anything until an inner block you can just write using (bla1 = ...) using (bla2 = sth(bla1)) using (bl3 = sth(bla2)) { //Do the real work here } //Stuff is cleaned up here as if you'd done lots of {} It's a matter of style, mainly. Since it isn't an if ...


4

Regardless of the purpose of the query, I see that it contains a lot of joins involving subselects. Therefore the query would benefit greatly from the use of Common Table Expressions.


4

Alright, here is my take on it: CREATE PROCEDURE dbo.usp_AssignPermissionsToRole ( @OldRoleName VARCHAR(50), @NewRoleName VARCHAR(50), @Database VARCHAR(50) ) AS BEGIN DECLARE @DataSQL NVARCHAR(MAX); DECLARE @PermissionSQL NVARCHAR(MAX) = ''; DECLARE @Start INT = 1; DECLARE @Total INT; DECLARE @PermissionState VARCHAR(60); ...


3

There are a couple of things slowing you down. Two Left Outer Joins and a Full Outer Join. There may not be much you can do about this unless you can trim down the fields that need to be returned. Start with trying to remove the Full Outer Join. It will have worse performance than the Left Outer Joins. Scanning an entire field will also be a bit on the ...


3

You haven't clearly shown why you need a function/procedure at all, but I'll assume it's a set requirement - i.e. someone said "Thou Shalt Use Procedures As A Database API". (Edit after update): A function is no different, in terms of transaction processing, to simply: BEGIN; INSERT ....; INSERT ....; INSERT ....; COMMIT; If there are race conditions ...


3

Your initialize and DeInitialize do the job of connecting and disconnecting your SQL Connection, but not the SQL Command that you create in each method. I am sure that closing and disposing the connection will take care of the Command but I like to make sure. so what I would do is to use a using statement initialize(); using (SqlCommand cmd = new ...


Only top voted, non community-wiki answers of a minimum length are eligible