The question is if we should change meaning of chr.score.rank. If no then
rank_t *CL_GetRankByIdx (const int index)
just useless,
chr.score.rank should point to an exact rank, while
level (or rank_no) is to define a promotion order between ranks. So the meaning of
chr.score.rank should not be changed BUT
CL_GetRankByIdx is not useless either. We wanna hide the underlying datastructure.
If we use
&ccs.ranks At time we change the datastructure we will need to search and replace all these references. If we use
CL_GetRankByIdx, only the body of that function needs to be changed.
I've already put changing ranks datastructure to a linked list on my personal wishlist so it's not limited to an artificial number (32 now).
You should also not forget, that
level is scripted, so 2 (or more) ranks can have the same level (even for the same employee category) unless we put extra check on that in ranks parser. But why would we? I think it's more flexible to allow more ranks of the same level maybe with different requirements and let the system choose randomly on promotion from which are met.
Can you say most typical mistake?
Mostly whitespace errors. It makes easier to read the code if you separate arguments of a funtion and so, like
- const rank_t *rank = CL_GetRankByIdx(chr->score.rank,EMPL_SOLDIER);
+ const rank_t *rank = CL_GetRankByIdx(chr->score.rank, EMPL_SOLDIER);
- for (int i=0;i<ccs.numRanks;i++)
- if (ccs.ranks[i].rank_no==index && ccs.ranks[i].type==type)
+ for (int i = 0; i < ccs.numRanks; i++)
+ if (ccs.ranks[i].rank_no == index && ccs.ranks[i].type == type)
Also, please put comments in separate lines before the thing it describes.
And check the gudeline of the Git commit comment:
* summary of the commit
* some detail notes if you feel like
The emplty line is important so as the asterisk at each line.
Coding tips & FAQ really lacks info about using git and specially about work with patches and stash.
The article
Getting the source contain some information about using Git.
Your patch was about right but... there is always a but
That two patches should be two but not in this way. As you fixed problems in the first patch with the second, they should be merged so it's easier to understand - you can use interactive rebasing for that:
git rebase -i
But you put two different things in the first commit:
* the campaign parameters patch
* and (part of) the ranks patch
These should be splitted. Interactive rebasing can help in this one too.
Git help:
http://www.kernel.org/pub/software/scm/git/docs/git-rebase.html one more about the code:
+ s->base[EMPL_SOLDIER] = -1;
+ s->rankBonus[EMPL_SOLDIER] = -1;
+ s->base[EMPL_WORKER] = -1;
+ s->rankBonus[EMPL_WORKER] = -1;
+ s->base[EMPL_SCIENTIST] = -1;
+ s->rankBonus[EMPL_SCIENTIST] = -1;
+ s->base[EMPL_PILOT] = -1;
+ s->rankBonus[EMPL_PILOT] = -1;
+ s->base[EMPL_ROBOT] = -1;
+ s->rankBonus[EMPL_ROBOT] = -1;
You could/should use a loop on employeeTypes for things like this. Shorter, better.
-geever