Spring boot application web service

Question

It's a web application. It should provide a service for entering data. Data is then evaluated and uses itself another service.

It was part of an interview process where I was given that task to solve. The feedback was that it was not sufficient and I got rejected. But I don't know what exactly they don't like. Could you please review it and give me improvement suggestions, please?

pom.xml

<?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 https://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>
        <version>2.2.4.RELEASE</version>
        <relativePath/> <!-- lookup parent from repository -->
    </parent>
    <groupId>de.interview.fizzbuzz</groupId>
    <artifactId>demo</artifactId>
    <version>0.0.1-SNAPSHOT</version>
    <name>demo</name>
    <description>Demo project for Spring Boot</description>

    <properties>
        <java.version>1.8</java.version>
    </properties>

    <dependencies>
        <dependency>
            <groupId>org.springframework.boot</groupId>
            <artifactId>spring-boot-starter-data-jpa</artifactId>
        </dependency>
        <dependency>
            <groupId>org.springframework.boot</groupId>
            <artifactId>spring-boot-starter-web</artifactId>
        </dependency>

        <dependency>
            <groupId>com.h2database</groupId>
            <artifactId>h2</artifactId>
            <scope>runtime</scope>
        </dependency>
        <dependency>
            <groupId>org.projectlombok</groupId>
            <artifactId>lombok</artifactId>
            <optional>true</optional>
        </dependency>
        <dependency>
            <groupId>org.springframework.boot</groupId>
            <artifactId>spring-boot-starter-test</artifactId>
            <scope>test</scope>
            <exclusions>
                <exclusion>
                    <groupId>org.junit.vintage</groupId>
                    <artifactId>junit-vintage-engine</artifactId>
                </exclusion>
            </exclusions>
        </dependency>
        <dependency>
            <groupId>junit</groupId>
            <artifactId>junit-dep</artifactId>
            <version>4.8.2</version>
            <scope>test</scope>
        </dependency>
    </dependencies>

    <build>
        <plugins>
            <plugin>
                <groupId>org.springframework.boot</groupId>
                <artifactId>spring-boot-maven-plugin</artifactId>
            </plugin>
        </plugins>
    </build>

</project>

Controller

package de.interview.fizzbuzz.demo.controller;

import de.interview.fizzbuzz.demo.model.FizzBuzzInput;
import de.interview.fizzbuzz.demo.model.FizzBuzzOutput;
import de.interview.fizzbuzz.demo.model.WorldClock;
import lombok.NoArgsConstructor;
import lombok.NonNull;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.web.bind.annotation.*;
import org.springframework.web.client.RestTemplate;

import javax.validation.Valid;
import java.util.Collection;
import java.util.SortedMap;
import java.util.TreeMap;

@NoArgsConstructor
@RestController
@RequestMapping("/api")
public class FizzBuzzController {

    private final Logger log = LoggerFactory.getLogger(FizzBuzzController.class);
    private SortedMap<Integer, FizzBuzzOutput> fizzBuzzOutputsRepo = new TreeMap<Integer, FizzBuzzOutput>();

    @GetMapping("/allFizzBuzzItems")
    public Collection<FizzBuzzOutput> allFizzBuzzItems() {
        System.out.println(fizzBuzzOutputsRepo.values());
        return fizzBuzzOutputsRepo.values();
    }

    @PutMapping("/fizzbuzz")
    public void createFizzBuzzItem(@Valid @RequestBody @NonNull FizzBuzzInput fizzBuzzInput) {
        Integer input = Integer.parseInt(fizzBuzzInput.getValue());
        if (input <= 0) {
            return;
        }

        String value = BusinessLogic.fizzBuzzOf(Integer.parseInt(fizzBuzzInput.getValue()));

        String timestamp = getTimestampFrom("http://worldclockapi.com/api/json/cet/now");

        FizzBuzzOutput fizzBuzzOutput = new FizzBuzzOutput(input, value, timestamp);
        fizzBuzzOutputsRepo.put(input, fizzBuzzOutput);

    }

    // delete all
    @DeleteMapping("/fizzbuzz/all")
    public void deleteAllFizzBuzzItems() {
        fizzBuzzOutputsRepo.clear();
    }

    public String getTimestampFrom(String serviceURL) {
        RestTemplate restTemplate = new RestTemplate();
        WorldClock result = restTemplate.getForObject(serviceURL, WorldClock.class);
        return result.getCurrentDateTime();
    }
}

The whole project: https://github.com/kodingreview/1

Answer 1

• fizzBuzzOutputsRepo is an in-memory map and there is no data persistence. So if the web server crashes, then there will be data loss.
• Even if data persistence is out of scope for the interview question, by default a web server works in a multi-user environment. However, there is no synchronization method to protect the data integrity of fizzBuzzOutputsRepo. The simplest way is to use syncronize keyword when declaring fizzBuzzOutputsRepo.
• Java has java.time.LocalDateTime library which can give you the current system clock time. It does seem like an overly heavy approach to make an HTTP request to some external endpoint to just get current date time. Because that introduces complexity such as managing outgoing HTTP connection pool, dealing with upstream connection timeout etc.
• From API design perspective, the common practice is to version the API and use the resource name in URL: For instance api/v1/fizzbuzz/ is the common part of the route. For GET and DELETE, api/v1/fizzbuzz/{fizzbuzz_id}/ means operating on a single resource while api/v1/fizzbuzz/ means operating on all resources. Although it is generally not good practice to support operation on all resources because as the amount of data increases, it puts increasing demand on a single web server and other parts of the stack such as the database that stores the data.

