forked from tgstation/tgstation
-
Notifications
You must be signed in to change notification settings - Fork 4
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Contribution reorganization: Theft Edition (tgstation#61451)
A while back oranges made this really nice pr. tgstation#59465 It was mostly done to make snake_case mandatory for pretty much everything, along with a few other style things. But he got fed up with our 800 line contrib file, and decided to split it up. I share a similar hatred for our guidelines, people don't read them and finding anything is just guesswork People will just throw their work at the bottom or top of a file with no regard for any order. It's hell to read and work with. So I "borrowed legally under agpl v3" ~~stole~~ his code and expanded on it. I think? he was working with the intent of splitting the main tower file into several different subfiles. I don't think I have the right vision to do that properly, but I've done what I can. Splits contributing.md's Specifications section into 2 files: - Style.md, which describes formatting patterns and clean code. - Standards.md, this lays out the rules we have for code. More function, less cleanliness. Each file is separated into different sections. They cover things like styling for procs, variables, paths, or how to structure your code properly, optimization pitfalls to avoid, etc. I've also created a development guides section in the contributing guidelines, to include links the above alongside things like autodoc, hard deletes, and the policy config section. Makes a subfolder of .github called /guides to hold files like this, and the ones that exist to teach people how to set up servers. ## Why It's Good For The Game I got bored, and MSO took away my keys, so I can't fix harddels right now. Plus if I did my job right this will make learning from and adding to what we've written down much easier.
- Loading branch information
1 parent
ec2d24d
commit 10edd4f
Showing
10 changed files
with
742 additions
and
702 deletions.
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,376 @@ | ||
# Style Guide | ||
This is the style you must follow when writing code. It's important to note that large parts of the codebase do not consistently follow these rules, but this does not free you of the requirement to follow them. | ||
|
||
1. [General Guidelines](#general-guidelines) | ||
2. [Paths and Inheritence](#paths-and-inheritence) | ||
3. [Variables](#variables) | ||
4. [Procs](#procs) | ||
5. [Things that do not matter](#things-that-do-not-matter) | ||
|
||
## General Guidelines | ||
|
||
### Tabs, not spaces | ||
You must use tabs to indent your code, NOT SPACES. | ||
|
||
Do not use tabs/spaces for indentation in the middle of a code line. Not only is this inconsistent because the size of a tab is undefined, but it means that, should the line you're aligning to change size at all, we have to adjust a ton of other code. Plus, it often time hurts readability. | ||
|
||
```dm | ||
// Bad | ||
#define SPECIES_MOTH "moth" | ||
#define SPECIES_LIZARDMAN "lizardman" | ||
#define SPECIES_FELINID "felinid" | ||
// Good | ||
#define SPECIES_MOTH "moth" | ||
#define SPECIES_LIZARDMAN "lizardman" | ||
#define SPECIES_FELINID "felinid" | ||
``` | ||
|
||
### Control statements | ||
(if, while, for, etc) | ||
|
||
* No control statement may contain code on the same line as the statement (`if (blah) return`) | ||
* All control statements comparing a variable to a number should use the formula of `thing` `operator` `number`, not the reverse (eg: `if (count <= 10)` not `if (10 >= count)`) | ||
|
||
### Operators | ||
#### Spacing | ||
* Operators that should be separated by spaces | ||
* Boolean and logic operators like &&, || <, >, ==, etc (but not !) | ||
* Bitwise AND & | ||
* Argument separator operators like , (and ; when used in a forloop) | ||
* Assignment operators like = or += or the like | ||
* Operators that should not be separated by spaces | ||
* Bitwise OR | | ||
* Access operators like . and : | ||
* Parentheses () | ||
* logical not ! | ||
|
||
Math operators like +, -, /, *, etc are up in the air, just choose which version looks more readable. | ||
|
||
#### Use | ||
* Bitwise AND - '&' | ||
* Should be written as `variable & CONSTANT` NEVER `CONSTANT & variable`. Both are valid, but the latter is confusing and nonstandard. | ||
* Associated lists declarations must have their key value quoted if it's a string | ||
* WRONG: `list(a = "b")` | ||
* RIGHT: `list("a" = "b")` | ||
|
||
### Use static instead of global | ||
DM has a var keyword, called global. This var keyword is for vars inside of types. For instance: | ||
|
||
```DM | ||
/mob | ||
var/global/thing = TRUE | ||
``` | ||
This does NOT mean that you can access it everywhere like a global var. Instead, it means that that var will only exist once for all instances of its type, in this case that var will only exist once for all mobs - it's shared across everything in its type. (Much more like the keyword `static` in other languages like PHP/C++/C#/Java) | ||
|
||
Isn't that confusing? | ||
|
||
There is also an undocumented keyword called `static` that has the same behaviour as global but more correctly describes BYOND's behaviour. Therefore, we always use static instead of global where we need it, as it reduces suprise when reading BYOND code. | ||
|
||
### Use early returns | ||
Do not enclose a proc in an if-block when returning on a condition is more feasible | ||
This is bad: | ||
````DM | ||
/datum/datum1/proc/proc1() | ||
if (thing1) | ||
if (!thing2) | ||
if (thing3 == 30) | ||
do stuff | ||
```` | ||
This is good: | ||
````DM | ||
/datum/datum1/proc/proc1() | ||
if (!thing1) | ||
return | ||
if (thing2) | ||
return | ||
if (thing3 != 30) | ||
return | ||
do stuff | ||
```` | ||
This prevents nesting levels from getting deeper then they need to be. | ||
|
||
### No magic numbers or strings | ||
This means stuff like having a "mode" variable for an object set to "1" or "2" with no clear indicator of what that means. Make these #defines with a name that more clearly states what it's for. For instance: | ||
````DM | ||
/datum/proc/do_the_thing(thing_to_do) | ||
switch(thing_to_do) | ||
if(1) | ||
(...) | ||
if(2) | ||
(...) | ||
```` | ||
There's no indication of what "1" and "2" mean! Instead, you'd do something like this: | ||
````DM | ||
#define DO_THE_THING_REALLY_HARD 1 | ||
#define DO_THE_THING_EFFICIENTLY 2 | ||
/datum/proc/do_the_thing(thing_to_do) | ||
switch(thing_to_do) | ||
if(DO_THE_THING_REALLY_HARD) | ||
(...) | ||
if(DO_THE_THING_EFFICIENTLY) | ||
(...) | ||
```` | ||
This is clearer and enhances readability of your code! Get used to doing it! | ||
|
||
### Use our time defines | ||
|
||
The codebase contains some defines which will automatically multiply a number by the correct amount to get a number in deciseconds. Using these is preffered over using a literal amount in deciseconds. | ||
|
||
The defines are as follows: | ||
* SECONDS | ||
* MINUTES | ||
* HOURS | ||
|
||
This is bad: | ||
````DM | ||
/datum/datum1/proc/proc1() | ||
if(do_after(mob, 15)) | ||
mob.dothing() | ||
```` | ||
|
||
This is good: | ||
````DM | ||
/datum/datum1/proc/proc1() | ||
if(do_after(mob, 1.5 SECONDS)) | ||
mob.dothing() | ||
```` | ||
|
||
## Paths and Inheritence | ||
### All BYOND paths must contain the full path | ||
(i.e. absolute pathing) | ||
|
||
DM will allow you nest almost any type keyword into a block, such as: | ||
|
||
```DM | ||
// Not our style! | ||
datum | ||
datum1 | ||
var | ||
varname1 = 1 | ||
varname2 | ||
static | ||
varname3 | ||
varname4 | ||
proc | ||
proc1() | ||
code | ||
proc2() | ||
code | ||
datum2 | ||
varname1 = 0 | ||
proc | ||
proc3() | ||
code | ||
proc2() | ||
. = ..() | ||
code | ||
``` | ||
|
||
The use of this is not allowed in this project as it makes finding definitions via full text searching next to impossible. The only exception is the variables of an object may be nested to the object, but must not nest further. | ||
|
||
The previous code made compliant: | ||
|
||
```DM | ||
// Matches /tg/station style. | ||
/datum/datum1 | ||
var/varname1 | ||
var/varname2 | ||
var/static/varname3 | ||
var/static/varname4 | ||
/datum/datum1/proc/proc1() | ||
code | ||
/datum/datum1/proc/proc2() | ||
code | ||
/datum/datum1/datum2 | ||
varname1 = 0 | ||
/datum/datum1/datum2/proc/proc3() | ||
code | ||
/datum/datum1/datum2/proc2() | ||
. = ..() | ||
code | ||
``` | ||
|
||
### Type paths must begin with a `/` | ||
eg: `/datum/thing`, not `datum/thing` | ||
|
||
### Type paths must be snake case | ||
eg: `/datum/blue_bird`, not `/datum/BLUEBIRD` or `/datum/BlueBird` or `/datum/Bluebird` or `/datum/blueBird` | ||
|
||
### Datum type paths must began with "datum" | ||
In DM, this is optional, but omitting it makes finding definitions harder. | ||
|
||
## Variables | ||
|
||
### Use `var/name` format when declaring variables | ||
While DM allows other ways of declaring variables, this one should be used for consistency. | ||
|
||
### 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](./STANDARDS.md#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) | ||
``` | ||
|
||
## Procs | ||
|
||
### Getters and setters | ||
|
||
* Avoid getter procs. They are useful tools in languages with that properly enforce variable privacy and encapsulation, but DM is not one of them. The upfront cost in proc overhead is met with no benefits, and it may tempt to develop worse code. | ||
|
||
This is bad: | ||
```DM | ||
/datum/datum1/proc/simple_getter() | ||
return gotten_variable | ||
``` | ||
Prefer to either access the variable directly or use a macro/define. | ||
|
||
|
||
* Make usage of variables or traits, set up through condition setters, for a more maintainable alternative to compex and redefined getters. | ||
|
||
These are bad: | ||
```DM | ||
/datum/datum1/proc/complex_getter() | ||
return condition ? VALUE_A : VALUE_B | ||
/datum/datum1/child_datum/complex_getter() | ||
return condition ? VALUE_C : VALUE_D | ||
``` | ||
|
||
This is good: | ||
```DM | ||
/datum/datum1 | ||
var/getter_turned_into_variable | ||
/datum/datum1/proc/set_condition(new_value) | ||
if(condition == new_value) | ||
return | ||
condition = new_value | ||
on_condition_change() | ||
/datum/datum1/proc/on_condition_change() | ||
getter_turned_into_variable = condition ? VALUE_A : VALUE_B | ||
/datum/datum1/child_datum/on_condition_change() | ||
getter_turned_into_variable = condition ? VALUE_C : VALUE_D | ||
``` | ||
|
||
### When passing vars through New() or Initialize()'s arguments, use src.var | ||
Using src.var + naming the arguments the same as the var is the most readable and intuitive way to pass arguments into a new instance's vars. The main benefit is that you do not need to give arguments odd names with prefixes and suffixes that are easily forgotten in `new()` when sending named args. | ||
|
||
This is very bad: | ||
```DM | ||
/atom/thing | ||
var/is_red | ||
/atom/thing/Initialize(mapload, enable_red) | ||
is_red = enable_red | ||
/proc/make_red_thing() | ||
new /atom/thing(null, enable_red = TRUE) | ||
``` | ||
|
||
Future coders using this code will have to remember two differently named variables which are near-synonyms of eachother. One of them is only used in Initialize for one line. | ||
|
||
This is bad: | ||
```DM | ||
/atom/thing | ||
var/is_red | ||
/atom/thing/Initialize(mapload, _is_red) | ||
is_red = _is_red | ||
/proc/make_red_thing() | ||
new /atom/thing(null, _is_red = TRUE) | ||
``` | ||
|
||
`_is_red` is being used to set `is_red` and yet means a random '_' needs to be appended to the front of the arg, same as all other args like this. | ||
|
||
This is good: | ||
```DM | ||
/atom/thing | ||
var/is_red | ||
/atom/thing/Initialize(mapload, is_red) | ||
src.is_red = is_red | ||
/proc/make_red_thing() | ||
new /atom/thing(null, is_red = TRUE) | ||
``` | ||
|
||
Setting `is_red` in args is simple, and directly names the variable the argument sets. | ||
|
||
## Things that do not matter | ||
The following coding styles are not only not enforced at all, but are generally frowned upon to change for little to no reason: | ||
|
||
* English/British spelling on var/proc names | ||
* Color/Colour - both are fine, but keep in mind that BYOND uses `color` as a base variable | ||
* Spaces after control statements | ||
* `if()` and `if ()` - nobody cares! |
Oops, something went wrong.