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

Rick and Morty First Commit #43

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

romaxa11
Copy link

@romaxa11 romaxa11 commented Nov 4, 2023

No description provided.

pom.xml Outdated
<dependency>
<groupId>com.mysql</groupId>
<artifactId>mysql-connector-j</artifactId>
<version>8.1.0</version>
Copy link

Choose a reason for hiding this comment

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

let spring to manage version

Suggested change
<version>8.1.0</version>

Comment on lines +13 to +20
private static RickAndMortyClient rickAndMortyClient;
private static CharacterService characterService;

@Autowired
public Application(CharacterService service, RickAndMortyClient client) {
Application.rickAndMortyClient = client;
Application.characterService = service;
}
Copy link

Choose a reason for hiding this comment

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

Application is responsible for booting your app.
Not for the characterService.saveAll(characters);
SRP violates here.

Consider using CommanLineRunner or ApplicationRunner.

Suggested change
private static RickAndMortyClient rickAndMortyClient;
private static CharacterService characterService;
@Autowired
public Application(CharacterService service, RickAndMortyClient client) {
Application.rickAndMortyClient = client;
Application.characterService = service;
}


@Operation(summary = "Get random character",
description = "Get a random characters from Rick and Morty world")
@GetMapping("/random")
Copy link

Choose a reason for hiding this comment

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

REST URI convention violates here.

Suggested change
@GetMapping("/random")
@GetMapping


@Operation(summary = "Search characters",
description = "Search characters by name")
@GetMapping("/search")
Copy link

Choose a reason for hiding this comment

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

Recall the REST URI convention.

Suggested change
@GetMapping("/search")
@GetMapping

import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.stereotype.Repository;

@Repository
Copy link

Choose a reason for hiding this comment

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

@repository is redundant here.

Suggested change
@Repository

private final ObjectMapper objectMapper;

public List<CharacterResponseDto> getCharacters() {
HttpClient httpClient = HttpClient.newHttpClient();
Copy link

Choose a reason for hiding this comment

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

Make HttpClient the spring bean.
It shouldnt be created every method invokation.

Suggested change
HttpClient httpClient = HttpClient.newHttpClient();
HttpClient httpClient = HttpClient.newHttpClient();

.readValue(response.body(), ListCharacterDto.class);
return listCharacterDto.getResults();
} catch (IOException | InterruptedException e) {
throw new RuntimeException(e);
Copy link

Choose a reason for hiding this comment

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

Add method description here.

Suggested change
throw new RuntimeException(e);
throw new RuntimeException(e);

@Service
public class CharacterServiceImpl implements CharacterService {
private int listSize;
private final Random random = new Random();
Copy link

Choose a reason for hiding this comment

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

We are using spring, so such instantiations is bad practice.
COnsider making random the spring bean.

Suggested change
private final Random random = new Random();
private final Random random = new Random();

spring.datasource.username=root
spring.datasource.password=Romaxa051979
#spring.datasource.driver-class-name=com.mysql.cj.jdbc.Driver
spring.jpa.database-platform=org.hibernate.dialect.MySQLDialect
Copy link

Choose a reason for hiding this comment

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

Is it required property? if not should be removed

Suggested change
spring.jpa.database-platform=org.hibernate.dialect.MySQLDialect
spring.jpa.database-platform=org.hibernate.dialect.MySQLDialect

@@ -1,13 +1,8 @@
package mate.academy.rickandmorty;

import org.junit.jupiter.api.Test;
import org.springframework.boot.test.context.SpringBootTest;

@SpringBootTest
class ApplicationTests {
Copy link

Choose a reason for hiding this comment

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

empty class should be removed.

or write the tests.

Choose a reason for hiding this comment

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

not fixed

@romaxa11 romaxa11 requested a review from sokimaaa November 5, 2023 07:41
Copy link

@kerrrusha kerrrusha left a comment

Choose a reason for hiding this comment

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

almost good


@Bean
public CommandLineRunner commandLineRunnerBean() {

Choose a reason for hiding this comment

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

Suggested change

import org.springframework.context.annotation.Configuration;

@Configuration
public class RandomUtil {

Choose a reason for hiding this comment

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

Suggested change
public class RandomUtil {
public class RandomConfiguration {

Comment on lines +10 to +12
Random util() {
return new Random();
}

Choose a reason for hiding this comment

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

Suggested change
Random util() {
return new Random();
}
public Random random() {
return new Random();
}

@@ -1,13 +1,8 @@
package mate.academy.rickandmorty;

import org.junit.jupiter.api.Test;
import org.springframework.boot.test.context.SpringBootTest;

@SpringBootTest
class ApplicationTests {

Choose a reason for hiding this comment

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

not fixed

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