1
\$\begingroup\$

I've inherited an Access database and am looking to start using it to do reporting. I've written a query that gets me the information I'm looking for, I'm just hoping someone can have a look over it and tell me if I'm doing it the wrong way or not.

Anyway, the query takes information from 3 tables:

  • sites - more detailed information on each workplace
  • employees - list of employees and their associated workplace
  • cases - holds the bulk of the information needed for the report

I want most fields from the cases table as this is how work is tracked, but I want to also group by the city which is in the sites table and by employee name from the employees table.

SELECT 
 sites.city, 
 employees.[first name], 
 employees.[last name], 
 cases.[assigned to], 
 cases.ref, 
 cases.status, 
 cases.priority, 
 cases.[opened date], 
 Round(Now()-cases.[opened date],0) AS Age
FROM
 cases 
LEFT JOIN 
 (employees LEFT JOIN sites ON employees.site = sites.site) 
ON 
 cases.[assigned to] = employees.username
WHERE 
 cases.[assigned to]<>"Not Assigned" 
AND 
 cases.status="Active" 
AND 
 cases.priority<>"first contact"
ORDER BY 
 sites.city, 
 employees.[first name], 
 employees.[last name], 
 cases.[opened date] desc;

Is there any stupid rookie mistakes in here that I've fallen prey to? Could that be done any better? Have I not given enough information in my question?

200_success
146k22 gold badges190 silver badges479 bronze badges
asked Sep 11, 2016 at 11:49
\$\endgroup\$
0

2 Answers 2

2
\$\begingroup\$

I can't see any major flaw in this query.

I would just make the Jointures like this, as it's more clear :

SELECT 
 sites.city, 
 employees.[first name], 
 employees.[last name], 
 cases.[assigned to], 
 cases.ref, 
 cases.status, 
 cases.priority, 
 cases.[opened date], 
 Round(Now()-cases.[opened date],0) AS Age
FROM ((cases 
LEFT JOIN employees ON cases.[assigned to] = employees.username)
LEFT JOIN sites ON employees.site = sites.site)
WHERE cases.[assigned to]<>"Not Assigned" 
 AND cases.status="Active" 
 AND cases.priority<>"first contact"
ORDER BY 
 sites.city, 
 employees.[first name], 
 employees.[last name], 
 cases.[opened date] desc;

If you can't have any case with non existing employees and sites, it's better to change your 2 LEFT JOIN into INNER JOIN. Just throw the 2 queries and compare the results.

In your WHERE clause, having some <> is generally inefficient on large tables. It's always better to make a IN() and list all the possibilities excluding "Not assigned" or "first contact" :

  1. If cases.[assigned to] can only contain an employee username or "Not Assigned", then just doing an INNER JOIN on the employee table will automatically filter the not assigned and you don't need to specify it in the WHERE clause.
  2. if you have a limited number of case.priority values, list them all but "first contact" : AND cases.priority IN ("priority1", "super urgent", "can wait")

For the Age, if you want to compute the days from your open_date, it's better to do :

dateDiff("d",Now(),cases.[opened date]) AS Age

you can also concatenate the first and last name in one column:

 (employees.[last name] & ' ' & employees.[first name]) AS EmpName,

That's all what comes to my mind.

answered Sep 11, 2016 at 12:31
\$\endgroup\$
2
  • \$\begingroup\$ I appreciate the tips. I had always assumed a NOT clause would be less taxing because it only dealt with one value instead of several. I'll have to do more reading. \$\endgroup\$ Commented Sep 12, 2016 at 2:28
  • \$\begingroup\$ The different than operator is a perf killer for all RDBMS I know. It often results in full table scan \$\endgroup\$ Commented Sep 12, 2016 at 13:53
1
\$\begingroup\$

The nested joins are unconventional. A "flat chain" would be the more common and readable way to write it.

FROM cases 
 LEFT JOIN employees
 ON cases.[assigned to] = employees.username
 LEFT JOIN sites
 ON employees.site = sites.site

SQL string literal a should be written using single quotes. Double quotes may be accepted by MS Access (and MySQL), but they are nonstandard and could be interpreted differently in standard-compliant databases.

WHERE 
 cases.[assigned to] <> 'Not Assigned' AND 
 cases.status = 'Active' AND 
 cases.priority <> 'first contact'

You shouldn't be using 'Not Assigned' as a special value, though. It's bad practice to use values with special significance that might also be valid data — even if this special value is less likely to occur than 'Null' or 'XXXXXXX'. If there really isn't anyone assigned to the case, use a NULL to indicate that fact, and use WHERE cases.[assigned to] IS NOT NULL in the query.

answered Sep 12, 2016 at 1:07
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the feedback. I inherited the design where 'Not Assigned' was a value instead of null which does seem like the logical choice. I'll certainly be looking to change it in the future. \$\endgroup\$ Commented Sep 12, 2016 at 2:22

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.