Reviewing Pull Requests - official-antistasi-community/A3-Antistasi GitHub Wiki
The style of reviewing code is very different per person.
However, here is an example for getting started:
I take 7 skims (quickly reading and not paying much attention) over the code looking for these things:
* [ ] Indentation & Headers
* [ ] Naming Conventions & Grammar
* [ ] Macros
* [ ] Param & Call
* [ ] Global Variables
* [ ] Local Variables
* [ ] Logic (Will take the longest)
- I have lined paper or an Excel spreadsheet to take notes.
- Coffee or Tea is a good review parter.
- This only applies to new or rewritten functions.
- Indentation must be in spaces. 4 spaces is normal. However, if there are a lot of indentation (Barbolani's
if-else
chaines), then less spaces are acceptable. I use Visual Studio Code to Check-Out the Pull-Request, then I search for the tab character so it will be highlighted. - Headers must attempt the standard header https://github.com/official-antistasi-community/A3-Antistasi/wiki/Standardised-Header.
Not all of the header information is necessary. The most important lines are:
• Author
(original author and people who might of helped him/her)
• Arguments
&Return Value
(Does not need to be as specific as Standardised-Variable-Types
. For example just <ARRAY>
would be fine. All arguments in param
must be listed here.)
• Environment
(This is important, if it uses sleep
it cannot be run from the debug console)
- People should not name their variables
_a
,_69
,_someUnrelatedName
,_bbv
,_bbc
. The variable name should describe what it holds. - Grammar is not important, and only matters when text is displayed to the player, using
A3A_fnc_customHint
,systemChat
ect.
- Macros are not common. When they are used, just check that they are correct and will not cause problems.
- Double check that the arguments for
call
match theparam
in the function. Check that the function always returns the expected type.
- All global variables created must be prefixed.
banList
will not be accepted, it must beA3A_banList
. - Check that global variables are always created before they are used.
- Tell the author if there are possible race-conditions. Usually when clients modify arrays and then broadcast with
publicVariable
orsetVariable [name,value,true]
- All must be declare with
private
. - Check that a local variable is not used in a
spawn
as it will not be defined in that scope.
- No syntax errors.
- No undefined edge-cases.
- Does not need to be 100% optimised. But it cannot be slow when there is a better way to code a function. (Check nested
findIf
,forEach
,forFrom
,multiplenearObject
ect.) - Make sure everthing works as expected.