Update Coding GuildLines (discussion)

[jokoho48]

Mike and i are currently Updating and Fixing all ACE Module Scripts and we are change some code for better performance and Readabilty they are not include in the Coding Guildlines for Example:

we Currently Change the PARAMS_N Macro from CBA to the params command that is now hardCoded in ArmA 3 sinse Update 1.48.
and we change some ForEach to Count because a Count is ca. 20% faster than a forEach but dont have a _forEachIndex

Other Changes are we doing for Swtiche Cases the Reason is that a Switch case is slower than a call {if ()exitWith{};}; and in only some places act different from a switch

[MikeMatrix]

Although it has to be noted, that for most smaller switch cases the difference is not as big.

Another big thing to consider is to fail as early as possible, to not waste any calculation time where you already know it's not going to matter.

Instances of that are chained boolean evaluations as return values, that could be separated out into multiple if () exitWith {}; statements. This improves readability and best case scenario is the first evaluation kicking it out of the script right away.

This also counts for parameter and fail state evaluation at the beginning of a file.
If there is a global or a command that the function depends on to be in a certain state, check right away. Before private even, since private is also an execution in SQF. If there is a failstate based on parameters, then do so right after parsing them.
If a parameter needs to be modified in order to evaluate them in the function, then this should also be done right after evaluation and fail checking of parameters.

So a small example would be something like

#include "script_component.hpp"

#define SOMESTATICVALUE 5

if (isServer) exitWith {};

private ["_someVariable"];

params ["_that", "_thing"];

if (isNull _thing) exitWith {};

_that = _that + 5;
_someVariable = _thing;

if (_that < SOMESTATICVALUE) exitWith {false};
if (ACE_player != _thing) exitWith {false};
true

[kymckay]

Guys, there comes a point where the performance gain is totally negligible and changes only make the code less readable. 😛

Moving conditions before the private statement is not something I'd bother to do. Also, you are aware that exitWith is slower than regular conditions?

Also there's some redundancy in your example:

#include "script_component.hpp"

params ["_that", "_thing"];

if (isServer || {isNull _thing}) exitWith {};

!((_that < 0) || (ACE_player != _thing))

[jokoho48]

yes its slower than then faster but than Swtich Case

[MikeMatrix]

@SilentSpike I guess I wasn't clear enough with this:

Instances of that are chained boolean evaluations as return values, that could be separated out into multiple if () exitWith {}; statements.

What I meant here is somethings along the lines of

ACE3/addons/respawn/functions/fnc_canMoveRallypoint.sqf

Lines 27 to 33 in 3a3dad9

	_unit getVariable ["ACE_canMoveRallypoint", false]
	&& {!isNull ([
	objNull,
	missionNamespace getVariable ["ACE_Rallypoint_West", objNull],
	missionNamespace getVariable ["ACE_Rallypoint_East", objNull],
	missionNamespace getVariable ["ACE_Rallypoint_Independent", objNull]
	] select ([west, east, independent] find _side) + 1)}

