Code style guidelines - VanaDev/Vana GitHub Wiki

There should not be more than one consecutive blank line.

Functions do not have any newlines before code or after the last line of code.

No lines should have any trailing whitespace - including blank lines.

There should be a copyright header at the top of each file that can be claimed for copyright.

Tabs are used for indentation, not spaces.

TODO FIXME is the token I use to convey something that needs to be done.

There is no hard or soft character limit, but try as much as possible to keep lines terse and readable. Usually, long lines can be broken up. In the case where a long line is broken up, continuation with a single tab is the preferred method of indentation. For example:

 Item f = GameLogicUtilities::isEquip(itemId) ?
 	Item{ChannelServer::getInstance().getEquipDataProvider(),
 		itemId,
 		Items::StatVariance::Normal,
 		player != nullptr && player->hasGmBenefits()} :
 	Item{itemId, amount};

There should not be a newline at the end of the file.

Binary operators should have spaces surrounding both sides.

 arg1 = arg2;
 arg1 = arg2 + arg3;
 arg1 = arg2 .. arg3;

All Lua files use the .lua extension.

Copyright header uses --[[ --]] block commenting style.

The local keyword for variables is used in most /scripts/utils/ Lua files unless the content is specifically designed to be exposed.

Typically, constants or things that should not be modified use underscore_naming_convention. Things that may be modified tend to use camelCaseNamingConvention. Function names use camelCaseNamingConvention.

Semicolons are used after all statements even though this is not required by Lua.

Double quote characters are used for strings.

If conditionals will not have wrapping parens unless absolutely necessary to ensure order of operations.

Function calls have no space between function name and opening paren.

Function arguments on both definition and usage side only have a space before consecutive arguments. For example, there is no space between open paren and arg1 or arg1 and the comma, but there is a space before arg2. There is no space between arg and the closing paren.

 function abc(arg1, arg2) end
 abc(1, 2);

Function definitions:

 function abc(arg1, arg2)
 	-- code
 end
 createAnonymousFunction(function()
 	-- code
 end);

Multiple assignment is fine and encouraged in some cases.

 arg1, arg2, arg3 = data[1], data[2], data[3];

Spaces are optional inside arrays, just ensure that spacing is consistent. This may change in the future. Both of these are currently valid:

 arg1 = { 1, 2, 3 }; -- However, this style is used very infrequently compared with the other style
 arg1 = {1, 2, 3};

When using the addText Lua export, please try to keep sentences and calls 1:1. For example:

 addText("Sigh ... ");
 addText("I'll definitely hear it from my boss if I don't make the norm today ... ");
 addText("Oh well, that sucks.");
 sendNext();

All C++ implementation files use the .cpp extension. All C++ interface files use the .hpp extension.

Copyright header uses /* */ block commenting style. There should be no newline between copyright header and file content.

#pragma once is the preferred #include guard. It goes immediately below the copyright header.

In .hpp files, there should be one newline before the #include block. In all files, there should be one newline after.

#include directives in .hpp files go in the following order: Common project includes first, project includes next (e.g. LoginServer), standard headers (e.g. <string>). Library code such as Asio is considered to be in the category of standard headers (e.g. <asio.h>).

#include directives in .cpp files go in the same order with one exception: The header that belongs to the .cpp file. This is specified first and has no path prefix.

Alphabetical order should be observed for #includes. Project-specific includes should use the #include "Project/File.hpp" style. Standard headers should use #include <string> style.

Preprocessor tokens all use UPPERCASE_NAMING style with one exception: Compatibility shims. For example, right now, we have a thread_local preprocessor token until thread_local is an officially supported keyword.

Types tend to use PascalCaseNaming style. If the type is basic enough, belongs to the STL, or is intended to work primarily with the STL, it uses underscore_naming style.