Answer 2

From API design perspective let me pitch in

1. Have a base path and then have the resource name in the corresponding request mapping.
   • Base path /api
   • version /v1
   • request mapping /fizzBuzz
2. Add HTTP status code in response. As simple as 200 for OK and 400 Bad request should be fine for a sample API. Further reading: HTTP status LIST

3. Add Swagger Docs it's pretty straightforward too.

   Maybe once your API is up then you can further improve them using the next steps

4. Adding Custom Exception.
5. Following RMM for API design improvement.
6. Adding Comments for code quality :-).

Answer 3

PUT vs. POST

@PutMapping("/fizzbuzz")
public void createFizzBuzzItem(...)

PUT and POST can be used to create new resources.

The difference is that PUT is idempotent which means:

calling it once or several times successively has the same effect

But your fizzbuzz implementation has a time stemp that gets updated:

FizzBuzzOutput fizzBuzzOutput = new FizzBuzzOutput(input, value, timestamp);

This is the reason why createFizzBuzzItem is not idempotent and should be a POST instead of PUT.

---

The Controller

The FizzBuzzController looks like it is the heart of the application:

• it stores data
• has some logic

A controller only receives user input, validates it and let the model operate on the incoming data.

You already did a good job in createFizzBuzzItem where the input gets validated by if (input <= 0) and then the business logic does some work via BusinessLogic.fizzBuzzOf(...).

But since the controller persist data beside the controlling part (what violates the single responsibility principle) some business logic is needed inside the controller like:

FizzBuzzOutput fizzBuzzOutput = new FizzBuzzOutput(input, value, timestamp);
fizzBuzzOutputsRepo.put(input, fizzBuzzOutput);

fizzBuzzOutputsRepo.clear();

return fizzBuzzOutputsRepo.values();

The controller should know nothing about the persistence and should only ask the model (BusinessLogic) for data, like:

@PutMapping("/fizzbuzz")
public void createFizzBuzzItem(@Valid @RequestBody @NonNull FizzBuzzInput fizzBuzzInput) {
  Integer input = Integer.parseInt(fizzBuzzInput.getValue());
  if (input <= 0) {
    return;
  }

  return BusinessLogic.fizzBuzzOf(Integer.parseInt(fizzBuzzInput.getValue());
}

@DeleteMapping("/fizzbuzz")
public void deleteAllFizzBuzzItems() {
    BusinessLogic.deleteAll();
}

---

Data Access Object

Human work with abstractions and naming is a good way to abstract by chunking thinks.

public void createFizzBuzzItem(@Valid @RequestBody @NonNull FizzBuzzInput fizzBuzzInput)

FizzBuzzOutput fizzBuzzOutput = new FizzBuzzOutput(input, value, timestamp);

First what I like is that you think about in- and output. But there is already a known abstraction for in- and output and this is the Data Access Object (DTO). The bad with the naming fizzBuzzInput and fizzBuzzOutput is that a reader of your code does not know if this are DTOs and have to verify it by scroll through the code what makes reading code more difficult than only working with abstractions.

public void createFizzBuzzItem(@Valid @RequestBody @NonNull CreateDTO dto)

---

Naming

Additional your naming is redundant. When I search for the word fizzbuzz inside the controller I get more than 20 results back.

It's suffices naming the class FizzBuzzController and from there you know by the context of the class that you deal with fizzbuzz:

public class FizzBuzzController {

  // ..

  // @...
  public Collection<FizzBuzzOutput> findAll() { /* ... */ }

  // @...
  public void create(@Valid @RequestBody @NonNull CreateDTO dto) {/* ... */ }

  // @...
  public void deleteAll() { /*...*/ }
}

---

A Service

Its common case to append the class that holdes the business logic with Service as a suffix like: FizzBuzzService. A service takes a dto and returns a dto.

For instance inside createFizzBuzzItem:

String value = BusinessLogic.fizzBuzzOf(Integer.parseInt(fizzBuzzInput.getValue()));

String value = fizzBuzzService.craeteBy(dto);

---

Duplication

Code duplication can lead to code that is hard to maintain.

Integer.parseInt(fizzBuzzInput.getValue())

This line of code is two times inside createFizzBuzzItem.

Additional I do not understand why you have to parse it. Can't FizzBuzzInput holdes a integer instead of a string value like:

class FizzBuzzIput {
    final int value;
}

---

Unused Code

The log gets never used and could be deleted if you do not want to log something.

---

Example

A refactoring could look like

@RequestMapping("/api/fizzbuzz")
@RestController
public class FizzBuzzController {

  private final FizzBuzzService service = new FizzBuzzService();

  @GetMapping
  public Collection<SavedDTO> findAll() {
    return service.findAll();
  }

  @PostMapping
  public ... createFizzBuzzItem(@Valid @RequestBody @NonNull CreateDTO dto) {
    if (dto.value <= 0) {
      // HTTP Bad Request (400)
      // accepts only values that are greater 0
    }

    return service.createBy(dto);
  }

  @DeleteMapping
  public ... deleteAll() {
    service.deleteAll();
  }
}