23
\$\begingroup\$

Some fellow code reviewers (hi @Janos!) have been inquiring about a SEDE query to allow to check progress of the Red Shirt "hat" progression.

Try it here!


Background

red-shirt-hat

Red Shirt

cast 5 downvotes on posts that are later deleted or closed


Limitations

There are certain Stack Exchange limitations which make querying this information a bit tricky. Namely:

  • The data is only refreshed once a week, on Sundays. This makes it impossible to have "real time" results.

  • User voting activity is anonymous, in that a user can only see their own voting activity in their own profile. This disallows joining voting and user data on SEDE.

Assumptions

I have made certain assumptions, based on the trends I generally see on closed questions. They are:

  • A user who votes to close/delete a bad question will usually also downvote the question.

  • A user who downvotes a question will usually do so before voting to close/delete.

Usage

To get usefulness out of this (and as indicated in the SQL comments at the top of the query):

The way that this report can be used is by comparing the results set side-by-side with your "votes" under your activity reports. Filter by down-votes and look to see if questions you down-voted are in the result set below.

For example:

red-shirt-query

Query

/* 
Winter Bash 2014
"Red Shirt" hat estimation
"Cast 5 downvotes on posts that are later deleted or closed"

The way that this report can be used is by comparing the results set 
side-by-side with your "votes" under your activity reports.
Filter by down-votes and look to see if questions you down-voted are
in the result set below.
*/

-- 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##;
-- 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);

DECLARE @target_week DATETIME;
SET @target_week = DATEADD(WEEK, @weeks_ago, CURRENT_TIMESTAMP);

-- POST-RELATED VARIABLES

DECLARE @downvote TINYINT;
SET @downvote = (SELECT Id FROM VoteTypes WHERE Name LIKE 'Down%'); --3

DECLARE @question_post TINYINT;
SET @question_post = (SELECT Id FROM PostTypes WHERE Name = 'Question'); --1

DECLARE @closed_post TINYINT;
SET @closed_post = (SELECT Id FROM PostHistoryTypes WHERE Name = 'Post Closed'); --10

DECLARE @deleted_post TINYINT;
SET @closed_post = (SELECT Id FROM PostHistoryTypes WHERE Name = 'Post Deleted'); --12

-- QUERY BEGINS

WITH cte_downvoted_posts AS
(
  SELECT Votes.PostId AS dvote
  FROM Votes
  INNER JOIN Posts ON Votes.PostId = Posts.Id
  WHERE VoteTypeId = @downvote
)

SELECT 
    Posts.Id AS [Post Link]               -- magic column
  , Posts.OwnerUserId AS [User Link]      -- magic column
  , Posts.CreationDate AS [Creation Date]
  , Posts.ClosedDate AS [Closed Date]

FROM Posts
INNER JOIN cte_downvoted_posts
    ON Posts.Id = cte_downvoted_posts.dvote
INNER JOIN PostHistory
    ON Posts.Id = PostHistory.PostId

WHERE Posts.CreationDate <= @today
  AND Posts.CreationDate > @target_week
  AND Posts.PostTypeId = @question_post
  AND Posts.ClosedDate IS NOT NULL

GROUP BY
    Posts.Id
  , Posts.OwnerUserId
  , Posts.CreationDate
  , Posts.ClosedDate

ORDER BY
    Posts.CreationDate DESC

Concerns

  • Nitpicks are fine! Anything from naming to indentation to inconsistencies, please don't be shy to point out anything at all!

  • I noticed a lot of nested loops in the execution plan, when it's doing joins. Is there a better way to do this to avoid them?

  • Is there a way to make this query result set more useful, or user-friendly?

  • Are my comments appropriate/useful? Should I have fewer, or more of them?

