Coding Guidelines

[RiZe]

Hi folks. I would like to discuss about some coding guidelines because I think the source is often a mess. I know that many people are working on the project and it isn't so easy to "switch mode" from style to style but some coding guildelines would be great for the project. Let's see an example:

Code: Select all

 public void removeFloodProtection(String ip){	if(!Config.FLOOD_PROTECTION)		return;	ForeignConnection fConnection = _floodProtection.get(ip);	if(fConnection != null)	{		fConnection.connectionNumber -= 1;		if (fConnection.connectionNumber == 0)		{			_floodProtection.remove(ip);		}	}	else	{		_log.warning("Removing a flood protection for a GameServer that was not in the connection map??? :"+ip);	}} 

I have to say: "Wow O_O, what the hell ...". Yes I know the project is still growing and the old code is still here with some additions but look at the example again. It's ... what to say? A mess with capital M, E and both S. There are so many examples like this one but much worse.

I would recommend you to use some existing code guidelines or write your own but please. Avoid to commit code like the code I have posted as example. I know that reformating is slow process but sometimes clean up is done so ... try to look on the style too please.

All the best.

---
Maybe you find it usefull. I know that this is for C# but the code formatting works for Java too.

http://telemetron.googlecode.com/files/ ... elines.pdf

[JIV]

whats exactly wrong with that code?

[RiZe]

Maybe you will don't agree with me but this is what I don't like in examples

Conditions

Code: Select all

 // In most cases there is an space between "if" and condition but not here, why?if(!Config.FLOOD_PROTECTION)	return; // Ok, this is better formif (!Config.FLOOD_PROTECTION)	return; // This is imo best but you don't have to agree with meif (!Config.FLOOD_PROTECTION){	return;} 

Try, catch block

Code: Select all

 // Hell, what is that? I saw that today but forget wheretry { ... } catch (Exception e) { } // Much better isn't it?try {	... } catch (Exception e) {} 

Line breaking

Code: Select all

 // Again, in most cases there is line break after condition but not herefor (L2ShortCut sc : allShortCuts) {	if (sc.getId() == id && sc.getType() == L2ShortCut.TYPE_MACRO)		_owner.deleteShortCut(sc.getSlot(), sc.getPage());} // Reformattedfor (L2ShortCut sc : allShortCuts){	if (sc.getId() == id && sc.getType() == L2ShortCut.TYPE_MACRO)	{		_owner.deleteShortCut(sc.getSlot(), sc.getPage());	}} 

Just few lines lower...

Code: Select all

 // Some line breaking would be great_revision++;L2Macro[] all = getAllMacroses();if (all.length == 0) {	_owner.sendPacket(new SendMacroList(_revision, all.length, null));} else {	for (L2Macro m : all) {		_owner.sendPacket(new SendMacroList(_revision, all.length, m));	}} // Like this_revision++;L2Macro[] all = getAllMacroses(); if (all.length == 0) {	_owner.sendPacket(new SendMacroList(_revision, all.length, null));} else {	for (L2Macro m : all) 	{		_owner.sendPacket(new SendMacroList(_revision, all.length, m));	}} 

I know that this is an old code but sometimes it appears in the timeline (I mean not so old code) too. You are doing great work at all and I don't want to hurt you but think about it.

[Probe]

try making huge switch cases and try inside try with such code format, tell me how soon you get lost

[RiZe]

Then ask yourself. Is switch right choice in your case? I'm talking about some standards which are missing in some files or file parts.

[Probe]

switch cases are much faster than 70 if clauses . performance is also of an issue here

[Deadmeat]

Like you said there is a lot of people here posting fixes and the L2J team has to look over and test them as well as work on there own projects and they don't have the time to go over every line of code and correct it, so they or other people like you post clean up scripts when they have the free time.

Myself and I'm sure some others here never went to school for programing or took any classes in it and are just learning on the fly how to program by looking at code here and other websites on the Internet and spending hours to do something it would take you seconds to do, and I do try and kept the code clean but when it starts getting late the code starts to go it's own way and I'm sure if you were to look at some of the stuff I've done it would make you sick, but the way I look at it why add more bytes to my files with spaces, returns, and braces.

[ZaKaX]

Lol... you do realize you can reformat whole core in few seconds using Eclipse? Instead of making that space/lines break corrections manually like a newb

Right click on the project > Source > Format. w00t! You just saved 465465hours of useless shitty work. Much better, isn't it?

P.S: Making brackets when you only have one line after a condition is useless, and ugly.

[Vapulabe]

In C, you've R&K and Ansi style for braces... I think that R&K still has some point...

R&K :

Code: Select all

 int func(void) {   if (cond) {    inst;  } else {    inst;  } } 

Ansi :

Code: Select all

 int func(void){   if (cond)  {    inst;  }  else  {    inst;  } } 

First has the advantage of giving smaller (in number of line) code. This allow you to see more code in your editor window and make the reading easier

I'd say that in while/for/if/..., braces should ALWAYS be used even if there is only ONE line.
- no doubt about the range of code when there are lots of for/if/... imbricated
- when you need to add one line, no risk of forgetting to add the braces
- when you've multiple IF and an ELSE statement, make code much clearer
- code reformatting works better when braces are present
- braces also act as a syntax check (you need to have opening and closing braces in pair)

I'd add that some JAVADOC could be very useful...

[ZaKaX]

I find "Ansi" easier to read...o0 the other one is ugly.

[janiii]

from work and from java tutorials on web, i am more used to r&k than ansi. but because l2j is in ansi, i have to use here ansi althaught i hate it too much lines for nothing.. ^^

[jurchiks]

I would agree to ZakaX, ANSI looks much better, and about the primary topic - backets do make it much simpler to understand the code

[Probe]

they do?
please scroll down the file L2AttackableAI.java
tell me how much of it you understand with your brackets
I agree that Ansi is more "readable" and nicer for the eye if the code isn't that complex, I use it myself for the simpler stuff.. but imo for the more complex files, such as AI's, and large objects (L2Character, L2PcInstance for eg.) R&K would be the preferable choice..

[jurchiks]

meh, that's what you think, and everyone has his own opinion, the main idea here is to keep the code clean, not messy, easier to understand

[Probe]

tell me if you think I'm not right after looking at that file