GH-534 Add ktlint for Kotlin and refactor codebase to utilize Kotlin idioms #536
Conversation
| val messageSource = ReloadableResourceBundleMessageSource() | ||
| messageSource.setBasename("classpath:messages") | ||
| messageSource.setDefaultEncoding("UTF-8") | ||
| return messageSource | ||
| return ReloadableResourceBundleMessageSource().also { | ||
| it.setBasename("classpath:message") | ||
| it.setDefaultEncoding("UTF-8") | ||
| } |
There was a problem hiding this comment.
Kotlin has a set of Scope Functions which are neat and especially useful for initialization or modification on newly created objects which makes the code easier to grasp.
| return ResponseEntity.ok(mapOf("access_token" to authenticationTokenCreator.create(authentication))) |
There was a problem hiding this comment.
Java's Collections.singletonMap may be replaced with a mapOf(x to y). This also removes the explicit generic arguments.
| import javax.servlet.http.HttpServletRequest | ||
| import javax.servlet.http.HttpServletResponse | ||
|
|
||
| internal class AuthenticationEntryPoint : AuthenticationEntryPoint { | ||
|
|
||
| @Throws(IOException::class) |
There was a problem hiding this comment.
Kotlin code does not require @Throws tags for anything and I believe the app should run and compile fine without them being present.
These tags are only used if third-party java code is written using the compiled kotlin jar, something which is probably not going to happen as this is a standalone project.
| data class OAuth( | ||
| var redirectUrls: List<String?> = ArrayList() | ||
| } | ||
| ) |
There was a problem hiding this comment.
These are POJOs, which can and should be moved into data classes
| if (authorizationHeaderContent != null && authorizationHeaderContent.startsWith(bearer)) { | ||
| return authorizationHeaderContent.substring(bearer.length) | ||
| return if (authorizationHeaderContent != null && authorizationHeaderContent.startsWith(bearer)) { | ||
| authorizationHeaderContent.substring(bearer.length) | ||
| } else { | ||
| null | ||
| } | ||
|
|
||
| return null |
There was a problem hiding this comment.
May lift the if into the return
| user = if (user != null) { | ||
| if (providerName != user.provider) { | ||
| throw RuntimeException("Looks like you're already signed up with " + user.provider + " account.") | ||
| } | ||
|
|
||
| user = updateExistingUser(user, userDetails) | ||
| updateExistingUser(user, userDetails) | ||
| } else { | ||
| user = createNewUser(userRequest, userDetails) | ||
| createNewUser(userRequest, userDetails) |
There was a problem hiding this comment.
user lifted out of if statement
| user.apply { | ||
| name = userDetails.name | ||
| displayName = userDetails.displayName ?: StringUtils.EMPTY | ||
| avatar = userDetails.avatar ?: StringUtils.EMPTY | ||
| } |
There was a problem hiding this comment.
This is similar to aforementioned .also for newly created objects, in this case we use .apply
| abstract class AbstractCrudController<S : CrudOperationsService<T, ID>, T : IdentifiableEntity<ID>, ID, U : AbstractDto<T>, C : AbstractDto<T>>( | ||
| protected open val service: S | ||
| ) { | ||
|
|
||
| companion object { | ||
| private const val SPEL_EXPRESSION = "(isAuthenticated() && principal.user.identifier.equals(#id)) || hasAuthority('ADMIN')" | ||
| } | ||
| abstract class AbstractCrudController<S, T, ID, U, C>( | ||
| protected open val service: S | ||
| ) where | ||
| S : CrudOperationsService<T, ID>, | ||
| T : IdentifiableEntity<ID>, | ||
| U : AbstractDto<T>, | ||
| C : AbstractDto<T> { |
There was a problem hiding this comment.
Kotlin has where constraits which do the same job as the former generic parameter declaration, but it's appended after the class signature in its own block, making the generic statement easier to grasp.
| return try { | ||
| return ObjectMapper().writer().writeValueAsString(this) | ||
| } catch (e: JsonProcessingException) { | ||
| e.printStackTrace() | ||
| null | ||
| } | ||
|
|
||
| return null |
There was a problem hiding this comment.
Move the try-catch block into the return
| if (cookies != null && cookies.isNotEmpty()) { | ||
| for (cookie in cookies) { | ||
| if (cookieName == cookie.name) { | ||
| return Optional.of(cookie) | ||
| } | ||
| } | ||
| val foundCookie = cookies?.let { cookie -> | ||
| cookie.firstOrNull { it.name == cookieName } | ||
| } | ||
|
|
||
| return Optional.empty() | ||
| return foundCookie?.let { Optional.of(it) } | ||
| ?: Optional.empty() |
There was a problem hiding this comment.
Just like Java streams, there are plenty of methods on the Kotlin Collections library which can reduce the above loop into a more condensed set of operations
|
Looks great, also thanks for all of these comments that clarify the intentions! ❤️
|
dzikoysk
changed the title
Closes #534
This patch adds https://github.com/pinterest/ktlint, a linting tool to keep code style across the project consistent as well as refactoring parts of the codebase to use idiomatic Kotlin as discussed in #534
Ktlint is executed through Maven with
mvn antrun:run@ktlint-format.Further refactoring
A lot of the types in the codebase are nullable (makes sense as this comes from java land) and because I'm not too familiar with the project itself it's difficult to tell if any nullable types are actually nullable or not. If these types can be made non-null we can ensure further type safety, if this is not the case, there should be fallback values as the functions the values are passed into doesn't expect a nullable value.
redirectUriis not null?objis not null?fieldErroris not null?Anything inside /test/old_tests has not been touched.