search

splunk / minecraft-app

Splunking Minecraft with the App Framework
Apache License 2.0
20 stars 9 forks source link

Feature/forge port #5

Closed alszeb closed 9 years ago

alszeb commented 9 years ago

Uses forge and not bukkit.

mzeb commented 9 years ago

High level: Style is good. Commenting is excellent (Thank you for this). MultiSplunkConnection is NYI, remove it for now. Pull it into a different branch and release this as single splunk only You have SplunkConnector as the base class for two “Connections”. Pick “connector” or “connection”. The second makes more sense to me. Change the name of the MessagePreparer to something not including “Original”. It implies some point in time and usefulness. Change it to “Basic” or “String” or something along those lines. Your code is hitting an average of about 50 lines per class. That’s pretty low. JUnit (which is considered highly factored) is about 70 lines. Apache code averages about 250. It means you likely have some common functionality that could be merged. Not saying you have to change it but worth considering. There is a 1-1 Mapping of EventAction, Logger, and LoggableEvents (mostly, player move is odd). Can the enums be embedded in the EventAction? I realize you access them but the fact that the enum is used in both classes makes things a bit highly coupled. Can we make the graph of classes using the enum linear instead of looped?

alszeb commented 9 years ago

@mzeb I think looking at how factored the code is is an interesting point. Do you think there will be issues with maintainability/readability? This basic structure is written with the expectation that we will end up adding a bunch more into the already existing classes - although this may be too forward thinking. I plan to move things to having a single subscriber that delegates the calls (