[Dev] Coding Standards

[Quintillus]

It looks like we don't have a thread on coding standards yet. While it's probably natural that the evolve, and I've hesitated to bring it up (since what if the standard we decide on is the opposite of what I'd prefer?), overall it's probably better to have a few than to have none whatsoever.

I'd like to think of this is a "propose something, we discuss/vote on it" type of thing. Where a vote could be a vote for option A, option B, option C (if available), or not setting a standard. A standard would only be set if one of the options receive a majority of votes (or a plurality, if someone says, "I vote for B, but switch to A if that makes it a majority").

I'll start with a couple proposals:

-----------

Variable type specifications:

Option A: We specify variable types:

int theAnswer = FindTheAnswerToLife(someParameter);
Civ3Map baseTerrainMap = new Civ3Map(map.numTilesWide, map.numTilesTall, map.RelativeModPath);
DifficultyObject relativeDifficulty = calculateMapDifficulty(baseTerrainMap);

Option B: We do not specify variable types:

var theAnswer = FindTheAnswerToLife(someParameter);
var baseTerrainMap = new Civ3Map(map.numTilesWide, map.numTilesTall, map.RelativeModPath);
var relativeDifficulty = calculateMapDifficulty(baseTerrainMap);

My vote: Option A.

Reason: The author always knows what type something will return (or should); specifying the type is easy up-front. Down the line, it improves readability. In the middle line above, it's easy enough, but in the first and third lines, you don't necessarily know that TheAnswerToLife always returns 42, or that the calculateMapDifficulty method returns an object instead of an number, when types aren't specified. Specifying the types saves a future reader who isn't familiar with those methods' return values from having to look them up, or having to try to figure it out from the code that comes next.

-----------

Multiple Return Values:

Option A: Allow Multiple Return Values

public (ReturnType1 firstReturnValue, ReturnType2 secondReturnValue) calculateSomeValues(int param1, string param2) {
    //Do some calculations
  return (firstReturnValue, secondReturnValue);

(returnValue1, returnValue2) = calculateSomeValues(14, "randomString");

Option B: Don't Allow Multiple Return Values

public ReturnType1 calculateOnlyTheFirstValue(int param1, string param2) {
    //Do some calculations
    return firstReturnValue;
}

public ReturnType2 calculateOnlyTheSecondValue(int param1, string param2) {
    //Do some calculations
    return secondReturnValue;
}

returnValue1 = calculateOnlyTheFirstValue(14, "randomString");
returnValue2 = calculateOnlyTheSecondValue(14, "randomString");

My vote: Option B.

Reason: It can be difficult to reason about why there are two return values, at least if they aren't obviously and closely related. Returning multiple values also makes it much easier to not follow the single-responsibility principle.

If we do have multiple return values, the "why" should be preferably obvious, but if not, then clearly stated. Maybe there's a tremendous performance speedup due to using one method, for example, although I'd probably ask if memoization might be a better idea in that case.

An example where I would be okay with multiple return values is a method that returns an X and a Y coordinate, if there is an appropriate reason to need that to be returned versus, for example, a Tile object.


-------

We also probably ought to decide on PascalCasing versus camelCasing versus snake_casing, and when to use each; we've been tremendously inconsistent there, although I haven't felt a lot of pain from it. I'll let someone else write a proposal for that when they are feeling annoyed by it.[/CODE]

 

[Puppeteer]

I vote specifying type when declaring variables.

I haven't really thought about returning multiple values. I thought I might have done so in the media reader code, but I don't see that offhand; I seem to be using classes to track multiple bits of data.

I thought I read somewhere that C# is PascalCase, but later I read that parameters and short names (i, x, y, etc.) should be camelCase. I see others are using camelCase in local variables and sometimes in class field names. I'm not that picky, I just need a reference for when I inevitably forget.

I don' think I've used snake_case for anything unless it was tied to existing code, but offhand I can't recall having to do that in C7. I did prepend the _Console folder with an underscore to kind of separate it from the other libraries.

New thing for the thread: My older (early 2021) code puts the leading curly brace on the same line as the block declaration, but I've since given into that C# just does it with both braces at the indent. But I haven't gone back and "fixed" any older code.

 

[Flintlock]

I vote we use "var", at least in situations where the variable's type is obvious like "var x = new Whatever()". Even in other circumstances, if you're reading some code and aren't sure of the type of some variable, your IDE should be able to tell you that easily.

About multiple return values, I vote we continue using those as well. The feature can be misused but there are situations where it's very useful so I don't agree with a blanket ban. For example, from my unit animation PR that Quintillus commented on, I added this function:

public static (ImageTexture palette, ImageTexture indices) loadPalettizedPCX(string filePath)

It loads the palette and color indices from a PCX file and returns them as two separate textures as opposed to combining them into one. Splitting this into two functions doesn't make sense. The sensible thing is to load the file once and get both pieces of it, not load it twice for each piece. Another possibility would be to add a struct containing the two pieces whose only purpose is to be returned by this function, but I think it's better for code simplicity to minimize the number of types & functions defined.

In this case the function naming is unfortunate. When I wrote this function there was already one called "LoadPCX" that also depalettizes the PCX to return a single texture. This function should have that name since it's a more raw loading function and LoadPCX should be called something like "LoadAndDepalettizePCX" to indicate that it does additional processing.



What happened on the question of tabs vs spaces? I saw you guys talking about it once and got the impression you had decided on tabs. I was happy to go along with that since it's my preference, but there are some recently created files that use spaces. We should decide one way or the other (my vote is for tabs) then make one big gnarly commit to convert everything and be done with it. We should get this done during the holidays while we aren't working on other things to minimize disruption.

 

[WildWeazel]

In general, I prefer using the conventions of the platform(s) in use. So Godot's style guides at least for the UI layer, otherwise Microsoft's C# standard, assuming they're not in conflict. But I'm open to suggestions. At the end of the day the most important things are clarity and consistency.