Hot answers tagged

43

I coded keeping SOLID principle in mind and here is my code. What did I miss? Let's see... SRP - Single Responsibility Principle A class should have only one reason to change. Violated. The name EmployeeLeave suggests it's a class that just stores some data about an employee-leave but its API says something else - I'm a repository. So what is it? ...


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

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


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


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


27

Let's look for some patterns in your code: First of all, we notice that you have three parameters with similar names. condition, condition2 and condition3. These names indicates a code smell, that we should use an array or list instead. Taking one step at a time, we see this code is repeated multiple times: if (StringUtils.isNotEmpty(condition.toString()))...


22

Technically, the full outer join is insufficient. If during a certain week there was not a single user who posted their first question or answer, the plot for that week needs to be 0. It would be misleading to plot a direct line from the previous week to the following week, skipping the quiet week. So, strictly speaking, you need to generate a series of ...


20

Do not store the connection information in a file unencrypted Assuming that you are connecting to a database somewhere on the internet... If you plan on distributing this application, do not store the connection information unencrypted in a file on the user's device. In fact, preferably don't store the database connection information at all. Do not let your ...


20

If this is actually your first SQL you're quite good in learning new languages. Some remarks: The Group By is not weird, it's the SUM, you apply a conditional aggregation but the result is the same as a simple COUNT(*) (you can always get the same result using different ways in SQL). The ORDER BY 'Downvotes' DESC will not work as expected, because single ...


19

Your query is simultaneously awful and awesome. It is truly impressive, especially if you are self-taught. Unfortunately, I don't believe it is correct. (Or, I may have misunderstood how you intended for the vote redistribution scheme to work.) For example, it reports that the redistribution in favour of Candidate 2 should be +0.125. Of the 8 ballots ...


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


18

There's a common credo believed when evaluating the security of software: If it's homemade, it's unlikely secure. Sec.SE has a Q&A about homemade algorithms, which is somewhat germane to your circumstance. I suggest you look into preparing your queries, as that would be your best action to take in this situation. It's essentially what you're trying to ...


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

public class EmployeeLeave : ILeaveRequest ... var leaveRequest = new EmployeeLeaveDetail(); This is confusing. Going by name, ILeaveRequest implies that the class implementing that interface is a "leave request" (I'm aware that classes don't have an "is a" relationship with interfaces). But then we see an actual leave request in the form of an ...


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

More important than the performance, your code is very likely vulnerable to SQL Injection. (I can't say for sure without looking at your PHP code) If $viewingUserId or $userId is chosen by the user in any way, then you have a problem. And even if it is not, I would highly recommend using prepared queries. I don't know if you're using the mysql_* methods or ...


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

That may sound a bit harsh, but have you ever heard of the for loop ? More on the SQL side of your code : SELECT * is BAD ! And that's really obvious in your code : the only field that you use is aanswer but you fetch all your fields. You may say that you only have two fields so it's not that important. Well in that case, this might be true. But what in 1 ...


14

The way to write that query is: SELECT * FROM member_copy WHERE id IN (17579, 17580, 17582); However, the real question is, where did that list of ids come from? If the list of ids is the result of another database query, then you should be doing either a subselect or a join instead.


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

'2011-Jan-01' --There was very little activity prior to this date. Including it skews the graph. And then '2011-Jan-01' --There was very little activity prior to this date. Including it skews the graph. If that wasn't Copy+Paste'd, you've given your fingers a lot of work here. And if it was... well then, you've given yourself more work than you should ...


14

One by one, then: Is it good that I stored the posts in tables, or is it unnecessary? It is unnecessary, in this case. What your table variables are doing for you here really is simulating a self-referencing join. You could instead use aliases on the actual table(s), for instance: FROM Posts AS "Answers" JOIN Posts AS "Questions" ON "Answers"....


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

catch (Exception) { throw; } Well this won't buy you anything but a few more lines of code. Just remove the try..catch at all. This construct just let the exception bubble up the call stack like it would if you wouldn't have the try..catch at all. public void ProcessLeaveRequest You don't validate all of the given method arguments. The reason ...


14

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


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