Jump to content

Santa Claus Event


melron

Recommended Posts

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.

Link to comment
Share on other sites

  • 2 weeks later...
16 hours ago, valentin said:

nice  

3333.png

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

Link to comment
Share on other sites

  • 2 months later...
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.. 

Link to comment
Share on other sites

  • 2 years later...

@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.

Link to comment
Share on other sites

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

  • Thanks 1
Link to comment
Share on other sites

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

Link to comment
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.



×
×
  • Create New...