We have type aliases for many of the standard library types so as to not affect calling code if these types change. We also have type aliases for many of the in-game types, like item identifiers. These are not checked in the hard sense (so for example, there's no real difference other than the keystrokes between item_id_t and int32_t), however, they are preferred to give the reader of the code semantic information.

Do not use typedef if it can be avoided. Use using aliases if at all possible.

Type aliases should probably go in Types.hpp unless there's a compelling reason not to.

Forward declarations should proceed in alphabetical order. class, enum, enum class, namespace, struct, template. Within each category, alphabetical order should be observed. For example:

 class PacketBuilder;
 class PacketReader;
 struct PortalInfo;
 struct SplitPacketBuilder;

The auto keyword should be avoided unless the type of an expression is already clear for other reasons or really doesn't matter. Examples:

 // This is bad, it doesn't convey what type the integer should be.
 auto x = 5;

 // This is good because the type is clear.
 auto x = reader.get<int32_t>();

 // This is okay.
 auto &provider = ChannelServer::getInstance().getEquipDataProvider();
 // The type of the provider isn't really important.
 // These never go into packets or anything and the compiler checks all of these anyway.

 // This is probably bad.
 auto x = someClass.functionCall();
 // We don't know what functionCall's return type is expected to be and the semantic information may be important.

Braces should be present on all conditional expressions, such as if, else, while, for, etc. The only exception is large if/else if/else chains that only do one thing.

Opening braces do not have their own lines with two exceptions:

• Constructor bodies
• Arbitrary scopes (since there's nothing to put before an arbitrary scope, this one is more by necessity)

Closing braces always have their own line with one exception: An opening brace and a closing brace are present in the same line.

 if (condition) { /* code */ }

Access labels are aligned with the class declaration level. switch cases are indented one level and their contents indented another level if the case takes up more than one line.

 switch (cond) {
 	case 1: return someValue;
 	case 2:
 		someValue += 5;
 		return someValue;

The auto function declaration style should be used.

 auto func(args) -> return_type

Braced initialization is preferred unless there is a strong reason to avoid it.

Member variables use a prefix of m_ and camelCaseNaming. Function arguments, local variables, etc. use camelCaseNaming. Private static variables should use a prefix of s_.

Constants should use PascalCaseNaming. Namespaces should use PascalCaseNaming.

There should be no spaces after opening parens or before closing parens and there should be spaces after semicolons in conditionals. Examples:

 if (condition)
 if (condition && other)
 for (int i = 0; i < 10; ++i)
 for (auto &elem : cont)

Prefix increment and decrement should be preferred to postfix.

Reference and pointer types in general should be hugging their associated variable. This is not always possible especially with const. When returning a reference or pointer from a function, there should be spaces on both sides of the operator.

 auto MapleTvs::getCurrentMessage() const -> const MapleTvMessage &
 for (const auto &val : playerSkill.vals)
 auto LuaExports::pushGetVariableData(lua_State *luaVm, LuaEnvironment &env, const string_t &value, VariableType::Type returnType) -> lua_return_t

Typically, const goes before the type. For pointers to const, const goes after the * and before the variable name.

 const MobSkillLevelInfo * const skill

bool should be dispreferred unless true or false are valid answers/values based on the name of the thing. enum class is the preferred method of handling cases where bool does not result in obvious semantics. For example:

 isOnGround = true; // Good, because "is" can be answered by "true" or "false"
 if (someFunctionCall()) // Bad, because "someFunctionCall" cannot be answered by "true" or "false"
 if (someFunctionCall() == Result::Successful) // Good, corrected to make it clear what's going on

std::string has an .empty() function. Prefer this to explicit comparison to "".

Initializer braces may have an optional space. Ensure that it's consistent on both sides. Examples:

 const int8_t byteorder[EntryByteQuantity] = { Byte1, Byte2, Byte3, Byte4, Byte5, Byte6, Byte7, Byte8 };
 array_t<bool, Inventories::InventoryCount> chanceItem = {false};

C-style casts should not be used. Use static_cast. const_cast should only be used in cases where it must be, for example, interop with C functions. reinterpret_cast should be used sparingly. dynamic_cast probably shouldn't be used or need to be used at all.

Avoid raw pointers if possible. There are lots of these littered around the code so it's unavoidable in many cases, but I'm slowly refactoring to eliminate as many raw pointers as possible. References should be preferred if possible and reference-counted pointers should be preferred next.

Do not write singletons. There are 4 left and I am eventually going to eliminate all of them.

Namespaces should introduce a block of indentation in header files. In implementation files, all wrapping namespaces should be stacked, introduce no indentation, and include newlines before and after the contained code. For example:

 namespace Vana {
 namespace Packets {
 namespace Code {
 //
 // ... code goes here
 //
 }
 }
 }

The exception to the indentation rule is if there is a namespace designed to contain information specifically for that implementation file, in which case, the namespace will introduce a level of indentation and be considered to be part of the implementation file's code, exempting it also from the stacking rule.

Inheritance should mostly be avoided. This is an endemic problem in the code right now and eventually I'm going to refactor it almost completely out. Inheritance in general is for is-a relationships. This causes problems when you apply it to things like Player. Player is a connection right now, instead of having a connection. Which means that once that connection closes, the Player is gone. It's imperative to be able to control this at a finer level of granularity. However, it makes sense for things like AbstractServer.

If possible, represent numeric constants as symbolic names in the source. The Common/Game Constants folder has a large set of predefined constants.

Avoid getter/setter properties in class design if possible.

Within class declarations, public access is preferred to go first with protected and private going after.

Intentional fallthrough in a switch should be marked by a comment to convey intent. Additionally, odd blank code should be noted as intentional via comment.