Problem
I was asked to provide a codebase for the following exercise. I have provided the requirement of the problem and also putting down the code of two main classes. Can you please review and provide your input on improvement and also point out any issue or general feedback?
Java Service capable of posting GitHub events to Slack including: issue requests, issue updates, branch creating, commit, pull requests.
Bonus features integrated:
Bonus 1
Associate GitHub user account with their corresponding slack handle i.e. “@johnathan added a new comment to issue #ISSUE43”
Bonus 2
Implement a queuing mechanism to prevent/handle connectivity failures between your process and posting to Slack.
Solution
/*
Created two main classed 1) for accepting the request 2)And SlackService
*/
This class is like a controller class which accepts the json payload to push an event to Slack.
public class GitSlackRequestController {
SlackService slackService = new SlackService();
/**
* The method for posting a message on a slack channel.
*
* It will validate and authenticate a request .
*
* @param reuqestBody the payload example
*
* { gitUserName:"test", gitOperation:"COMMIT"; opertaionDescriptiveText:
* "Commit on Jan 13,2014 on the master branch"; slackChannel:"devGroupProject1",
* "secureToken","xsdrt" (it can be part of header like in case HTTP) }
*
* @return respone in JSON format
*/
public String postEventToSlack(String reuqestBody) {
String response = null;// response in JSON
SlackChannelRequest slackRequest = null;
// convert request payload to a java object
try {
slackRequest = convertJsonToObject(reuqestBody);
} catch (Exception e) {
return createRespone("Not a valid paylod request =" + e.getMessage(), true);
}
if (!isValidRequest(slackRequest)) {
return createRespone("Not a valid request", true);
}
if (!isAuthenticateAndAuthorizeRequest(slackRequest)) {
return createRespone("Not a valid user to perform this operation", true);
}
// bonus 1
slackRequest.setSlackUserName(getSlackUserForGitUser(slackRequest.getGitUserName()));
slackService.postMessage(slackRequest);
response = createRespone("Sucessfully subitted request to post the mesasge", false);
return response;
}
private String getSlackUserForGitUser(String gitUseNamer) {
// GIT Profile API or local cache to get if any slack acocunt associated with a git account.
// for demo assume gituser and slackuser are same
return gitUseNamer;
}
private SlackChannelRequest convertJsonToObject(String reuqestBody) throws Exception {
SlackChannelRequest slackRequest = new SlackChannelRequest();
// json library code here to convert or throws exception if not valid request
//for demo manually creating it
slackRequest.setGitoperation(GitOperation.COMMIT);
slackRequest.setOpertaionDescriptiveText("commit happend on the branch");
slackRequest.setGitUserName("userabc");
slackRequest.setSecureToken("xdf");
return slackRequest;
}
private String convertResponseToJson(Response response) {
// code to convert object to json
return " Json Conversion of Response";
}
private boolean isValidRequest(SlackChannelRequest slackRequest) {
// validation logic
if (slackRequest.getGitoperation() == null
|| slackRequest.getOpertaionDescriptiveText() == null)
return false;
return true;
}
private String createRespone(String message, boolean isError) {
Response responseObj = new Response();
if (isError) {
responseObj.setStatusCode(StatusCode.ERROR);
} else {
responseObj.setStatusCode(StatusCode.OK);
}
responseObj.setResponseMessage(message);
String response = convertResponseToJson(responseObj);
return response;
}
private boolean isAuthenticateAndAuthorizeRequest(SlackChannelRequest slackRequest) {
// the token should be generated before a differen call before submitting
if (slackRequest.getSecureToken() == null)
return false;
// it should validate this here ideally it should be in interceptor instead of here
return true;
}
}
public class SlackService {
int MAX_CONCURRENT_SLACK_TASKS = 1;
BlockingQueue<SlackChannelRequest> postMessageQueue =
new LinkedBlockingQueue<SlackChannelRequest>();
ExecutorService pool = null;
public SlackService() {
pool = Executors.newFixedThreadPool(MAX_CONCURRENT_SLACK_TASKS);
pool.submit(new SlackTask(postMessageQueue));
}
public void postMessage(SlackChannelRequest slackRequest) {
postMessageQueue.add(slackRequest);
}
protected void finalize() throws Throwable {
pool.shutdown();
}
}
Solution
Use more helper methods
Instead of calling createResponse with true or false as second parameter, it would be more natural to create helper methods createErrorResponse and createNormalResponse. These methods could pass the status code directly to createResponse, and eliminate the if-else in that method, like this:
private String createErrorResponse(String message) {
return createResponse(message, StatusCode.ERROR);
}
private String createNormalResponse(String message) {
return createResponse(message, StatusCode.OK);
}
private String createResponse(String message, StatusCode statusCode) {
Response responseObj = new Response();
responseObj.setStatusCode(statusCode);
responseObj.setResponseMessage(message);
return convertResponseToJson(responseObj);
}
Make fields final
Whenever possible, it’s good to make class member fields final. It protects you from accidental reassignment, and knowing that something will always have its initial value can make it easier to understand a program. For example, all fields in the slack service class can be final.
Inline unnecessary local variables
In postEventToSlack, String response is declared at the top of the method,
but not used until near the end, where you assign to it a value and return it.
In fact you never needed this variable, you could return the response directly.
return createNormalResponse("Sucessfully subitted request to post the mesasge");
In other methods too, you can eliminate local variables that are assigned just before the return statement.
Return boolean values directly
Instead of wrapping in an if-else,
you can return boolean values directly.
private boolean isAuthenticateAndAuthorizeRequest(SlackChannelRequest slackRequest) {
if (slackRequest.getSecureToken() == null)
return false;
return true;
}
private boolean isValidRequest(SlackChannelRequest slackRequest) {
if (slackRequest.getGitoperation() == null
|| slackRequest.getOpertaionDescriptiveText() == null)
return false;
return true;
}
Like this:
private boolean isAuthenticateAndAuthorizeRequest(SlackChannelRequest slackRequest) {
return slackRequest.getSecureToken() != null;
}
private boolean isValidRequest(SlackChannelRequest slackRequest) {
return !(slackRequest.getGitoperation() == null || slackRequest.getOpertaionDescriptiveText() == null);
}
Formatting
I suggest to copy-paste the code into an IDE like IntelliJ/Eclipse/Netbeans and use the auto-reformat function.
Typos
There are too many typing errors in variable and method names. Making a few mistakes can be normal, but making a lot of them may be perceived as lack of attention to detail.
Bonus 1
The posted code doesn’t do much for this feature. It’s practically just a placeholder method with a trivial implementation. Don’t expect much credit for this.
Bonus 2
The posted code doesn’t do nearly enough for managing a queue of slack tasks, though there are some good elements.
It’s good that you used an executor service, and it’s on the right track to manage a queue in a thread safe manner.
And it’s good that you are aware of blocking queue and its implementations, but you haven’t posted the code that uses its features. If you used the take method to remove elements when processing the queue by the executor then that makes sense.
Although the queue is unbound that seems reasonable, as it wouldn’t make much sense to use this program with a github feed that can potentially exhaust the memory with queued messages. But an explanation of this in comments may have improved your solution.
Overriding finalize is not a good way to clean up on shut down. The Java specification gives no guarantees about the timely execution of this method, or even that it will be executed at all. This method should almost never be used. You need to find another mechanism to cleanly shut down your service.
Overall, the code doesn’t really do enough to earn the credit for the bonus item 2.