Code Acis Coupons + Redeem

[xxdem]

But well since you explained you see it as a "error" and not an "exception" (so far it's called NullPointerException, not NullPointerError :D), what you say is correct, for yourself. If you seek on the internet the "standard" behavior, however, it's not that. After it's personal preferences, personally when I see a null I drop it, it remembers me the dark age of L2JIL where everything was tested using null, even if not needed ; so you were losing tracks of what could be null and what couldn't.

 

Of course its an error, I don't know why the Java creators decided to make the NPE a RuntimeException in the family of throwables, imho it should only exist in debug versions of software indirecting the developer to fix his code asap. NPE should never exist in a code and should be handled with null checks. Of course in Java its coding style, you can avoid null checks and nullptr with many ways like the one you've suggested above, which ofcourse adds unnecessary overhead and for my coding taste over-complicated due to the fact that you use allocated memory as a matter to exit your code or do nothing.

 

Personally I have a strong C background, and the nullptr is the ultimate tool to solve such problems, I don't know why Java developers get crazy with it, maybe its due to the fact that they don't know that null checks cost very little cpu cycles whereas workarounds involve unnecessary JMP instructions that have a hundred time bigger overhead.

Edited July 20, 2017 by xxdem

[Tryskell]

Of course its an error, I don't know why the Java creators decided to make the NPE a RuntimeException in the family of throwables, imho it should only exist in debug versions of software indirecting the developer to fix his code asap. NPE should never exist in a code and should be handled with null checks. Of course in Java its coding style, you can avoid null checks and nullptr with many ways like the one you've suggested above, which ofcourse adds unnecessary overhead and for my coding taste over-complicated due to the fact that you use allocated memory as a matter to exit your code or do nothing.

 

Personally I have a strong C background, and the nullptr is the ultimate tool to solve such problems, I don't know why Java developers get crazy with it, maybe its due to the fact that they don't know that null checks cost very little cpu cycles whereas workarounds involve unnecessary JMP instructions that have a hundred time bigger overhead.

 

I think it's just a design pattern, like for example you can use .remove( from a List/Map without the need to know if such entry actually exists or not. You simply invoke the method no matter if it removes something or not, and you can only know if it removed something when you use the returning variable. It would be annoying to test null everytime for everything. I think it's the same behavior with the "not null container" pattern.

[xxdem]

I think it's just a design pattern, like for example you can use .remove( from a List/Map without the need to know if such entry actually exists or not. You simply invoke the method no matter if it removes something or not, and you can only know if it removed something when you use the returning variable. It would be annoying to test null everytime for everything. I think it's the same behavior with the "not null container" pattern.

 

Yes but on the specific case we are not talking about values that may be many, but a collection which has to be null checked only once, basically the developer should always know what can be null and what not, on big projects annotations like @Nullable can be used, but you are right, if you loose the track of what can be null and what not bad things are going to happen and that's the developer to blame not the programming style

[melron]

I saw this idea in one of retail Lineage 2 quests.

Which one?

 

the idea actually came from here  :P

 

Updates:

 

• coupon manager list replaced with ConcurrentHashMap . Ty Tryskell
• some queries removed as variables and passed directly in prepareStatement. Ty Tryskell
• coupon generating id method changed since i have all the necessary tools in some packages (now using StringUtil methods and String instead of char[], Random -> Rnd too). Ty Tryskell
• the way of storing/getting rewards from coupon changed (@Tryskell i believe rewards should be in the Coupon class since we are talking about the same thing). Ty Tryskell
• all checks about lists are changed from null check to emptyList() . Ty .Elfocrash
• added some ternary statements . Ty .Elfocrash
• added grade type (NG,D,C,B,A,S) . Once the grade stored will not changed. Ty Tryskell
• grade type setted with multiplier so if you want you can multiple your rewards based on the coupon's grade
• added 1 more column in coupon table for storing the grade type.
• different html will apear if player havent coupons and try to talk to the npc. Ty Tryskell
• redeem bypass changed . Ty Tryskell

 

Thank you guys for your suggestions you are awesome!

Reborn12, on 20 Jul 2017 - 5:29 PM, said:

ah Baggo You Don't Know What Real Love Means,This flame is just love  :D

 

on Topic nice idea melron thanks for share

 

'Baggos', on 20 Jul 2017 - 5:47 PM, said:

hahahah pirama love you re, more than Tryskell.  :o

 

Merlon thanks for this share mate.. It's a fresh idea.   :1010:

 

Thank you guys!!

Edited July 20, 2017 by melron

[Tryskell]

- Use 

int gradeLevel = player.getSkillLevel(239);

to find directly the correct grade level (under an int) - since it's automatically rewarded, instead of calculating based on level.

 

- getCouponId is tricky, I wait an int from it (like getNpcId, getItemId(), etc), not a String. Maybe rename it. You can also simply use IdFactory and attribute an objectId, which avoid all String manipulation (care, you need to release the id on ticket remove). If you use IdFactory, then getCouponId() is really an id.

- your bypass got no try/catch, therefore if someone send crap values (not parsable int on selectedButton, let's say) you end with Exception.

- rewards still need to be edited.

- double check.

couponId.isEmpty() || couponId.isEmpty()

- Following is pointless, since coupons is already generated :D. Simply return the already existing List coupons.

+   public List<Coupon> getCoupons()
+   {
+       return coupons.isEmpty() ? Collections.emptyList() : coupons;
+   }

------------

 

For each

rewards.get(i)

inside a for loop, Elmoreden gods killed Pirama. So yeah, that explains why he is so angry. Edit your for loop for enhanced version :

+           for (int i = 0; i < rewards.size(); i++)
+           {

>

+           for (CouponReward reward : rewards)
+           {

And use "reward" directly, which references always to the same object. Doing a .get( means you SEARCH the object AGAIN on the list, which is a performance killer since you search it 6 times per iteration (10 objects = 60 searches). Also enhanced version is easier to understand.

 

------------

 

On this part of code, you don't need the temporary List "couponsIds". You can generate the sb directly from "coupons".

+       List<Coupon> coupons = player.getCoupons();
+      
+       if (!coupons.isEmpty())
+       {
+           List<String> couponsIds = new ArrayList<>();
+           for (Coupon cpn : coupons)
+               couponsIds.add(cpn.getCouponId());
+          
+           StringBuilder sb = new StringBuilder();

------------

sb.append(id + ";");

You deny the only point to use a StringBuilder, which is to avoid to concat strings using + :D. Therefore, and until JIT optimization handles it correctly nowadays, you are creating inner invisible StringBuilder inside your StringBuilder. On aCis we got StringUtil.append(, which is really handy for such case.

Edited July 20, 2017 by Tryskell

[melron]

....

int gradeLevel = player.getSkillLevel(239);

• will give 1..6 right? but ordinals from enum are 0..5 so i have to rework the whole thing but for sure will be more readable :P
• try/catch you mean to catch exceptions while storing the tokens?  Integer.parseInt stands there to make a type casting between String-> int right?
• Done :P
• Others are fixed :P

P.s about Id :P i dont find a reason for rename because ID in l2 is missunderstood since id means identity so , coupon's identity :P

Edited July 20, 2017 by melron

[Tryskell]

- You can store directly the skillId, that will avoid you some computation. Or you can check your enum doing skill id level -1.

- The best would be to generate an objectId directly, as I said. Id stands for identidier, and it's better to work with int rather than String (which is a costy variable).

- fontLine must use a StringBuilder with StringUtil.append (another time where this method can shine, because concat strings in a for loop is the devil).

- following strings should be "private static final" and set out of the method, for reusability.

+           final String selectedItem = "<FONT COLOR=\"LEVEL\">";
+           final String fontForRareItem = "<FONT COLOR=\"FF9988\">";
+           final String fontEnd = "</FONT>";

Edited July 20, 2017 by Tryskell

[pirama]

inside a for loop, Elmoreden gods killed Pirama. So yeah, that explains why he is so angry. Edit your for loop for enhanced version :

i can't understand what you mean ( low english ) but ( Pirama != pirama ) like ( getvalue != getValue() )

 

i can't understand what help can give you this

+       boolean already = true;
+       while (already)
+       {
+           for (Coupon cpn : coupons)
+           {
+               if (cpn.getCouponId().equalsIgnoreCase(couponId))
+               {
+                   couponId = getCouponId(category);
+                   break;
+               }
+           }
+          
+           already = false;
+       }

i think you don't need while on this step ( or if you need it , sure is without this checks ) 

i don't see all the code but loop don't work with this checks

[melron]

i can't understand what you mean ( low english ) but ( Pirama != pirama ) like ( getvalue != getValue() )

 

i can't understand what help can give you this

 

+       boolean already = true;
+       while (already)
+       {
+           for (Coupon cpn : coupons)
+           {
+               if (cpn.getCouponId().equalsIgnoreCase(couponId))
+               {
+                   couponId = getCouponId(category);
+                   break;
+               }
+           }
+          
+           already = false;
+       }

i think you don't need while on this step ( or if you need it , sure is without this checks ) 

i don't see all the code but loop don't work with this checks

It's just a check for the string if already picked but it could be written with do-while instead of while to remove the Boolean. Ty

[Tryskell]

It's just a check for the string if already picked but it could be written with do-while instead of while to remove the Boolean. Ty

 

The do/while is unecessary on this case, you can leave "while" (do/while assures at least one run, but anyway your code will assure one loop). You still have to use a condition anyway to leave the infinite loop.

 

Something which is wrong and buggy : you generate a coupondId in the for loop, meaning if you successful found an existing one, and it generates anew during the for loop, it won't check the ALREADY CHECKED stuff. So it can actually passes sucessfully the test even if there is already an existing id.

 

Exemple : you generate a couponId. It already exists, but you go through the for loop, find it exists on the 5678th spot. You regen it, break the loop, put "already" to false and say it's good. But nothing told you this new generated id is correct.

 

Simply use IdFactory (3rd time I say it) you are sure to don't be bothered by such question. :D.

Edited July 21, 2017 by Tryskell

[Rootware]

Which one?

 

the idea actually came from here  :P

 

I don't remember now. It was the long times ago when i wrote quests for aCis. Also, this system seems like event/quest with urn in MoS. Anyway idea is not a new.  :)

[melron]

I don't remember now. It was the long times ago when i wrote quests for aCis. Also, this system seems like event/quest with urn in MoS. Anyway idea is not a new.  :)

Anyway idea is new for me :)

 

Update:

 

• couponId is now integer with idFactory next id.
• couponName is generating with the couponId as base
• grade store/restore changed from ugly level checks to skill 239 level.
• renamed names of some methods
• Ty tryskell.
• table coupons now storing coupon id instead of coupon name
• removed the 'while' useless check ty pirama for remind it ;)

Edited July 21, 2017 by melron

[xxdem]

I wouldn't use idFactory, coupons should be generated through a unique Seed stored in your DB exclusively for coupon codes

[melron]

I wouldn't use idFactory, coupons should be generated through a unique Seed stored in your DB exclusively for coupon codes

 

why to make something new while idFactory is there? I found it great idea since it's generating just id's

[xxdem]

why to make something new while idFactory is there? I found it great idea since it's generating just id's

 

idFactory generates sequential IDs, you need something more random, you can use its IDs as seeds btw, but it will consume available IDs from server objects so you still need a workaround.

Edited July 21, 2017 by xxdem