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

Feature/performant old filmlist reader #150

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

Nicklas2751
Copy link
Member

In context of the Filmlistmerger I created a new, more performant, reader for the old Filmlist .

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 6, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Member

@TheSasch TheSasch left a comment

Choose a reason for hiding this comment

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

Didn't had the time to have a look at the unit tests. I will do it later.

import static java.time.format.FormatStyle.MEDIUM;

@Mapper
public interface RawFilmToFilmMapper {
Copy link
Member

Choose a reason for hiding this comment

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

Ok, lets talk about the mapper :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure :)

@@ -1,6 +1,13 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>
<parent>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-parent</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

wieso Spring Boot? wird doch in MLib gar nicht verwendet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Im spring Boot parent sind für viele Abhängigkeiten die Versionen aufeinander abgestimmt und getestet. Wenn wir den, wie hier von mir getan, nutzen müssen wir uns nicht um Kompatibilität einzelner Versionen zueinander kümmern. Zum updaten von dependencies können wir so auch "einfach" die Spring version erhöhen und müssen uns dann nur noch um die zusätzlichen Deps kümmern.

@@ -9,7 +9,7 @@

import java.io.IOException;

import static jakarta.ws.rs.core.HttpHeaders.CONTENT_LENGTH;
import static javax.ws.rs.core.HttpHeaders.CONTENT_LENGTH;
Copy link
Contributor

Choose a reason for hiding this comment

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

MServer verwendet die jakarta-Variante, keine Ahnung ob das im Zusammenspiel mit MLib problematisch werden könnte. Warum wieder zurück zur "alten" Variante?

Copy link
Member Author

Choose a reason for hiding this comment

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

Dependency Version hat sich geändert und da sind die Pfade noch so

@Nicklas2751 Nicklas2751 requested a review from TheSasch February 27, 2022 16:01
Copy link
Member

@TheSasch TheSasch left a comment

Choose a reason for hiding this comment

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

I also wrote some findings to Discord.


@Mapper
public interface RawFilmToFilmMapper {
Logger LOG = LogManager.getLogger(RawFilmToFilmMapper.class);
Copy link
Member

Choose a reason for hiding this comment

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

Use @slf4j when you already use Lombok instead of self definiton

Film rawFilmToFilm(RawFilm rawFilm);

default List<GeoLocations> mapGeolocation(RawFilm rawFilm) {
return GeoLocations.find(rawFilm.getGeo()).map(List::of).orElse(new ArrayList<>());
Copy link
Member

Choose a reason for hiding this comment

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

return GeoLocations.find(rawFilm.getGeo()).stream().collect(Collectors.toList());


@AfterMapping
default void complexMappings(RawFilm rawFilm, @MappingTarget Film film) {
long groesse = mapSize(rawFilm);
Copy link
Member

Choose a reason for hiding this comment

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

could be final

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 9, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 21 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 9, 2022

Please retry analysis of this Pull-Request directly on SonarCloud.

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.

3 participants