\$\endgroup\$
6
\$\begingroup\$
  • You don't need to join to Posts at all in your CTE since you don't use any of its columns and you get a "Post must exist" requirement from your main query.
  • Along the same lines, you don't use PostHistory at all in your query but join to it all the same.
  • You declare @deleted_post but you never use it; you just set @closed_post twice (presumably incorrectly the second time to the ID of the "Deleted Post" row.)
  • But then again, you don't use @closed_post or @deleted_post in your query, so why are you getting those exactly?
  • Some None-1-2 testing would reveal if SE ever adds a new votetype beginning with "Down" your query will fail as you assume your @downvote variable will only be assigned a single scalar value. So either add TOP 1 to your query or change your WHERE clause to an = operator to prevent this. (Also, since you're only searching for the "DownMod" votetype, why the LIKE operator in the first place?)

I removed the CTE entirely and changed your Main Query to:

SELECT 
    Posts.Id AS [Post Link]               -- magic column
  , Posts.OwnerUserId AS [User Link]      -- magic column
  , Posts.CreationDate AS [Creation Date]
  , Posts.ClosedDate AS [Closed Date]

FROM Posts
INNER JOIN Votes
    ON Posts.Id = Votes.PostId

WHERE Posts.CreationDate <= @today
  AND Posts.CreationDate > @target_week
  AND Posts.PostTypeId = @question_post
  AND Posts.ClosedDate IS NOT NULL
  AND VoteTypeId = @downvote

GROUP BY
    Posts.Id
  , Posts.OwnerUserId
  , Posts.CreationDate
  , Posts.ClosedDate

ORDER BY
    Posts.CreationDate DESC

Personally, I would just call your Top 1 Subquery to get the DownMod VotetypeID and Question PostTypeID directly in the query, but that's just religion.

\$\endgroup\$
  • \$\begingroup\$ I did notice I left in some variables I had previously intended to use, from an older versiom which did not turn out very well. Nice catch and nice review! \$\endgroup\$ – Phrancis Dec 20 '14 at 15:57
13
\$\begingroup\$

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, because if I remember correctly, BETWEEN uses <= and >= conditions. But even if so, it might not make a difference anyway in the case of this query. (I confirmed with 1 week to go back, the results are identical.)

Btw, @Malachi pointed out an excellent article about range queries and their dangers.

A very minor nitpick, but in this column alias I would spell out the "dvote" nicely as "downvote":

  SELECT Votes.PostId AS dvote
\$\endgroup\$
  • \$\begingroup\$ those two where statements are not the same. between is a little weird, it acts like a WHERE x < Date AND x > otherDate and doesn't include the start and end date. \$\endgroup\$ – Malachi Dec 19 '14 at 20:22
  • \$\begingroup\$ there is a couple of posts on StackOverflow. give me a second. looks like this answer is saying something a little different than what I am saying, what do you think? stackoverflow.com/a/16347680/1214743 \$\endgroup\$ – Malachi Dec 19 '14 at 20:25
  • 1
    \$\begingroup\$ Aaron Bertrand is pretty well known on SO his blog talks about Dates and SQL Server pretty thoroughly. sqlblog.com/blogs/aaron_bertrand/archive/2009/10/16/… \$\endgroup\$ – Malachi Dec 19 '14 at 20:31
11
\$\begingroup\$
-- 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 though.

\$\endgroup\$
  • 2
    \$\begingroup\$ I've never heard of ABS() that's a good idea! \$\endgroup\$ – Phrancis Dec 19 '14 at 20:30
10
\$\begingroup\$

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##;
  1. The comment about the number of weeks should be much closer to the declaration.
  2. A better variable name would eliminate the need for this variable entirely.
SELECT 
    Posts.Id AS [Post Link]               -- magic column
  , Posts.OwnerUserId AS [User Link]      -- magic column

Okay, so it's a magic column? What does the magic column do? (For those that don't know, this makes special columns display as a hyperlink.)

\$\endgroup\$
  • 1
    \$\begingroup\$ The magic-column thing is sede specific though... Anyone using sede probably has stumbled upon them a few times.. \$\endgroup\$ – Vogel612 Dec 19 '14 at 20:52
  • \$\begingroup\$ Maybe @Vogel612. Maybe not. What's that do? \$\endgroup\$ – RubberDuck Dec 19 '14 at 20:54
7
\$\begingroup\$

Common Table Expressions are for all, intents and purposes, a Table and should be named accordingly,

WITH cte_downvoted_posts AS

Should be

WITH DownvotedPosts AS

Drop the Hungarian Naming style and go with Pascal for big name items like Tables and Columns.

All objects in the Database should be Pascal (In my opinion)

\$\endgroup\$
  • \$\begingroup\$ Fair enough.. but CTE is technically not a database object.. \$\endgroup\$ – Mathieu Guindon Dec 19 '14 at 20:13
  • 2
    \$\begingroup\$ how technical are we talking here? it's a Temporary Table that is destroyed after the transaction. @Mat'sMug \$\endgroup\$ – Malachi Dec 19 '14 at 20:16
5
\$\begingroup\$

This is more of a feature request than a proper code review, but it might be nice to allow the user to pick particular topics to either filter or sort.

Also, a possibly quicker way to check against one's own history would be to simply click on the question link. If you downvoted it, the downvote button on that question will be highlighted.

\$\endgroup\$
  • \$\begingroup\$ Good idea, perhaps adding tags as options would be good! \$\endgroup\$ – Phrancis Dec 19 '14 at 20:28
  • \$\begingroup\$ I've hacked this this query to order by the first tag. \$\endgroup\$ – Edward Dec 19 '14 at 20:42

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.