Refactor internal/character directory #671
Conversation
| return nil | ||
| } | ||
|
|
||
| // Cast a Blizzard on very close mobs, in order to clear possible trash close the player, every two attack rotations |
There was a problem hiding this comment.
Eh no .. why all character would get blizzard sorceress logic from what supposed to be base logic?
There was a problem hiding this comment.
Right. That should be removed and have the parent class just use step.PrimaryAttack.
| @@ -130,39 +129,6 @@ func (s LightningSorceress) shouldCastStaticField(monster data.Monster) bool { | |||
| return hpPercentage > LightningStaticFieldThreshold | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Removing static specific for light sorc logic?? Isnt same as blizz and fire
There was a problem hiding this comment.
I don't understand the difference other than lightning sorceress's usually have higher -LR than a blizzard sorceress. However, that is irrelevant, because it is very possible that a blizzard sorcress has the same -LR (e.g. wearing Griffon's Eye, lightning Rainbow Facets, or Infinity Runeword). Why does lightning_sorceress.go use magic numbers of 1 and 3 for the distance of static field rather than actual distance of the skill?
Furthermore, why is there a LightningStaticFieldThreshold? Shouldn't static field be cast until it would do less damage than the primary skill?
There was a problem hiding this comment.
Now how will foh use its own killboss logic? Its removed and defaulting to killmonstersequence
There was a problem hiding this comment.
If FOH truly has special logic for bosses than those can be replaced. I did not test all 14 current builds. Thank you for your review, sir.
There was a problem hiding this comment.
How is it going to use killbosswithstatic now
There was a problem hiding this comment.
I notice now that nova and lightning sorcress builds share this method. I just am not quite sure how to implement it in the parent (since I never programmed in Go before), or if that is even what the maintainers would like to do. So I was first just proposing this rough change before committing to a proper cleanup/refactor.
There was a problem hiding this comment.
Ok when submitting untested things/suggestion pr can be made as a “draft” to avoid confusion thanks
|
The main issue is that most character builds tackle situations differently. Every method needs to be overriden. Also, currently, there are a few character builds that are quite generic and don't work very well. You would end up with massive catch all methods in a parent class. What you're probably looking for is the concept of interfaces. |
I am new, go easy on me.
BlizzardSorceress inherits from its parent CharacterBuild which inherits from its parent BaseCharacter. Maybe this is not exactly how the maintainers would like to refactor it, but I hope this thought is helpful.
I reduced the character folder size by ~900 lines, but this isn't about saving lines--the main point is that if you expect to add more builds in the future, they all should share common methods such as KillMonster or KillDiablo; that's how inheritance works.
I also noticed that some methods that were copy-pasta'd between all 14 of the character builds has slight differences. Some of these differences were just a comment, some seem trivial, and some I removed, so some functionality is probably broken.
All of the classes would need to be tested and some minor changes are necessary before pulling this, but if you want to keep improving the codebase, more refactoring needs to be done.