7
\$\begingroup\$

I am building a simple Java command-line application. And I want to know the best practice and make my code clean. Can anybody suggest to me how many different classes I can make and what more method I need to write to make my code clean? How can I write unit testing for the methods? I am new to writing unit tests.

The question is, I need to call an API then take input from a user and according to the user input I need to call another API, then show some data based on minimum or maximum. I am confused that should I write a separate class for API calls or keep them in the same file. Should I divide my API calling methods more or not? Thanks :)

import org.json.JSONObject;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.net.HttpURLConnection;
import java.net.URL;
import java.text.SimpleDateFormat;
import java.util.*;

import static java.lang.System.out;

public class BitcoinRateCheckApplicationByCurrency {
    public static final String URL = "https://api.coindesk.com/v1/bpi/currentprice.json";
    public static JSONObject jsonAPIresponseObject = null;

    public static void main(String[] args) throws IOException {
        jsonAPIresponseObject = getJSONAPIdataFromURL(URL);
        out.println("Input a currency code (USD, EUR, GBP, etc.)");
        ;
        Scanner scanner = new Scanner(System.in);
        String currency = scanner.nextLine();

        JSONObject bpiObject = (JSONObject) jsonAPIresponseObject.get("bpi");

        JSONObject currencyDataObject = (JSONObject) bpiObject.get(currency);

        Double currentRate = (Double) currencyDataObject.get("rate_float");

        Date dNow = new Date();

        SimpleDateFormat ft = new SimpleDateFormat("yyyy-MM-dd");

        System.out.println("Current Date: " + ft.format(dNow));

        Calendar cal = Calendar.getInstance();
        cal.setTime(new Date());
        cal.add(Calendar.DATE, -30);
        Date dateBefore30Days = cal.getTime();


        String currentDate = ft.format(dNow);
        String fromDate = ft.format(dateBefore30Days);

        String apiDataRangeURL = "https://api.coindesk.com/v1/bpi/historical/close.json?start=" + fromDate + "&end=" + currentDate + "&?currency=" + currency;
        JSONObject lastOneMonthDataObject = getJSONAPIdataFromURL(apiDataRangeURL);

        JSONObject obj = (JSONObject) lastOneMonthDataObject.get("bpi");

        Set<String> keySet = obj.keySet();
        ArrayList<Double> priceListForDateRange = new ArrayList<Double>();
        for (String x : keySet) {
            double price = (Double) obj.get(x);
            priceListForDateRange.add(price);
        }


        out.println("- The current Bitcoin rate, in the requested currency: " + currentRate);
        out.println("- The lowest Bitcoin rate in the last 30 days, in the requested currency: " + getMinValue(priceListForDateRange));
        out.println("- The highest Bitcoin rate in the last 30 days, in the requested currency: " + getMaxValue(priceListForDateRange));


    }

    public static JSONObject getJSONAPIdataFromURL(String url) throws IOException {

        JSONObject responseJSON = null;
        URL urlForGetRequest = new URL(url);
        String readLine = null;

        HttpURLConnection conection = (HttpURLConnection) urlForGetRequest.openConnection();
        conection.setRequestMethod("GET");
        int responseCode = conection.getResponseCode();

        if (responseCode == HttpURLConnection.HTTP_OK) {
            BufferedReader in = new BufferedReader(new InputStreamReader(conection.getInputStream()));
            StringBuffer response = new StringBuffer();
            while ((readLine = in.readLine()) != null) {
                response.append(readLine);
            }
            in.close();
            responseJSON = new JSONObject(response.toString());
        } else {
            System.out.println("GET NOT WORKED");
        }

        return responseJSON;
    }

    public static double getMaxValue(ArrayList<Double> numbers) {

        double maxValue = numbers.get(0);
        for (int i = 1; i < numbers.size(); i++) {
            if (numbers.get(i) > maxValue) {
                maxValue = numbers.get(i);
            }
        }
        return maxValue;
    }

    public static double getMinValue(ArrayList<Double> numbers) {
        double minValue = numbers.get(0);
        for (int i = 1; i < numbers.size(); i++) {
            if (numbers.get(i) < minValue) {
                minValue = numbers.get(i);
            }
        }
        return minValue;
    }
}

```
\$\endgroup\$
4
\$\begingroup\$

Before separating the code into classes try separating it into more methods. At the very least separate business/logic code and input/output code. You should be able to use the same, unchanged business code no matter if it's a command line program, has a GUI or is a web application. For example getJSONAPIdataFromURL shouldn't write to System.out, but return an error value or throw an exception (which you should catch and display to user in the input/output part of the code). That business code would then be a prime example to be put into a separate class.

There are many other things that can be done:


import java.util.*;

Don't import using wildcards.

import static java.lang.System.out;

If you import something statically, then use it everywhere and not just in a few places. However I wouldn't statically import System.out, since System.out.println(...) is more readable than just out.println(...).


There is no reason for jsonAPIresponseObject to be a (static) field. It's not used anywhere else than in the main method, and even if it weren't, then it should be passed around to other methods as an argument.


Consider (optionally) allowing the user to pass the currency as a command line argument.


.close() the Scanner and the InputStream you get from the HttpConnection after you've used it, for example using "try with resources" and .disconnect() the HttpConnection.

Consider using an third party HTTP client library. It maybe a a bit heavy weight for such a small project, but the API is much nicer. (Or Java 9 has a new HTTP client, that is better than the old one).


You are neither checking if the JSONObject::get calls are throwing an exception nor checking if you safely can cast its results. Use JSONObject::optJSONObject (and JOSNObject::optDouble for doubles) to get results that don't need casting and check them for null.


If you have Java 8, then use the "new" LocalDate class to handle dates. It's much simpler than Date/Calendar:

    // The formatter should be a static final field;
    DateTimeFormatter ft = DateTimeFormatter.ISO_LOCAL_DATE;

    LocalDate dNow = LocalDate.now();
    LocalDate dateBefore30Days = dNow.minusDays(30);

    String currentDate = ft.format(dNow);
    String fromDate = ft.format(dateBefore30Days);

When building an URL make sure that URL parameters are properly escaped, especially since the currency is user input.


Use the smallest possible/sensible interface when declaring variables and method signatures. Instead of

ArrayList<Double> priceListForDateRange = new ArrayList<Double>();

and

public static double getMaxValue(ArrayList<Double> numbers) { 

just

List<Double> priceListForDateRange = new ArrayList<>();

and

public static double getMaxValue(List<Double> numbers) {

The min/max methods can be simplified by using Java 8 streams, e.g:

public static double getMaxValue(List<Double> numbers) {
   return numbers.stream().max(Comparator.naturalOrder()).get();
}

or since you only need the min and max values, calculate them directly in the loop reading them from the JSONObject.

\$\endgroup\$
  • \$\begingroup\$ Many many thanks. I am really grateful for your suggestions. I tried to incorporate all of your suggestions. Here is my code. Any further suggestions if needed would be appreciated. github.com/forhadmethun/… \$\endgroup\$ – forhadmethun Oct 9 at 21:01
  • \$\begingroup\$ @forhadmethun Post a new question with the new code. \$\endgroup\$ – RoToRa Oct 10 at 7:02
3
\$\begingroup\$

I'm just adding some elements to the explanation by RoToRa, use the method toString of the class LocalDate because it outputs the date in ISO-8601 format uuuu-MM-dd, and this is exactly the format you are using. If you are looking for math operations on streams of Double like searching for min, max, average you can use the class DoubleSummaryStatistics like the example below:

List<Double> priceListForDateRange;// previously initialized in the code from json values
DoubleSummaryStatistics statistics = priceListForDateRange.stream()
                                        .mapToDouble(Double::doubleValue)
                                        .summaryStatistics();
System.out.println("- The lowest Bitcoin rate in the last 30 days, in the requested currency: " + statistics.getMin());
System.out.println("- The highest Bitcoin rate in the last 30 days, in the requested currency: " + statistics.getMax());
\$\endgroup\$
  • \$\begingroup\$ Many many thanks. I am really grateful for your suggestions. I tried to incorporate all of your suggestions. Here is my code. Any further suggestions if needed would be appreciated. github.com/forhadmethun/… \$\endgroup\$ – forhadmethun Oct 9 at 21:01
  • \$\begingroup\$ @forhadmethun You are welcome, once you have modified your code you can post a new question with the new code. \$\endgroup\$ – dariosicily Oct 10 at 10:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.