Documentations and warnings

[Lighta]

Hi,

in order to improve Rathena dev I believe we need a better inner documentation, as it been said many time. There was no real improvement here for existing code even trough new one had huge comment.

So I did a little documentation myself, very basic but may be usefull, I gave it some time before on irc but doesn't seem you guys wanted to comit it so here it is :

http://pastebin.com/zB9rBZJb

Another subject of great important is all the ignore warnings x in the configure script. Thus creating and compiling our file without those check or at least thoses warnings.

So now let's face it what the points of ignoring some warnings ?

Are we so great that we know our code so well that we don't need to check warnings ? Even so we still could show them and ignore it'd be better then saying don't even tell me nothing about it. And if it was the case we wouldn't spent so much time trying to fix somes trivial warnings even trought when fixing them we create others warnings that we don't know about cause we don't show them...

Finally this would help people doing mod.

Ok now to speak more concrete we have those building option :

Wno-sign-compare -Wno-unused-parameter -Wno-pointer-sign -Wno-switch -fno-strict-aliasing

The only one understanble may be wno-switch since we have really long enumeration sometime, and that adding default : break; isn't something pretty.

fno-strict-aliasing may also still since will have to rewrite a lot to match it but it may be good to do it for optimisation :

http://cellperformance.beyond3d.com/articles/2006/06/understanding-strict-aliasing.html

Finally here a quick diff for configure to see our current warnings without the overrinde :

http://pastebin.com/UWUisbTi

It's quite ugly but it's just to see the warnings, we may add a configure options in future to add those flag or not.

[sketchyphoenix]

A lot of these comments are redundant and should be removed, actually.

Commenting isn't really a competition or anything. If the code speaks for itself, then it's fine.

[Lighta]

If you look closely I wasn't only speaking about documentation, sure code may speak by itself but it's mainly true cause were used to and edit much as we like.

Here was really basic thing something better would be general purpose of function and perhaps variable in structure so even fresh dev will know how to edit thing consistently.

NB : even on rathena we have something like :

status_heal(bl, hp, 0, 0);
               if( tstatus->hp != tstatus->max_hp )
                   clif_skill_nodamage(&src->bl, bl, AL_HEAL, hp, 0);

Instead simply do

status_heal(bl, hp, 0, 2);

Here it's quite ok since tstatus take battlestatus wich also take account bonus but there was code similar wich ain't resulting in not healing (if we had done sd.status.max_hp...)

Beside this check is unecessary since status_heal can handle it and do a similar check, resulting on a non consistent code at least and not optimise (since I seing lot of optimise attempt lately...)

I already told Xaxax about this for EPICLEIS and I see the change was made wich i'm glad but there is other and I hope this probably wouldn't happen with even a basic doc.

Another point is if you looked pc.c doc you'll see that some return value conflict, (not a big issue but since they aren't fully consistent we can't really use them safely or at least I wont for all)

Now the other part of this was WARNINGS. I honnestly don't see the benefit of shutting them down. (like let's auto ignore them)

A warning is afterall just something to point out our attention that something may be wrong.

So ok maybe we could keep -Wno switch since we have a lot of enum and turning this one on mean adding a lot of default.

But :

-Wno-unused-parameter : same as before if we want to do some optimisation why the hell would we want to assign/declare variable we wont use ?

-Wno-sign-compare : simple consistency, actually only @mapflag giving some errors on this.

-Wno-pointer-sign :I don't really remenber what function had warnings with this one..

Also just a quick note :

There been a lot of

fileReadCount = fread(...);

I suppose this to fix the warnings, "return value unsed", well we still don't really use it if we just assign it to a variable we never read... we might also another warnings exception at this point.

A real fix would be to trowh an arror if the bytecount isn't matching we pass on argument or something like this.

Hope you get what I mean now.

[databits]

Good code documentation is a great idea, but only when done properly. It's as simple as following a few basic rules.

