[#417] refactor: Eliminate raw use of parameterized class #891
Conversation
cchung100m
changed the title
Codecov Report
@@ Coverage Diff @@
## master #891 +/- ##
============================================
+ Coverage 55.24% 57.08% +1.84%
Complexity 2201 2201
============================================
Files 333 313 -20
Lines 16449 14089 -2360
Branches 1307 1307
============================================
- Hits 9087 8043 -1044
+ Misses 6851 5606 -1245
+ Partials 511 440 -71
... and 20 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -32,7 +32,7 @@ public static <T> T retry(RetryCmd<T> cmd, long intervalMs, int retryTimes) thro | |||
| } | |||
|
|
|||
| public static <T> T retry(RetryCmd<T> cmd, long intervalMs, int retryTimes, | |||
| Set<Class> exceptionClasses) throws Throwable { | |||
| Set<Class<? extends Object>> exceptionClasses) throws Throwable { | |||
There was a problem hiding this comment.
I believe Class<? extends Throwable> is preferred.
There was a problem hiding this comment.
Hi @advancedxy
Thanks for the kind reminder, I updated the part you mentioned.
| private Response handlerRequest( | ||
| Callable<Response> function) { | ||
| Response response; | ||
| private <T> Response<T> handlerRequest( |
There was a problem hiding this comment.
It might be more suitable to make BaseServlet a generic class BaseServlet<T>?
@xianjingfeng what do you think?
There was a problem hiding this comment.
Currently, it is allowed that the same servlet returns different data types.
There was a problem hiding this comment.
How about Response<Object> or Response<?> ?
There was a problem hiding this comment.
Currently, it is allowed that the same servlet returns different data types.
Yeah, I get the flexibility. However, most of the servlets just return the same data type?
There was a problem hiding this comment.
No rules,We can implement all REST api through one servlet.
There was a problem hiding this comment.
We can implement all REST api through one servlet.
If we are going to implement all REST API through one servlet, then there's no point to make this handlerRequest method a generic method. It should be simply defined as private Response<Object> handleRequest(...) then..
I would propose to something like this
public abstract class BaseServlet<T> extends HttpServlet {
private Response<T> handlerRequest(
Callable<Response<T>> function) {
Response<T> response;
try {
// todo: Do something for authentication
response = function.call();
} catch (Exception e) {
response = Response.fail(e.getMessage());
}
return response;
}
protected Response<T> handleGet(
HttpServletRequest req,
HttpServletResponse resp) throws ServletException, IOException {
...
}
protected Response<T> handlePost(
HttpServletRequest req,
HttpServletResponse resp) throws ServletException, IOException {
...
}
}
For concrete servlet which handles and returns only one type should implement it with a concrete type.
For servlet that might return multiple types could defined it as
public class AnyReturnServlet extends BaseServlet<Object> {
...
}
There was a problem hiding this comment.
It is ok for me.I don't care too much about this. Because maybe we are going to use jersey. #864
There was a problem hiding this comment.
Thank you for sharing your thoughts and active feedback. I am open to whether make BaseServlet a generic class or not. Willing to do the change if we have a conclusion.
There was a problem hiding this comment.
I am open to whether make BaseServlet a generic class or not. Willing to do the change if we have a conclusion.
Let's make it a generic class first, then we could replace it with jersey
…orrect indentation level 4, expected level should be one of the following: 6, 8, 10. [Indentation]
… Class<? extends Throwable>
… HashMap<String, Object>
…ject>' with ConcurrentHashMap<Obejct, Object>
…orrect indentation level 4, expected level should be one of the following: 6, 8, 10. [Indentation]
cchung100m
force-pushed
the
refactor_417_raw_use_of_parameterized_class
branch
from
4d39124
to
9283469
Compare
jerqi
changed the title
Hi @zuston @advancedxy
Any comments and suggestions would be appreciated if you are available, thank you.
What changes were proposed in this pull request?
Eliminate
raw use of parameterized classWhy are the changes needed?
Fix: #417
Does this PR introduce any user-facing change?
No.
How was this patch tested?
current UT