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

@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 September 27, 2021 08:38
@mpro7 mpro7 marked this pull request as ready for review September 27, 2021 08:38

import org.knora.webapi.IRI

trait ProjectCreatePayloadTraitADM {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trait is superfluous here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

call it something else

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


import org.knora.webapi.IRI

trait ProjectCreatePayloadTraitADM {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

call it something else


import org.knora.webapi.IRI

trait ProjectCreatePayloadTraitADM {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, trait is not needed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

case None => None
}

val project: ProjectCreatePayloadADM =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@subotic
Copy link
Collaborator

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 September 29, 2021 07:15
@mpro7
Copy link
Collaborator

mpro7 commented Sep 29, 2021

@subotic all comments reflected in a0d5f97.

Copy link
Collaborator

@subotic subotic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks! LGTM!

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

@subotic subotic Sep 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@mpro7 mpro7 deleted the wip/DSP-1555-refactor-projects-route branch September 29, 2021 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants