You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2020/06/06 23:11:22 UTC
[GitHub] [fineract] thesmallstar opened a new pull request #990: FINERACT-821 Added and Enforced EmptyBlock Checkstyle
thesmallstar opened a new pull request #990:
URL: https://github.com/apache/fineract/pull/990
I had doubt on one of the change If you can review that?
I thought maybe we can log that the response code received is not 200?
Or will you recommend to just delete it?
I have removed LITERAL_CATCH since it is already covered in emptyCatchBlock checkstyle.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger merged pull request #990: FINERACT-821 Added and Enforced EmptyBlock Checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger merged pull request #990:
URL: https://github.com/apache/fineract/pull/990
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #990: FINERACT-821 Added and Enforced EmptyBlock Checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #990:
URL: https://github.com/apache/fineract/pull/990#discussion_r436992125
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/campaigns/sms/service/SmsCampaignDropdownReadPlatformServiceImpl.java
##########
@@ -70,19 +71,18 @@ public SmsCampaignDropdownReadPlatformServiceImpl(final SmsConfigUtils smsConfig
public Collection<SmsProviderData> retrieveSmsProviders() {
Collection<SmsProviderData> smsProviderOptions = new ArrayList<>();
String hostName = "" ;
- try {
Map<String, Object> hostConfig = this.smsConfigUtils.getMessageGateWayRequestURI("smsbridges", null);
URI uri = (URI) hostConfig.get("uri");
hostName = uri.getHost() ;
HttpEntity<?> entity = (HttpEntity<?>) hostConfig.get("entity");
ResponseEntity<Collection<SmsProviderData>> responseOne = restTemplate.exchange(uri, HttpMethod.GET, entity,
new ParameterizedTypeReference<Collection<SmsProviderData>>() {});
+ if (!responseOne.getStatusCode().equals(HttpStatus.OK)) {
Review comment:
I'm not 100% sure if PlatformDataIntegrityException is "correct" here, but don't really have a better idea without more closely than I have time for, and it probably doesn't really matter all that much, anyway. One fine day we should look into and then write up Guidance in the README about which exact Fineract Exception type to use in which "layer" (e.g. *Resource VS *Service). @ptuomola I doubt that's something you would fancy digging into?
##########
File path: fineract-provider/src/main/java/org/apache/fineract/organisation/teller/service/TellerManagementReadPlatformServiceImpl.java
##########
@@ -261,8 +261,10 @@ private String getTellerCriteria(final String sqlSearch, final Long officeId, fi
extraCriteria.append(" and status = 300 ");
} else if (status.equalsIgnoreCase("inActive")) {
extraCriteria.append(" and status = 0 ");
- } else if (status.equalsIgnoreCase("all")) {} else {
+ } else {
+ if (!status.equalsIgnoreCase("all")){
Review comment:
Why changing this? Just curious..
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] thesmallstar edited a comment on pull request #990: FINERACT-821 Added and Enforced EmptyBlock Checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on pull request #990:
URL: https://github.com/apache/fineract/pull/990#issuecomment-640817087
From the guidelines ->
> Either way, including the root cause by using catch (SomeException e) and then either ..
The root cause, in this case, is not an exception (If I am not wrong), we are checking the response code and then throwing an exception. It won't make much sense, in this case, to rethrow the same exception twice? or any other exception.
I hope this is a correct way
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #990: FINERACT-821 Added and Enforced EmptyBlock Checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #990:
URL: https://github.com/apache/fineract/pull/990#discussion_r439086408
##########
File path: fineract-provider/src/main/java/org/apache/fineract/organisation/teller/service/TellerManagementReadPlatformServiceImpl.java
##########
@@ -261,8 +261,10 @@ private String getTellerCriteria(final String sqlSearch, final Long officeId, fi
extraCriteria.append(" and status = 300 ");
} else if (status.equalsIgnoreCase("inActive")) {
extraCriteria.append(" and status = 0 ");
- } else if (status.equalsIgnoreCase("all")) {} else {
+ } else {
+ if (!status.equalsIgnoreCase("all")){
Review comment:
got it, thanks for clarifying!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] thesmallstar commented on pull request #990: FINERACT-821 Added and Enforced EmptyBlock Checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #990:
URL: https://github.com/apache/fineract/pull/990#issuecomment-640776749
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #990: FINERACT-821 Added and Enforced EmptyBlock Checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #990:
URL: https://github.com/apache/fineract/pull/990#discussion_r439088743
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/campaigns/sms/service/SmsCampaignDropdownReadPlatformServiceImpl.java
##########
@@ -78,8 +78,9 @@ public SmsCampaignDropdownReadPlatformServiceImpl(final SmsConfigUtils smsConfig
ResponseEntity<Collection<SmsProviderData>> responseOne = restTemplate.exchange(uri, HttpMethod.GET, entity,
new ParameterizedTypeReference<Collection<SmsProviderData>>() {});
smsProviderOptions = responseOne.getBody();
- if (!responseOne.getStatusCode().equals(HttpStatus.OK)) {
- }
+ //Better approch? Maybe log/throw exception that this happen?
+ // if (!responseOne.getStatusCode().equals(HttpStatus.OK)) {
+ // }
} catch (Exception e) {
}
Review comment:
this was done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #990: FINERACT-821 Added and Enforced EmptyBlock Checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #990:
URL: https://github.com/apache/fineract/pull/990#discussion_r436310518
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/campaigns/sms/service/SmsCampaignDropdownReadPlatformServiceImpl.java
##########
@@ -78,8 +78,9 @@ public SmsCampaignDropdownReadPlatformServiceImpl(final SmsConfigUtils smsConfig
ResponseEntity<Collection<SmsProviderData>> responseOne = restTemplate.exchange(uri, HttpMethod.GET, entity,
new ParameterizedTypeReference<Collection<SmsProviderData>>() {});
smsProviderOptions = responseOne.getBody();
- if (!responseOne.getStatusCode().equals(HttpStatus.OK)) {
- }
+ //Better approch? Maybe log/throw exception that this happen?
Review comment:
let's throw some suitable exception in this case, don't just log it (and certainly not just ignore it, like the current bad code)
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/campaigns/sms/service/SmsCampaignDropdownReadPlatformServiceImpl.java
##########
@@ -78,8 +78,9 @@ public SmsCampaignDropdownReadPlatformServiceImpl(final SmsConfigUtils smsConfig
ResponseEntity<Collection<SmsProviderData>> responseOne = restTemplate.exchange(uri, HttpMethod.GET, entity,
new ParameterizedTypeReference<Collection<SmsProviderData>>() {});
smsProviderOptions = responseOne.getBody();
- if (!responseOne.getStatusCode().equals(HttpStatus.OK)) {
- }
+ //Better approch? Maybe log/throw exception that this happen?
+ // if (!responseOne.getStatusCode().equals(HttpStatus.OK)) {
+ // }
} catch (Exception e) {
}
Review comment:
while you are already here anyway, let's get rid of this overly broad ` } catch (Exception e) {` as well? see if you can just remove it, or if not, make it catch a more specific exception, and then re-throw that. No need to log it if you rethrow. As per https://github.com/apache/fineract#error-handling-guidelines
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] thesmallstar commented on a change in pull request #990: FINERACT-821 Added and Enforced EmptyBlock Checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on a change in pull request #990:
URL: https://github.com/apache/fineract/pull/990#discussion_r437115045
##########
File path: fineract-provider/src/main/java/org/apache/fineract/organisation/teller/service/TellerManagementReadPlatformServiceImpl.java
##########
@@ -261,8 +261,10 @@ private String getTellerCriteria(final String sqlSearch, final Long officeId, fi
extraCriteria.append(" and status = 300 ");
} else if (status.equalsIgnoreCase("inActive")) {
extraCriteria.append(" and status = 0 ");
- } else if (status.equalsIgnoreCase("all")) {} else {
+ } else {
+ if (!status.equalsIgnoreCase("all")){
Review comment:
Else if block is empty for condition (status.equalsIgnoreCase("all")), this is not supported.
But as else if block works, we cannot simply remove it, or even if the (status.equalsIgnoreCase("all")) value is true it will go in else.
We don't want that, so I have added a extra check in else block to make sure (status.equalsIgnoreCase("all")) is not true, and only then perform else action that what we received is incorrect and act on it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org