Write clear rules on descriptive and obvious names in the contribution guidelines (#56872)

This has more or less been enforced already by me and Rohesie, but puts it into clear writing, as the frequency of these names stays about the same.

Formally bans variables like M, C, and H. Writes an exclusion for `i`, as though I don't like this one, I believe other maintainers disagree. Let me know if I'm misinformed.

Also adds some additional details on negated names, such as `is_not_flying` -> `is_flying`.
This commit is contained in:
Mothblocks
2021-02-13 10:48:59 -05:00
committed by GitHub
parent 8daeb19f04
commit 43200a3167
+72
View File
@@ -463,6 +463,78 @@ When adding new signals to root level procs, eg;
```
The `SHOULD_CALL_PARENT(TRUE)` lint should be added to ensure that overrides/child procs call the parent chain and ensure the signal is sent.
### Use descriptive and obvious names
Optimize for readability, not writability. While it is certainly easier to write `M` than `victim`, it will cause issues down the line for other developers to figure out what exactly your code is doing, even if you think the variable's purpose is obvious.
#### Don't use abbreviations
Avoid variables like C, M, and H. Prefer names like "user", "victim", "weapon", etc.
```dm
// What is M? The user? The target?
// What is A? The target? The item?
/proc/use_item(mob/M, atom/A)
// Much better!
/proc/use_item(mob/user, atom/target)
```
Unless it is otherwise obvious, try to avoid just extending variables like "C" to "carbon"--this is slightly more helpful, but does not describe the *context* of the use of the variable.
#### Naming things when typecasting
When typecasting, keep your names descriptive:
```dm
var/mob/living/living_target = target
var/mob/living/carbon/carbon_target = living_target
```
Of course, if you have a variable name that better describes the situation when typecasting, feel free to use it.
Note that it's okay, semantically, to use the same variable name as the type, e.g.:
```dm
var/atom/atom
var/client/client
var/mob/mob
```
Your editor may highlight the variable names, but BYOND, and we, accept these as variable names:
```dm
// This functions properly!
var/client/client = CLIENT_FROM_VAR(usr)
// vvv this may be highlighted, but it's fine!
client << browse(...)
```
#### Name things as directly as possible
`was_called` is better than `has_been_called`. `notify` is better than `do_notification`.
#### Avoid negative variable names
`is_flying` is better than `is_not_flying`. `late` is better than `not_on_time`.
This prevents double-negatives (such as `if (!is_not_flying)` which can make complex checks more difficult to parse.
#### Exceptions to variable names
Exceptions can be made in the case of inheriting existing procs, as it makes it so you can use named parameters, but *new* variable names must follow these standards. It is also welcome, and encouraged, to refactor existing procs to use clearer variable names.
Naming numeral iterator variables `i` is also allowed, but do remember to [Avoid unnecessary type checks and obscuring nulls in lists](#avoid-unnecessary-type-checks-and-obscuring-nulls-in-lists), and making more descriptive variables is always encouraged.
```dm
// Bad
for (var/datum/reagent/R as anything in reagents)
// Good
for (var/datum/reagent/deadly_reagent as anything in reagents)
// Allowed, but still has the potential to not be clear. What does `i` refer to?
for (var/i in 1 to 12)
// Better
for (var/month in 1 to 12)
// Bad, only use `i` for numeral loops
for (var/i in reagents)
```
### Other Notes
* Code should be modular where possible; if you are working on a new addition, then strongly consider putting it in its own file unless it makes sense to put it with similar ones (i.e. a new tool would go in the "tools.dm" file)