FINERACT-2169: SMS API refactoring and clean up; #4353
Conversation
VictorPavfurious
force-pushed
the
FINERACT-2169-SMS-API
branch
2 times, most recently
from
400f3a4 to
87e6f6d
Compare
|
|
||
| @GET | ||
| public String retrieveAll(@Context final UriInfo uriInfo) { | ||
| public Collection<SmsData> retrieveAll() { |
There was a problem hiding this comment.
Should we use List instead?
There was a problem hiding this comment.
Yes, you are right, better use List
...src/main/java/org/apache/fineract/infrastructure/sms/service/SmsReadPlatformServiceImpl.java
Show resolved
Hide resolved
| this.jdbcTemplate = jdbcTemplate; | ||
| this.sqlGenerator = sqlGenerator; | ||
| @PostConstruct | ||
| public void init() { | ||
| this.businessRuleMapper = new BusinessRuleMapper(sqlGenerator); |
There was a problem hiding this comment.
Should we just do the initialization at field definition?
private final SmsCampaignMapper smsCampaignMapper = new SmsCampaignMapper();
private final BusinessRuleMapper this.businessRuleMapper = new BusinessRuleMapper(sqlGenerator);
There was a problem hiding this comment.
@adamsaghy what do you think about make this SmsCampaignMapper and BusinessRuleMapper like bean Component and inject via construct with using DI?
I think it will be better
There was a problem hiding this comment.
I reckon that also works.
There was a problem hiding this comment.
Do you mean that ?
private final SmsCampaignMapper smsCampaignMapper = new SmsCampaignMapper();
private final BusinessRuleMapper this.businessRuleMapper = new BusinessRuleMapper(sqlGenerator);
I think yes, but i'd want to avoid NPE (on sqlGenerator) and make architecture more clear and testability with using DI
There was a problem hiding this comment.
i meant, I dont mind if you create beans from those classes..
VictorPavfurious
force-pushed
the
FINERACT-2169-SMS-API
branch
3 times, most recently
from
b942e52 to
be65e8c
Compare
VictorPavfurious
force-pushed
the
FINERACT-2169-SMS-API
branch
from
be65e8c to
9d4ed34
Compare
Description
Describe the changes made and why they were made.
Ignore if these details are present on the associated Apache Fineract JIRA ticket.
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
Write the commit message as per https://github.com/apache/fineract/#pull-requests
Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
Create/update unit or integration tests for verifying the changes made.
Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)
FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.