Skip to content
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

Multiple quality improvements - squid:S00117, squid:S00116 #30

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Multiple quality improvements - squid:S00117, squid:S00116 #30

wants to merge 1 commit into from

Conversation

m-ezzat
Copy link
Contributor

@m-ezzat m-ezzat commented May 17, 2016

This pull request is focused on resolving occurrences of Sonar rules squid:S00117 - Local variable and method parameter names should comply with a naming convention
squid:S00116 - Field names should comply with a naming convention

You can find more information about the issues here:
https://dev.eclipse.org/sonar/coding_rules#q=squid:S00117
https://dev.eclipse.org/sonar/coding_rules#q=squid:S00116

Please let me know if you have any questions.

M-Ezzat

@m-ezzat
Copy link
Contributor Author

m-ezzat commented Jun 28, 2016

I am not sure whether you do not agree with these changes or not. Please close this PR if not.

@@ -79,15 +79,14 @@ public UserMailboxHttpService(ApiKeyService apiKeyService, UserPluginConfig conf
public void listenMails(RakamHttpRequest request) {
RakamHttpRequest.StreamResponse response = request.streamResponse();

List<String> apiKey = request.params().get("api_key");
Copy link
Member

Choose a reason for hiding this comment

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

you can't basically change string values, it will break the api.

@m-ezzat
Copy link
Contributor Author

m-ezzat commented Jun 29, 2016

Oh, yep, this may be happened after fixing the conflicts

@buremba
Copy link
Member

buremba commented Jun 29, 2016

You also make some fields static. I think it's hard to guess which variables should be static with automation tools because some of the fields are instance fields because of thread-safety issues. I would rather to keep them as they are.

@m-ezzat
Copy link
Contributor Author

m-ezzat commented Jun 29, 2016

Ok, reverted

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.

2 participants