Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(projects): refactor projects route with value objects (DEV-64) #1909

Merged
merged 5 commits into from Sep 29, 2021

Conversation

@irinaschubert
Copy link
Contributor

@irinaschubert irinaschubert commented Sep 23, 2021

In addition to DEV-64 implementing escaping characters, which has been already done, we have added the value objects handling for projects route.

@mpro7 mpro7 requested a review from subotic Sep 27, 2021
@mpro7 mpro7 marked this pull request as ready for review Sep 27, 2021

import org.knora.webapi.IRI

trait ProjectCreatePayloadTraitADM {
Copy link
Collaborator

@subotic subotic Sep 27, 2021

Trait is superfluous here.

Copy link
Collaborator

@subotic subotic Sep 27, 2021

call it something else

Copy link
Collaborator

@subotic subotic Sep 27, 2021

in this case you don't even need it, just delete it


import org.knora.webapi.IRI

trait ProjectCreatePayloadTraitADM {
Copy link
Collaborator

@subotic subotic Sep 27, 2021

call it something else


import org.knora.webapi.IRI

trait ProjectCreatePayloadTraitADM {
Copy link
Collaborator

@subotic subotic Sep 27, 2021

in this case you don't even need it, just delete it

/**
* Project payload
*/
sealed abstract case class ProjectCreatePayloadADM(
Copy link
Collaborator

@subotic subotic Sep 27, 2021

@irinaschubert I found the example for smart constructors. There is a private keyword missing:

sealed abstract case class ProjectCreatePayloadADM private (

case object InvalidGivenOrFamilyName extends ValidationError
case object InvalidPassword extends ValidationError
case object InvalidLanguageCode extends ValidationError

trait UserCreatePayloadTraitADM {
Copy link
Collaborator

@subotic subotic Sep 27, 2021

same here, trait is not needed.

Copy link
Collaborator

@subotic subotic Sep 27, 2021

a trait can be seen as a kind of an interface. you don't need an interface here, at is only used here and nowhere else.

/**
* Project shortcode value object.
*/
sealed abstract case class Shortcode(value: String)
Copy link
Collaborator

@subotic subotic Sep 27, 2021

missing private keyword

import org.knora.webapi.exceptions.BadRequestException
import org.knora.webapi.exceptions.{AssertionException, BadRequestException}
import org.knora.webapi.messages.StringFormatter
import org.knora.webapi.messages.store.triplestoremessages.StringLiteralV2

import scala.util.matching.Regex

trait ValueObject[T, K] {
Copy link
Collaborator

@subotic subotic Sep 27, 2021

I don't like the ValueObject trait. It brings hardly any benefits, but makes the implementations look more complicated than necessary. Don't over engineer things. Please keep them simple.

Copy link
Contributor

@mpro7 mpro7 Sep 28, 2021

Do you mean by this, no trait at all here?

case None => None
}

val project: ProjectCreatePayloadADM =
Copy link
Collaborator

@subotic subotic Sep 27, 2021

it is a projectCreatePayload and not a project.

val requestMessage: Future[ProjectCreateRequestADM] = for {
requestingUser <- getUserADM(
requestContext = requestContext,
featureFactoryConfig = featureFactoryConfig
)
} yield ProjectCreateRequestADM(
createRequest = apiRequest.validateAndEscape,
createRequest = project,
Copy link
Collaborator

@subotic subotic Sep 27, 2021

having project here is confusing, i.e., unexpected. code should never surprise.

Copy link
Contributor

@mpro7 mpro7 Sep 28, 2021

In typed languages it isn't that bad, because you always see what is under the variable due the type, so this can be even just p ;)

Copy link
Collaborator

@subotic subotic Sep 28, 2021

impairs readability. I'm doing review exclusively in the browser, so no types.

Copy link
Collaborator

@subotic subotic Sep 28, 2021

of course it works and can be anything. the compiler doesn't care. but code is read by humans.

@subotic
Copy link
Collaborator

@subotic subotic commented Sep 27, 2021

looks good.

@mpro7 mpro7 changed the title refactor(projects): refactor projects route with value objects (DSP-1555) refactor(projects): refactor projects route with value objects (DEV-64) Sep 28, 2021
@mpro7 mpro7 requested a review from subotic Sep 29, 2021
@mpro7
Copy link
Contributor

@mpro7 mpro7 commented Sep 29, 2021

@subotic all comments reflected in a0d5f97.

Copy link
Collaborator

@subotic subotic left a comment

Great, thanks! LGTM!

/**
* Project payload
*/
sealed abstract case class ProjectCreatePayloadADM private (
Copy link
Collaborator

@subotic subotic Sep 29, 2021

strictly speaking this does not need a smart constructor, since it doesn't do any "smart" checks on it and could be written as a final case class ProjectCreatePayloadADM(...). We can leave it like it is and if no smart checks get added over time, then we can refactor it.

@mpro7 mpro7 merged commit 172cf77 into main Sep 29, 2021
11 checks passed
@mpro7 mpro7 deleted the wip/DSP-1555-refactor-projects-route branch Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants