Code Santa Claus Event

[pnsrx8]

maybe a config like true or false for on/off will be better ;) 

 

 

Quote

 

[Kara]

On 29/11/2017 at 9:23 PM, .Elfocrash said:

Awesome job as always. 

 

In terms of code review and improvements here are my comments.

 

Line : Comment

 

51: ChristmasEvent should be extending an abstract class called event and shouldn't be a singleton. The XML Document should be on it's own EventConfigLoader. Remember that classes should be doing one job only.

53: Rewards shouldn't be static.

156-157: Should be extracted to their own method called something like determineSpawnProbability()

160-167: Should be extracted to spawnRandomNpc

169-171: Should be extracted to handleNpcDespawn

205-252: This is bad class design because it limits you to a very specific type of a set. Something like this should be generic, so i would suggest you check the Pair class.

271: Loading singletons like this is deceiving to the reader because what you express via your code is that the only reason this line is there is to print the reward size while in reality you are initialising the singleton. Just a .getInstance() should be enough.

332: applyTo is a bad method name not indicating the actions. A reward is not applied. It's given/rewarded. Should be rewardPlayer

337-341: Should be extracted to a method called handleXpReward()

343-347: Should be extracted to a method called handleSpReward()

349-353: Should be extracted to a method called handleRecReward()

355-356: Should be extracted to a method called handleRewardItems()

 

I won't comment on the onBypassFeedback method because the whole thing is poorly designed to begin with anyway.

 

Comments:

Here are some guides that you should keep on your mind when you are coding any type of system. Those are not the only ones, but they apply on what i saw.

• Classes should only have one responsibility.
• You should always use long self descriptive names for methods, classes, etc. Your code is your documentation.

I am only giving you really constructive feedback because i know you are keen to learn. Don't take it the wrong way. It is amazing effort.

 

Remember he is learning. Too much info for a small share :)) Nice of you but is too detailed for such a small engine.

[valentin]

nice  

Edited December 12, 2017 by valentin

[melron]

16 hours ago, valentin said:

nice  

look at the first line of the error. the system couldnt find SantaClaus.java in net.sf.l2j.gameserver.model.actor.instance

You probably paste it accidentally somewhere else like Custom package

[martuxas1]

Can adapte for l2jfrozen?

[Kara]

31 minutes ago, martuxas1 said:

Can adapte for l2jfrozen?

 

No because Melron is currently working in creating ARTIFICIAL INTELLIGENCE system which will take L2Monsters in another level!.

I'm kidding. Afcourse it can be adapted everywhere. Its up to you.. 

[BAN_L2JDev]

@melron Hi where can i put this? aCis 384 !

+		if (ChristmasEvent.getInstance().isEnabled() && killer instanceof Player)
+			ChristmasEvent.getInstance().luckySpawn(this, ((Player) killer));
+	

or I can put it to connect with Load ();
em ChristmasEvent.java
sorry for the few words i use google translator.

[melron]

23 hours ago, L2VANPER said:

@melron Hi where can i put this? aCis 384 !

+		if (ChristmasEvent.getInstance().isEnabled() && killer instanceof Player)
+			ChristmasEvent.getInstance().luckySpawn(this, ((Player) killer));
+	

or I can put it to connect with Load ();
em ChristmasEvent.java
sorry for the few words i use google translator.

should be in Monster class at method doDie

[BAN_L2JDev]

23 hours ago, melron said:

should be in Monster class at method doDie

Obrigado já avia conseguido antes da sua resposta quando vi o Codigo Killer pensei mesmo em colocar la
mais aCis 384 Attackable.java