fix(general): clean up typos and code #62
Conversation
Changes: * Fix some of the markdown used * Removed unused imports * LambdaHandlerProcessor - Make null check easier to read * LambdaJsonLayout - Remove unused objectMapper * Fix typos
|
Thanks for the PR Michael, just looking at this now. |
There was a problem hiding this comment.
@michaelbrewer Thanks for updates to some of the docs and typos. Muck appreciated:) Just one suggestion.
| @@ -22,7 +22,7 @@ | |||
| import org.aspectj.lang.ProceedingJoinPoint; | |||
|
|
|||
| public final class LambdaHandlerProcessor { | |||
| private static String SERVICE_NAME = null != System.getenv("POWERTOOLS_SERVICE_NAME") | |||
| private static String SERVICE_NAME = System.getenv("POWERTOOLS_SERVICE_NAME") != null | |||
There was a problem hiding this comment.
In order to keep it consistent with how we will like to check for null, I will prefer if we keep constant(null) to the left.
There was a problem hiding this comment.
Ok sure, i can revert this, just confusing to read left to right.
Maybe this would be cleaner and work with java8
private static String SERVICE_NAME = Optional.ofNullable(System.getenv("POWERTOOLS_SERVICE_NAME")).orElse("service_undefined")There was a problem hiding this comment.
There was a problem hiding this comment.
@pankajagrawal16 SERVICE_NAME and IS_COLD_START look like constants, but are actually mutable due to the unit tests requiring this to be mutable. Maybe we should change either the unit tests or these misleading names.
There was a problem hiding this comment.
Ok sure, i can revert this, just confusing to read left to right.
Maybe this would be cleaner and work with java8
private static String SERVICE_NAME = Optional.ofNullable(System.getenv("POWERTOOLS_SERVICE_NAME")).orElse("service_undefined")
We have intentionally avoided use of optional here considering its a simple null check which belongs to an internal class.
I am a big fan of functional interfaces in java myself and have to always find a balance when I would like to use it.
Prefer using optional on public API that is public to let users ability to write functional code and not worry about nulls.
But these are just personal preferences too. Considering this being a internal class, I will still be happy to leave it the way it was. Thanks for the input. 🙂
There was a problem hiding this comment.
@pankajagrawal16 - ok i reverted back to the original code.
|
@pankajagrawal16 - maybe where possible we make things Maybe there anti-bike shedding type of autoformatter do java like "black" for python or "ktlint" for kotlin. To clean up imports and naming conversions. |
Having autoformatter sounds like a good idea, but I will suggest please open a RFC for it so that we can track it. As far as client of the library is concerned, it doesn't impact them directly, but definitely worth evaluating at some point. |
Issue #, if available:
Description of changes:
Changes:
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.