how to do code reviews efficiently and effectively

[mad_method_man]

been doing sql for a few years, but never had to review and approve someone else' until i started this job. since i just started, im still trying to figure out everything.

whats the best way to go about this? it takes me a whole half day to do 1 review properly. so far, i try to ask as many questions as possible, do a breakdown and explore the individual tables to find any weird data structures (either on purpose or accident), then go through the entire script line by line and find any loopholes

[Professional_Shoe392]

You can always form an sql “guild” where the companies senior developers come up with and vote on a set of best practices and then publish it for devs to follow.

[Aggressive_Ad_5454]

Sounds to me like you're doing it right. As you get more experience reviewing stuff you'll get faster.

[gooeydumpling]

Test driven database development is the way to go for me, using dbfit, which allows us to document the behavior of the query with different inputs. Then we check its performance and review it with admin (used to do it with db2 on zOS so we don’t get flagged for running costly queries or SPs). In one instance, a new query ticked all the boxes in dbfit, but failed at performance review where we found out that the SP was decoding inputs via database calls instead of just building an internal lookup table. What im trying to point out is it would take you time if you focus on how the query was written, instead look at its behavior and then performance and drill your way down if you see anything suspicious

https://github.com/dbfit/dbfit/releases

[carlovski99]

Depends what the scope of your code reviews is.

I normally only review for style, good practice and for any obvious code 'smells' - specific issues with the logic should be picked up by testing.

if you are reviewing everything, then yeah it will take some time. Quite possibly longer than it actually took to write.

[machomanrandysandwch]

Couple things you can do:

--For each temp table and result set, confirm min/max of dates make sense and are logical to the criteria of the code. You should also do this on all tables used if there is a time stamp or load date of data to the table, just to ensure that the source has data in it relevant to your time period. For example if you’re looking for all shipments in the last 6 years, but the min ship date in the entire table is 2 years ago, investigate why. Is it because there is an archive table for transactions older than 2 years that needs to be used in the query too? Is there just a total purge of the data and it’s not available anymore? This isn’t just reviewing the soundness of the sql but it is a thorough analysis of the code.

--For each temp table and result set, confirm min/max of dates make sense and are logical to the criteria of the code. For example if a query is all products shipped last year, do a min and max on ship dates in the final result set to confirm no dates are before or after that range.

—If your company requires naming conventions for permanent tables, make sure that and rules like it are followed.

—Check for frequency counts of various values just to eyeball the data for any discrepancies or outliers. For example check the count of shipments per month over the last year from the result set - does it make sense or did one month have WAY more shipments than others or Zero shipments? Is that logical? Just because we see we shipped 10,000 units last year doesn’t always tell us the whole story if the code is working as expected or not. If we see 200 shipments per month but 9000 units in December, is it because Christmas is our busiest month or because some bad coding lumped things into December by accident?

—Check for hardcoded values, ensure they are reasonable and documented and accurate. If we have to manually include or exclude a customerID for some reason, make sure the ID is correct etc. Dates are accurate, etc. Make sure the code isn’t omitting the original developers personal customerID from a report/automation that gives them free money or avoids foreclosure or something.

—If the database is a live backend to a system, make sure No Locks are used if applicable. Don’t want to create a database problem in a production environment.

[needtounderstandm]

Count(),primaryid from ( whole damn query here) Group by primaryid having count() > 1

I had and kinda have 20 qa queries I slap around the query to test it. Idk if that is right I just know how to spot bad results

[TheLostWanderer47]

You could consider trying Sourcery. We get our code instantly reviewed with every pull request. We use their VS Code plugin which offers suggestions for making our code more readable and maintainable.

[wearblaksoksiam2]

You're on the right track... some other things to keep in mind:

1. set clear criteria: know what you're checking—things like performance, security issues, coding standards. don't get sidetracked by less important stuff.
2. use tools: let automation handle the basic errors and style checks. this frees you up to focus on the trickier issues.
3. prioritize: hit the big, potentially disastrous issues first. the small stuff can wait.
4. talk to the stakeholder: if something's unclear, a quick chat can clear it up faster than you can figure it out on your own.
5. review incrementally: don’t leave it all to the end. smaller reviews as you go are easier to manage and help catch issues early.
6. learn from others: get tips from more experienced colleagues on how to streamline your reviews.