Extract service logic from controllers #450
Conversation
There was a problem hiding this comment.
It looks pretty good to me. I'm really happy that we are properly dividing the app logic and separating concerns according to the good principles were learning from our professors. Thank you @jerodw!
There are some improvements I would still make as discussed in the comments. Additional code duplication to pull out, and a few organizational choices.
src/main/java/edu/byu/cs/controller/InternalServerException.java
Outdated
Show resolved
Hide resolved
|
How's this? (I tried but couldn't think of a better name 🤷🏻♂️) |
|
Just checking in, @frozenfrank @ThanGerlek do we have an update on this PR? |
@webecke Thanks for checking in. I just realized I still have a pending review comments I'll review it again now. |
This is a novel move in this repository, and it is not applied to other packages as well. In this case, there are a large number of controller classes intermingled with multiple exception classes which warrants additional organizational layers. Other packages that have *both* fewer classes and fewer exceptions are fine to remain.
There was a problem hiding this comment.
I have a few adjustments that I've developed locally and will push the branch.
My main comment is an important comment, but I feel it will be appropriate to address in a separate issue. See #476.
| private static final Logger LOGGER = LoggerFactory.getLogger(CasService.class); | ||
| public static final String BYU_CAS_URL = "https://cas.byu.edu/cas"; | ||
|
|
||
| public static User callback(String ticket) throws InternalServerException, BadRequestException, DataAccessException, CanvasException { |
There was a problem hiding this comment.
Building the entire system on static methods is contrary to the best practices we teach the students in CS 240. Yes, storing an instance and passing it through each level adds lines of code at each level, but the marginally small change provides big benefits in terms of testing and overall software design. (Do I need to describe more why this is a valuable pattern?)
I'm not sure whether or not the DAO classes are used statically as well. If they're not, we should update those as well. However, if we're introducing (or at least emphasizing) the service layer classes now, then we should start off strong by following good practices.
There was a problem hiding this comment.
This issue is now represented by #476.
frozenfrank
added
the
clean-up
|
@ThanGerlek Are you planning on merging this PR? On this team, we like to let the original author of the PR press the "Merge" button. Personally, I feel like the tradition leads to hiccups and additional delays, but it is how it is right now. (You may need to re-update the branch since other commits may have been added to |
|
I was, but I'm complete swamped, so I haven't had time to review your changes or the merge from main that still needs to happen. If you want to ping Dallin or Michael to review your changes, they or you can merge with main and then merge in the PR |
|
Honestly, if you're confident that your changes are small enough, I'd be willing to trust your judgement if you want to go ahead and merge without review. Just be careful obviously |
There's no rush, I just saw that you had updated the branch, and I expected the branch to be merged at the same time. Just review it when you have a moment 👌 |
Converts the "endpoint" Controllers (i.e. excluding
TrafficControllerandWebSocketController) into the beginnings of a Handler layer, and moves most of the logic into Service classes.This should improve testability and compliance with SRP, Demeter, and DRY. It also isolates and simplifies JavaSpark code, which will make it much easier to migrate to Javalin.