why are you implementing Serializable
|
TimedTaskType.java
[Only registered and activated users can see links. ]
TimedTaskReward.java
[Only registered and activated users can see links. ]
TimedTaskManager.java
[Only registered and activated users can see links. ]
AbstractTimedTask.java
[Only registered and activated users can see links. ]
TimedTaskInterfaceManager.java
[Only registered and activated users can see links. ]
all these bois get touched like seafood
Example usage:
Code:package com.rs.game.player.content.timedtasks.week; import com.rs.game.item.Item; import com.rs.game.player.content.timedtasks.AbstractTimedTask; /** * @author Tyluur <[email protected]> * @[Only registered and activated users can see links. ] 2018-12-26 */ public class RuniteOreWeeklyTimedTask extends AbstractTimedTask { private static final long serialVersionUID = 8547553057115818247L; @Override protected String title() { return "Mine @[email protected] rune ores"; } @Override public int targetAmount() { return 1_000; } @Override public Item[] rewards() { return new Item[] { new Item(995, 1_000_000) }; } }
why are you implementing Serializable
Why would anyone want to represent the TimedTask as a sequence of bytes? It doesn't make sense.
Also the enum here: [Only registered and activated users can see links. ]
I believe you should represent this DATA not in a enum but as flat file maybe JSON. The fact that you use a enum to represent this data means if someone wanted to add something like YEAR to that enum they'd be modifying current smelly code inside TimeTaskManager and making it smell more due to using a enum.
Fishy indeed
.e.g. Tyluur says to his manager i want to add YEARLY TimeTaskType. Tyluur tells his manager its gonna take 2 weeks, his manager asks why? Tyluur says he has to modify a bunch of if statements after he adds a new item to the enum, to support this change.
You can submit multiple files in a single gist.
[Only registered and activated users can see links. ] your giveTasks method is pretty bloated and could be simplified. The method is private so based on ur implementation the only possible input to the method is DAY/WEEK. As a result, the code
Can be simplified/shortened by changing WEEK.pickRandomReward()-> day.pickRandomReward(). You can also do that with the other methods as well so that you dont just have an if/else with mostly identical code.Code:private void giveTasks(TimedTaskType day) { if (day == TimedTaskType.DAY) { int amount = Utils.random(DAY.minimumAmount(), DAY.maximumAmount()); long overAt = System.currentTimeMillis() + DAY.getTime(); List<AbstractTimedTask> tasks = generateRandomTasks(DAY, amount); tasks.forEach(task -> task.setOverAt(overAt)); timedTasks.removeIf(task -> task.getDurationType() == DAY); timedTasks.addAll(tasks); rewardMap.put(DAY, DAY.pickRandomReward()); lastDayRecieveDate = new Date(); } if (day == TimedTaskType.WEEK) { long overAt = System.currentTimeMillis() + WEEK.getTime(); int amount = Utils.random(WEEK.minimumAmount(), WEEK.maximumAmount()); List<AbstractTimedTask> tasks = generateRandomTasks(WEEK, amount); tasks.forEach(task -> task.setOverAt(overAt)); timedTasks.removeIf(task -> task.getDurationType() == WEEK); timedTasks.addAll(tasks); rewardMap.put(WEEK, WEEK.pickRandomReward()); lastWeekReceiveDate = new Date(); } }
Also TimedTaskType day is a pretty terrible name for the parameter in this case.
garbage take. it is far too easy to store things in a readable format these days for someone to use serialization for something they will store locally. in what world would you want to work with binary blobs over something more readable. it will be a pain to refactor, and you will have binary blobs sitting there that are of no use to query/look at/graph whatever (well unless of course you make special tooling but then...). why in the world would you store things as serialized objects this is 2021 people are literally spoonfeeding you better ways.
scenario: you need to refactor a timed task or remove a timed task type.
OT: Not the way I would do this. Not a fan of relying on package names? to determine which classes belong to which task type. You keep digging yourself deeper and deeper into a pigeonhole when this is supposed to be "abstract". What about a task that happens every 4 hours, or whatever? Why do I need to add a bunch of infrastructure changes to support that? Why is the abstract task forcing you to provide a list of items (I get you can override the giveRewards, but still, why am I forced to implement rewards()). Why not have AbstractTask (everything) -> ItemRewardTask (for convenience, base class for those item reward ones). Your enum also has item rewards... like why are item rewards so heavily integrated into this for no reason.
I would reccomend you take a step back and think "what is the engine here". The answer is: something that will assign/do something to each player based on some "clock time". First make that and then worry about the task stuff secondarily. Right now that is already poorly written and making your life difficult.
« [Any/Kronos] JSON Equip Weapon/Armor Sounds | [Any RuneLite/Kronos] Adding Your Own Chat Crowns via Cache » |
Thread Information |
Users Browsing this ThreadThere are currently 1 users browsing this thread. (0 members and 1 guests) |