Skip to content

Conversation

@Agilitis
Copy link

…e TFW server, and has other good utilities

Copy link
Contributor

@therealkrispet therealkrispet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added comments to parts that require changes, but the overrall code quality is nice.

Copy link
Contributor

@therealkrispet therealkrispet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rewrite commit messages to be imparetive, short and start with an uppercase letter

}
}

send(key, data){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MessageSender should not be used to send data, only instructions (clients should use a naked TFWConnector for that (this method should be implemented in TFWServerConnector)

}
}

sendToEventHandlers(message){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MessageSender should not be used to send data, only instructions (clients should use a naked TFWConnector for that (this method should be implemented in TFWServerConnector)

PUSH_ADDRESS = "tcp://localhost:" + PUSH_PORT;


class TFWConnector {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be named TFWServerConnector to harmonize with python, cpp and java implementations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MessageSender.send() and MessageSender.sendToEventHandlers() methods should be implemented here as mentioned above (MessageSender is only meant to write instructions).


class Utilities{

constructor(messageSender){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use a TFWServerConnector instance, not a MessageSender, and should initialise one if the user does not provide one as a parameter.


class MessageSender{

constructor(connector){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

connector should be optional, and if the user does not provide one it should be initialised.

Agilitis added 2 commits July 5, 2018 10:14
Rename mistyped Connector class
Add comment on to_state() method to warn users
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants