I must say, you're doing well for someone who is completely self-taught
Those rules are pretty good. I'd also suggest:
- Doing unnecessary calculations is sometimes ok if it makes the script easier to read or shorter and won't cause problems. A lot of "optimisations" people try to make don't actually make a difference to performance, are done in places where performance doesn't matter, and make the code longer/less readable (there's an expression: premature optimisation is the root of all evil). For example if you're in OnMissionStart then performance doesn't matter unless your code will be using more than 10s of milliseconds, which it won't be unless you're doing something very CPU intensive. And storing something like States.HouseType in a local variable will make very little performance difference unless you use it hundreds of times, so
- Use shorter variable names in places where it doesn't affect readability (HouseWalker would be more readable as I, that's a commonly accepted name for for-loop variable when it's an integer)
- Put spaces after each comma (,) in function/procedure calls: DoSomething(a, b, 7) rather than DoSomething(a,b,7)
- Comments which describe what a procedure does are not helpful at all. For example "this procedure gives player 1 an inn and executes another procedure". Describing your doesn't help me read it unless it's very complicated/hard to understand. By looking at the lines below your comment I can see that it gives an inn and call some other procedure, but why does it do that? How. It's far better to explain why you are doing something, how it fits into the bigger purpose, or explain what it does without worrying programming concepts about HOW it does it (for example, "this procedure searches for inns and then displays a message to all players" could be written as "Displays a message to all players for each inn found in the game".
- Comments which explain simple ideas which your reader should already know are not helpful. For example "define global constants". That's only helpful if I haven't seen the "const" keyword before, hopefully all your readers will have, and if they haven't, it's not your job to explain what it means. I don't want to discourage you from writing comments, but make sure the comment tells the reader something he didn't know or would have had to spend time figuring out.
- You use WAY too many unnecessary local variables, it bulks out your code without improving readability. Examples:
a. HousesListLength := Length(HouseList); // What's better about the left side? Both imply "Length of HouseList" very clearly.
b. CurrentHouseID := HouseList[HouseWalker]; // Steps like this are only needed when the right hand side is something very long and ugly, like "HouseList[Players[i + PLAYER_OFFSET] - LAST_HOUSE + 1]". In that case you need this variable, but in your case it just makes the code longer, and means I have to go and check "where did CurrentHouseID come from?" if I start reading from the middle of the loop. If you take my suggestion of using I for your loop variable, then you get HouseList
which is very clear to me, since it immediately tells me that it is being iterated by the loop. CurrentHouseID does not tell me that.
c. CurrentInn := CurrentHouseType; // This can be useful if you have some very complicated code which is hard to understand, and you want to make absolutely clear in the code after this that you are now dealing with an Inn, but in simpler cases like the one you presented it's just unnecessary.
- It's very unusual to indent all the lines within a procedure (including begin, end, var, etc.). It's more standard to have something like this:
- Code:
procedure DoSomething;
var
I, FirstVar: Integer;
SecondVar: array of Booleans;
begin
FirstVar := 7;
for I:=0 to 7 do
DoSomethingElse(I);
end;
Here's how I'd format your block:
- Code:
const
PLAYER_1 = 0;
INN = 27;
ALL_PLAYERS = -1;
// For every inn in the game, display a message to all players
procedure DisplayInnMessages;
var
I: Integer;
HouseList: array of Integer;
begin
// retrieve all house-ids and store them into an array
HouseList := States.PlayerGetAllHouses(PLAYER_1);
// search the array for inns. if found, show a message
for I := 0 to Length(HouseList)-1 do
if States.HouseType(HouseList[I]) = INN then
Actions.ShowMsg(ALL_PLAYERS,'Inn found, housetype = ' + IntToStr(States.HouseType(HouseList[I])));
end;
// At the start give player 1 an inn and tell all players about all inns in the game
procedure OnMissionStart;
begin
Actions.GiveHouse(PLAYER_1, INN, 10, 8);
DisplayInnMessages;
end;
You'll notice my code is MUCH shorter, making it easier to read and maintain (more of your code fitting on your screen at once). This is a balance between shortness and readability of the code, but I think you went too far towards readability, shortness is also important. The only one of your local variables I considered keeping was CurrentHouseType, because it's used multiple times and the IntToStr bit has a lot of layers. However, I find it more readable without it, the way I formatted it above. I wouldn't say it's wrong to keep it though, since you probably find it easier to read that way with less layers. Using it twice means that it is slightly more efficient to store it locally, but that would only be worthwhile if this procedure was running OnTick, currently it's not happening often enough for that kind of optimisation to be needed.
Is my way right? Well I think it pretty good, but it's a matter of personal taste. You can see examples of code formatted in my style in Annie's Hill, Annie's Garden, Center Castle, Coastal Encounter Scored, and maybe a few other places. I've tried to write those as neatly/correctly as possible so people can get into good habits. Open them up and see what you think. Some of them are a bit longer so you'll see that my attempt to keep things short makes it more readable overall. If I'd used your style it would be twice as long.
