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
Conversation
|
||
import org.knora.webapi.IRI | ||
|
||
trait ProjectCreatePayloadTraitADM { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trait is superfluous here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
call it something else
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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] { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 = |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
;)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
looks good. |
There was a problem hiding this 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 ( |
There was a problem hiding this comment.
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.
In addition to DEV-64 implementing escaping characters, which has been already done, we have added the value objects handling for projects route.