Intention (What's the code supposed to do)

Don't comment individual lines of code. Instead comment entire sections or blocks with a short explanation on what that portion of code is supposed to do (not what it does).

For example, in the code posted above, avoid things like this:

default: //do nothing
 break;

This comment is meaningless. It's obvious that the default condition does nothing, the comment itself serves no purpose.

Consistency Try to pick a standard to use for the project and stick to it.

For example, don't do things like this:

//Comment 1

versus

// Comment 2

This is a little harder to do on group projects, but it's not really any different than bracket and white-space usage. People working on the code should adapt to the style the project uses.

Authoring and Credit

Don't write your name in the comments by sections of code that you modify or add. eAthena has stuff like this strewn throughout it, and it serves no purpose to the code itself. If you need to find who changed a line, it's not hard to look at the history of the file in version control. If each file needs to have a list of authors, it should be at the top in the file comment header section. You don't even need a comment on what was changed, again you can refer to commit logs.

[Nameless2you]

I have to disagree on this part, as much as commenting is not really required, leaving the authors name on a specific section if they've created it should be ok, that way they can get some attention and gratitude for the work they've done and not be generalized as simple "dev".

The do nothing example of course is although incorrect because it should be does nothing, but aside from that I agree with it.

However I wouldn't mind if behind

getitem 601,1;

there was

getitem 601,1; //apple

, now the example is silly because everybody knows 601 is an apple but, not everybody knows that 12103 is a bloody branch.

As to indenting, I know I personally don't use it too much when I'm on a script spree because I know what I'm doing, but I have to agree with it's usage, it makes things easier for everybody else and therefore I support it, however anything that's in a comment should be up to the author, one could go through each script and change it but that would simply be aesthetic and not required as it isn't required to be read anyways so why make it pretty.

Edited May 16, 2012 by Nameless2you

[Lighta]

Well yeah the "do nothing" was obious still i'm pretty sure there was already something similar in emu.

I add that "default : break;" only for the switch warnings, but sure we could explain default more, or sometime the switch already have a short desc, (e.g : "parsing boss immunities.."), so well you could understand it like "do nothing for the rest of statuses". It will anyways sound more and more obvious as you know more of emu. My philosophie with it is that comment don't hurt if they're short enough to not increment code line, (like 3 line with /* * */ for something we don't care much is annoying) and that you can maintain them (they are up to date easely, cuz if you have to change 10 comment for 1 move that annoying as well.)

Name and Credit,

Well sure you can look at svn if you have one, starting to be more delicate when you switching, like if you taking that way you really need to keep old log in memory. (wich is good but longer)

Log doesn't always state the change : "* Merged changes up to eAthena 15096" rA r16106. (This one was about 3rparty and vcproj).

So let say our myql having an issue, you 1st need to found out that "merge up 15096" was containing the change then go to eathena tracker to see who did it.

Ofc since we don't put name on those file this example doesn't match really but the point was log may be long and it will be anyway longer then a comment.

Now it's not a big issue you choose to put them or not that all, if it's a basic change then you don't need to add it. If it something that conflict/custom then yeah I'm adding it (that only to prevent people merging this area and broke it, like before changing it you could ask me a quick question and what was that for ?..). With this said it's right that they not that usefull if you can't reach the guys anyway.

[xRaisen]

Lighta is right. And its a good practice to have it commented and documented. For it will be very useful in the future. Lighta must be an experienced person for he felt the importance of it.

And just incase you guys retired, new developer can maintain it so easily.

This is indeed a warning..

Edited May 16, 2012 by xRaisen

[databits]

I respect your opinion on putting names in comments, but I still don't agree. If people are working on an open source project solely for attention and gratitude, then there's already an issue. You don't need your name next to every piece of code you add or edit to receive thanks for contributing to the project, and as Lighta pointed out, it does no good having a name for someone retired from the project. Besides, most users of the software won't even see it, or if they do they likely won't care. There's a reason that many other projects out there have something akin to a credits page or document. What happens if the section of code you wrote ends up being replaced or removed? Should you no longer receive credit for contributing to the project? Well if you're in a credits page/document rather than just a name strewn around the code, it's still shown that you did contribute, even if your contribution is no longer used.

As for the bad commit messages, that's just people not really knowing how to use version control. Commit early, commit often. Small, concise commits with a short description on what the commit does. A merge message should be exactly that, a merge. If there's anything other than merging and conflict resolution in it, then the developer should just not do that. Just because something is open source doesn't mean people working on it should ignore good practices.

[xRaisen]

I agree to your last paragraph databits. For what I understand, comments like "//this function should do the circling" or "//needs a clean code revision" is a good practice and should be always followed. if anything more to that that is unnecessary should not be included like "// he-he-he this is funny" or "//coded by hackmenot" and the worst i encounter "//your such a noob mr.xxx. go look and find me ". however, it is good to put that thingy if its just to make a temporary marker where your last touch is and should be again removed if its not useful anymore.

[Nameless2you]

I respect your opinion on putting names in comments, but I still don't agree. If people are working on an open source project solely for attention and gratitude, then there's already an issue. You don't need your name next to every piece of code you add or edit to receive thanks for contributing to the project, and as Lighta pointed out, it does no good having a name for someone retired from the project.

Read lighta's post please.

Not every change only custom stuff or major things.

For instance adding a new script command makeitem or other stuff that has significance, it also helps for the future when you need support for the specific command and want to ask the creator.

[sketchyphoenix]

Except you'll find out the creator quit a long time ago and fell off the face of the internet or they aren't interested in helping anymore. A lot of the core and oldest parts of rA's code come from either jA or people that were around 6+ years ago that aren't developing anymore.

Credits would be best left to a txt file or at the top of a source file right after the license info and not scattered all over the place.

[databits]

That was kind of part of my point. Same issue if the person IS still on the project, but ends up out on vacation or some family emergency for an unknown amount of time.

Nameless2you, I did read the post, entirely in fact. It's why I deliberately said, "..add or edit...". I myself don't distinguish between the two. Code is code, regardless if it's a new addition, a re-factor, or just bug corrections. The whole point of commenting is to explain what a piece of code is supposed to be doing. If you need to track down an author for any reason to explain it, then it usually means the code probably isn't commented properly, is poorly written, or both (assuming you're actually a programmer and can read the code to begin with).

[edit]: added missing space

Edited May 18, 2012 by databits