where the gain of exiting early is a big benefit.
(Obviously there are other optimizations, that can get rid of this specific example, but that's what came to mind right away.)

[PabstMirror]

One thing that really should be considered is how often the code is run.

If something is inside of a loop or a per frame or called on every shot fired, then yes we need to optimize it as best we can.

But for code that is run ~5 times in a session we don't need to go too far. Readability and maintenance can become just as important.

[MikeMatrix]

@PabstMirror absolutely agreed.

[jonpas]

I agree with @PabstMirror here as well, additional case where speed can be very important besides loops is where a code must execute very quickly, eg. something doesn't interrupt it or similar.

[commy2]

There are more differences between forEach and count.

forEach returns whatever the last loop returned, while count always returns a number.
And most importantly count expects either NIL or BOOL as return value of the script on the left side. This caused many script errors in AGM when BI changed pushBack to return the array size insted of nil.

The difference might be 20% for very basic code, but even that is neglible when the content of the loop gets more complex. Both commands are reasonably fast (for SQF), so 20% isn't actually that much of an improvement in time.

I wouldn't replace count with forEach and I have no idea why the idea of it is so popular.

[jokoho48]

hmm you can add a boolean or nil at the end and dont have problem with a pushback and save performance

[bux]

I wouldn't replace count with forEach and I have no idea why the idea of it is so popular.

I kinda agree. 'Cause we all know the saying: Premature optimization is the root of all evil.

Maybe someone could do performance tests to see if it is really that much of a difference.

[jokoho48]

yeah ruthberg and i tested it both and count is on our both maschines ca 20% faster

[commy2]

I would test it, but game updater ate my arma3.exe and the steam check files shit is still at 0% after an hour.

[MikeMatrix]

I just ran some tests for you to get you some values.

I filled an array with 1000 string elements "test".
The functions that I tested were as follows:

{nil} forEach test;

and

{nil} count test;

This is to minimize the effect of any outside performance influence to a point where the difference is more clear.

The performance tests where conducted using CBA's CBA_fnc_benchmarkFunction.

result = [function, nil, 1000] call CBA_fnc_benchmarkFunction;

Tests were conducted in the editor, with an empty map except 1 unit, running the development version of the current master branch.

Here is a couple of values I observed:

forEach	count	% diff
0.00120801	0.000904053	28.7828
0.00117407	0.000899902	26.4391
0.00119397	0.000906006	27.4255
0.00119202	0.00094397	23.2255
0.00117908	0.000910889	25.6644
0.00118298	0.000912109	25.8579
0.00118408	0.000906006	26.609
0.00120691	0.000914062	27.6144
0.00117798	0.000901978	26.5391
0.00118701	0.000912964	26.1001
0.00119006	0.000911011	26.5629
0.00123303	0.000919067	29.1775
0.00118201	0.000904907	26.5559
0.00118005	0.000903931	26.4995
0.00121509	0.000916016	28.0674
0.00118591	0.000907105	26.6418
0.0011759	0.000896118	27.006
0.00119897	0.000928955	25.3786
0.00118701	0.000903931	27.0769
0.00121204	0.000912964	28.148

Obviously this test is designed to make the difference extremely clear if there is any. Actual performance improvements will vary, depending on the implementation they are used in. However it is clear that there is performance improvements.

Result values are not always relevant when it comes to iterations. If there is the rare case of it actually mattering, it would likely be an easy fix.

When it comes to readability, both are fairly equivalent in my personal opinion.

[commy2]

In a real application of forEach you typically have a small array and a large chunk of script.
0.0003 ms seems neglible (0.018 ms/s on 60 FPS), but that time depends on the machine you are using.

The time you'd use on replacing forEach with count is better spent on actual performance improvements, creating new content or going outside, IMO.

[MikeMatrix]

@commy2 since we're already going over the code anyways, this is a good opportunity to actually replace those. Even if it's just a small gain. There is other aspects, that slowly are getting replaced, for example the deprecation of the PARAMS_N macro.

TL;DR: We're doing code cleanups anyways, why not take advantage of that minor bit of performance, introduce it as something that is encouraged to do, but not enforced.

[commy2]

Go ahead. Just remember to make sure to return nil. Don't expect anything noticable though.

[commy2]

Ah. And test if exitWith inside count works the same as in forEach (with the returned value). I think I remember that I had issues with that previously.

[MikeMatrix]

Thinking about it logically, in the count you're not supposed to return anything except nil, true or false, so it is to be expected if you want to return anything else.
If you really want a result from an iteration like that, then you have to use forEach. If it's simply an iteration, then you should use count considering it is faster.
Again, this is intended as an encouragement, since the small gains accumulate fast over a large project.

EDIT: I'll test that in a bit.

[commy2]

Yes, you are correct.

Edit:
Although you can't use exitWith {true} or exitWith {false} in count, because you exit the scope with that.

[jonpas]

Replacing forEach with count doesn't impact readability IMO, but it should be noted that other minor performance improvements like that which do impact readability should only be considered in loops/PFHs/code that must execute as fast as possible, otherwise readability should always be number one concern.

[voiperr]

Re: count vs forEach:

Explain the difference in the documentation, but only enforce it in old code that hasn't changed in a while. Coders constantly tweak their stuff, especially if it's an initial release. It would be bad if PRs get stuck in gridlock because the code doesn't follow a minor performance benefit.

[thojkooi]

I don't see the count vs foreach becoming a standard. Best practice, I'd say.

I do disagree with the statement made by @jonpas; I think count does reduce readability as it's not immedialy clear what the intend of the code is. Do we want to count something or just simply loop through some elements?

Edit: that's not to say I don't think we should replace forEach by count where performance matters a lot; For example in per frame handlers.

[simonamdev]

I can relate with glowbal's comment. The reason I barely ever use count over forEach is because when I come back to the code in 2 hours/days/weeks/months/years time, forEach is understandable instantly: i'm doing something for each of these things, whereas count doesn't come so fast.