-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add kc command #4067
Add kc command #4067
Conversation
{ | ||
if (!response.isSuccessful()) | ||
{ | ||
throw new IOException("Unable to look up killcount!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably just return 0 here, maybe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree, a 0
is different to "I couldn't get a value" - Though I would update the usage of this on ChatCommandPlugin:290 so that when the IOException
is thrown, it lets the user know in their chatbox, rather than, from a user perspective, failing silently.
@Value | ||
class KillCountKey | ||
{ | ||
String username; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Access modifiers? private final maybe. Even though lombok generates most, i am not sure, I think this should still have some
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Value
will generate all fields as private final.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make this private final anyway as that is how we do it at other places and this could just throw false warnings in IDE.
|
||
String response = new ChatMessageBuilder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final
} | ||
catch (IOException ex) | ||
{ | ||
log.debug("unable to lookup killcount", ex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably use 0 here, maybe. Not sure how we handle such cases with !price commands
|
||
private void killCountLookup(ChatMessageType type, SetMessage setMessage, String search) | ||
{ | ||
String player; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final
player = sanitize(setMessage.getName()); | ||
} | ||
|
||
int kc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final
|
||
String boss = value.substring(4); | ||
|
||
int kc = getKc(boss); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fianl
|
||
chatboxInput.setStop(true); | ||
|
||
String boss = value.substring(4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final
@Subscribe | ||
public void onChatboxInput(ChatboxInput chatboxInput) | ||
{ | ||
String value = chatboxInput.getValue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final
} | ||
} | ||
|
||
public void sendChatboxInput(int chatType, String input) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably make this something like continueWithMessage so we will not have public API for sending messages to server
int kc; | ||
try | ||
{ | ||
kc = killCountClient.get(player, search); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The UX of this means that if an IOException
occurs the user gets no feedback without checking their logs - Could we not present the user with a nicely formatted "Something went wrong" message?
{ | ||
private final String value; | ||
private final int chatType; | ||
protected boolean stop; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be protected? private seems fine
|
||
case "dusk": | ||
case "dawn": | ||
case "galio": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even though this is funny, I dont think we should use this. Galio is reference to League of Legends character that is big gargoyle
case "gargs": | ||
return "Grotesque Guardians"; | ||
|
||
case "archaeologist": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add also "crazy arch" here
case "sire": | ||
return "Abyssal Sire"; | ||
|
||
case "smoke devil": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"thermy" is another very common abbreviation
case "kril": | ||
case "kril trutsaroth": | ||
return "K'ril Tsutsaroth"; | ||
case "arma": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"armadyl" too
final int kc = getKc(boss); | ||
if (kc <= 0) | ||
{ | ||
chatMessageManager.queue(QueuedMessage.builder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont agree with not resuming the message and simply not sending them to other players. 1. that would look weird, when player sent some message to chat but nothing got displayed on other side 2. we dont do this in any other command and we just simply send it and display error message to everyone
} | ||
catch (Exception ex) | ||
{ | ||
log.warn("unable to submit killcount", ex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my previous comment
@Value | ||
class KillCountKey | ||
{ | ||
String username; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make this private final anyway as that is how we do it at other places and this could just throw false warnings in IDE.
public void resume() | ||
{ | ||
if (resumed) | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{
}
} | ||
} | ||
|
||
private void sendChatboxInput(int chatType, String input) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method can be called from different thread, maybe make it synchronized or sending volatile? Or event post it to clientThread, maybe
16e487e
to
64eca2e
Compare
No description provided.