10
\$\begingroup\$

I made this query to create a graph of a user's popular questions and the view count on that question. It allows for a minimum of 500 views, and a score of 3.

DECLARE @allowed_min_views INT = 500;
DECLARE @allowed_min_score INT = 3;

DECLARE @user_id INT = ##UserId:int?-1##;
DECLARE @min_views INT = ##MinimumViews:int?500##;
DECLARE @min_score INT = ##MinimumScore:int?3##;
DECLARE @question INT = 1;

IF (@min_views < @allowed_min_views)
BEGIN
  PRINT '@MinimumViews must be larger than 499.'
END

IF (@min_score < @allowed_min_score)
BEGIN
  PRINT '@MinimumScore must be larger than 2.'
END

IF (@min_views >= @allowed_min_views AND @min_score >= @allowed_min_score)
BEGIN
  SELECT
      ViewCount
    , Score
    FROM Posts WHERE
      PostTypeId = @question
      AND OwnerUserId = @user_id
      AND ViewCount >= @min_views
      AND Score >= @min_score
    ORDER BY ViewCount ASC;
END

Finally, here's some sample input (I'm using @Mat'sMug's user ID):

@user_id:   23788
@min_views: 500
@min_score: 7
\$\endgroup\$
8
\$\begingroup\$

Good things

You use good local variables, and you are consistent with your naming. The typical naming for T-SQL is using PascalCase, however there are no standards and snake_case or camelCase work just as good, as long as you are consistent (which you are).

You validate your values, although I am not quite sure why you chose 500 and 3 as arbitrary minimums (might be worth documenting).


Results are not very useful...

As written, your query returns this:

ViewCount Score --------- ----- 571 10 629 5 685 6 721 10 728 11 761 12 840 25 849 7 870 17 888 9 1065 10 ...

Which is all well and good, except, it doesn't give much information. Let's say we rewrite it a bit like this:

IF (@min_views >= @allowed_min_views AND @min_score >= @allowed_min_score)
BEGIN
  SELECT
      ViewCount
    , Score
    , [User Link] = @user_id
    , [Post Link] = Id
    , CreationDate
    , [Tags] = Tags
    FROM Posts 
    WHERE
      PostTypeId = @question
      AND OwnerUserId = @user_id
      AND ViewCount >= @min_views
      AND Score >= @min_score
    ORDER BY ViewCount DESC;
END

Notice I changed ORDER BY to DESC, I think it makes more sense to show highest first.

Then we get a more sensible result set, e.g.:

enter image description here

\$\endgroup\$
  • \$\begingroup\$ I used ASC because it made more sense to have lower views first on a graph. \$\endgroup\$ – Ethan Bierlein Jul 13 '15 at 0:25
  • \$\begingroup\$ Ah OK. I didn't see a graph generated by your query so I didn't know \$\endgroup\$ – Phrancis Jul 13 '15 at 0:45
5
\$\begingroup\$
DECLARE @allowed_min_views INT = 500;
DECLARE @allowed_min_score INT = 3;

This feels very user unfriendly. What is the reasoning for this limit?


SELECT
    ViewCount
  , Score
  FROM Posts WHERE
    PostTypeId = @question
    AND OwnerUserId = @user_id
    AND ViewCount >= @min_views
    AND Score >= @min_score
  ORDER BY ViewCount ASC;

Most queries don't indent the FROM any farther than the SELECT and put the WHERE in its own line as well. Otherwise, this looks good to me.

\$\endgroup\$
2
\$\begingroup\$

I like symmetry. It's a bit off-putting to see PascalCase symbols (the column names in the SE tables) and snake_case symbols (your variables) mixed together. It would be better to adjust to the given style, and use PascalCase for your variables.

The variable @question is a bit misleading. It's not a "question", not even a "question id", it's actually the id of the question type. So @QuestionTypeId would be a more natural name.

Finally, the requirement of a user id is not very user friendly. Everybody knows @Mat's Mug but I doubt many people know that he also goes by 23788. Sure, it's not too hard to look that up, but it still takes a few clicks per user. It would be better to allow lookups by either user name or user id.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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