Pull Request Review Checklist

[Sirryan2002]

Review Checklist

Here are things that PR Reviewers should go over on each PR, these should all be fixed before merging. Feel free to use this as a baseline for reviewing PRs.

The full checklist

• Does the Pull Request have an acceptable and descriptive title?
• Is the Pull Request template used in the PR description?
• Confirm that the pull request is not in a Work in Progress (WIP) state, if it is, it needs to be properly set to WIP or closed
• If the PR has player-facing changes, does it have a correctly formatted and filled-out change log?
• Has the Pull Request passed all tests? If not, are the tests reporting the issues correctly?
• Are all significant changes documented and (if needed) screenshotted in the PR description?
• Does the code follow our cleanliness standards? Are things properly capitalized and spaces? Are defines being used? Are things named well?
• Is the code readable? Is indentation well managed and the code logic easy to follow? Is reusable code abstracted out? Are variables and procs documented?
• If new documentation is added (or modified), does it follow our AutoDoc format?
• Is the code organized? Do you know if it's defined in the right place and are new files being properly put under the correct directory?
• Is the code performant and efficient? Is anything being redundantly called and would there be better ways to iterate through things?
• Are there safety checks put in place to prevent unnecessary runtimes? Are there proper guard clauses where needed?
• Are Initialize(), New(), and Destroy() being correctly used?
• If new references are being added, are they using good Garbage Collection principles?
• If there has been icon changes, check the IconDiff results to ensure everything is proper
• If there have been mapping changes, check the MapDiffBot2 to make sure there are no stray edits
• Has the author TESTED their PR and reported the result? If not… MAKE THEM. Any PR that goes beyond changing values/parameters should have each individual new/changed feature or mechanic tested.

Extremely Detailed Checklist

Unsure about some of those code standard check marks? Here's very specific things to look for.
Code Cleanliness Standards

• Ensuring src is not being used to access methods and variables
• Ensuring that new proc and variable names in atom subtypes use snake_case
• Ensuring that proper spacing is enforced
• No spaces between if, else if, for, while declarations and their conditions
• Newlines between proc and type definitions
• Spaces between math operators
• Space between comments and code... - Ideal format => do_thing() // Comment
• Where 1s and 0s are being used as booleans, they are correctly converted to TRUE and FALSE defines
• #define’s that are only used in the file they are defined in should be #undef’d at the end of the file
• Is native BYOND syntax being used in a way that is acceptable under CONTRIBUTING.md?

Code Readability Standards

• Guard clauses are in place to prevent over-indented code
• Code logic flow is being handled intelligently with switch, if, else, if else, and while statements
• Code logic statements are not multiline, instead they’re broken up into multiple readable and clearly distinct statements
• Little to no use of undefined magic numbers; if there’s not a clear indication of what a number is then it is put into a well-named #define
• New variables have good descriptive names and are accompanied with an autodoc comment if the purpose/relevance of the variable is not immediately clear from its name; Additionally, variable names should not conflict with native BYOND syntax or common temporary var names.
• Each new proc should have a name that indicates what the proc actually does
• Each proc is limited to a reasonable line count; Overly large procs or procs with a lot of reused code should be abstracted out into more small procs
• Common istype() statements should be converted into is_helpers

Code Organization Standards

• Defines that are used over multiple files need to be put in a proper global defines file under the _DEFINES directory
• Global variable definitions should be done in a file under the _GLOBALS directory, especially if it will potentially be used over several files
• No more additions to unsorted.dm
• General rule of thumb: if someone codes a helper that isn’t specific to any type or system then one should double check that it doesn’t already exist

Code Efficiency and Performance Standards

• Avoid performing the same check multiple times in a row without a good reason
• We should not null check variables right before running istype() generally
• Iterations over large areas or massive lists are optimized or limited in use

Runtime Prevention/Tracking

• Public/External procs should have null checking enforced for parameters
• Non-critical Internal procs should generally not have a lot of safety checks and instead should be allowed to runtime when bad parameters are passed or properly implement stack_trace() calls that give detailed error reports
• Atom references passed as parameters to code contained in timer procs / after sleep() should be QDELETED() checked.
• Unsafe method accessing with the : operator should never be done

Initialize(), New(), and Destroy() Good Practices

• Initialize() must always call parent at the beginning of the proc
• Initialize() must always return either the return value of the parent proc or one of the defined initialize defines
• New() should rarely be used for Atoms, it should only ever be used as a constructor, for SDMM tricks (such as layering/coloring to make map editing easier), or to perform critical tasks that must be done before initialization.

Garbage Collection and Destroy()

• Destroy() must always call and return parent at the end of the proc unless it returns one of the QDEL_HINTs, if it uses them then there must be an option to force qdel which overrides the QDEL_HINT
• Any hard references to other atoms should be cleared in Destroy()

Testing Best Practices

General

• ALWAYS compile before committing
• A PR is not a merge-ready state until it is run on a local test server and the round is STARTED
• Any mechanic / feature that is modified should be individually tested to ensure that it still works properly
• If modified, Icon states need to be checked to make sure they're working