Read from dynamic source and caching

Question

I want to load JSON data from a link at the beginning of an application.

I need to be a little bit about clear the following code.

And also want to ask about another opinions that what could be the best approach if I need to read from a URL and want to read data from this resource.

package com.app;

import com.app.entity.Event;
import com.app.service.EventCommandService;
import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.boot.CommandLineRunner;
import org.springframework.boot.SpringApplication;
import org.springframework.boot.autoconfigure.SpringBootApplication;
import org.springframework.context.annotation.Bean;

import java.io.*;
import java.net.HttpURLConnection;
import java.net.URL;
import java.util.List;

@SpringBootApplication
public class Application {
    static HttpURLConnection connection;
    Logger logger = LoggerFactory.getLogger(Application.class);
    @Value("${url.path}")
    String urlValue;

    public static void main(String[] args) {
        SpringApplication.run(Application.class, args);
    }

    @Bean
    CommandLineRunner runner(CommandService CommandService) {
        return args -> {
            BufferedReader reader;
            String line;
            StringBuilder responseContent = new StringBuilder();

            // read json and write to db
            ObjectMapper mapper = new ObjectMapper();
            TypeReference<List<Event>> typeReference = new TypeReference<>() {
            };

            try {
                URL url = new URL(urlValue);
                connection = (HttpURLConnection) url.openConnection();

                // Request setup
                connection.setRequestMethod("GET");
                connection.setConnectTimeout(5000);
                connection.setReadTimeout(5000);

                reader = new BufferedReader(new InputStreamReader(connection.getInputStream()));
                while ((line = reader.readLine()) != null) {
                    responseContent.append(line);
                }
                int index = responseContent.toString().indexOf("[{");
                byte[] bytes = responseContent.substring(index).getBytes();
                InputStream inputStream = new ByteArrayInputStream(bytes);
                try {
                    List<Event> events = mapper.readValue(inputStream, typeReference);
                    // Save events to h2 db
                    CommandService.save(events);
                    logger.info("Events Saved");
                } catch (IOException e) {
                    logger.info("Unable to save events: {}", e.getMessage());
                }
                reader.close();

            } catch (IOException e) {
                e.printStackTrace();
            } finally {
                connection.disconnect();
            }

        };
    }

}

Answer 1

Validate inputs

The program expects a valid HTTP URL to work correctly. It would be good to have some validation logic, and exit with a user-friendly error message, rather than a stack trace.

Also, even if the URL is valid, the content might not be valid JSON. It would be good to add validation for that too, and handle unexpected content more gracefully than crashing.

Use try-with-resources with AutoCloseables

The try-with-resources greatly simplifies the correct closing of AutoCloseables. The reader and inputStream should use it.

Close objects as soon as you're done with them

The reader stays open while using the mapper and saving to database. It would be better to close it after it's no longer needed.

The same goes for connection.

Declare variables in the smallest necessary scope

When variables are visible outside the scope where they are necessary, they might get referenced or overwritten by mistake. In case of the posted code:

• connection should be a local variable
• reader and inputStream should be declared in a try-with-resources statement
  • line should be declared within the scope of the reader
• mapper and typeReference should be declared right before they are needed

Separate concerns

Too many things are mixed together in the class and the runner method:

• It's best when the runnable class with the main method has almost no logic at all. The implementation of runner would be better in another class, injected into the main application.

• The runner does multiple logical steps intermixed in one method. These steps could be in separate methods, and unit tested separately:

  • Read text from a URL
  • Parse the list of events from text
  • Save the list of events

Note that when the logic is separated like that, comments like "// read json and write to db" become unnecessary, since you will be able to simply read the same thing when reading the code top to bottom.

Consider making member variables private final

urlValue will never change during the execution of the class, and shouldn't be visible outside of the class, therefore it should be private final.

Same for logger, and it should also be static, as commonly the case with loggers.

Other smaller things

• The variable CommandService should be commandService to avoid confusion with the class with the same name, and follow common Java conventions.
• Avoid printing to stdout or stderr as in e.printStackTrace(), use the logger
• Instead of responseContent.toString().indexOf("[{"), you can do responseContent.indexOf("[{")