1
\$\begingroup\$

I've got a class that is like a facade, like an engine for my application which is responsible for fetching data from url and summing it.

I have got problem with proper names for my methods.

  1. executeNBPParserEngine is like method that connects everything to one. It gots if and else statement. If statement checks if the given day is included in current year (data from current year is stored in different page than from previous years). If yes, then it opens connection to the given site (data from current year is stored in different page than from previous years), fetch some data and sum it in sumOfSellingRate and sumOfBuyingRate.

The class looks like this. I do not know if every method here is named properly. Maybe I should create some more methods to shorten it more?

import conditionchecker.ConditionChecker;
import org.w3c.dom.Document;
import org.w3c.dom.Element;
import org.w3c.dom.Node;
import org.w3c.dom.NodeList;
import org.xml.sax.SAXException;

import javax.xml.parsers.ParserConfigurationException;
import java.io.IOException;
import java.time.LocalDate;
import java.util.List;
import java.util.Scanner;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import static java.lang.System.in;

class NBPParserEngine {

    public static final String KOD_WALUTY = "kod_waluty";
    private ConditionChecker conditionChecker;
    private DataFetcher dataFetcher;
    private Scanner scanner;
    private float buyingRate;
    private float sellingRate;
    private float sumOfBuyingRate = 0;
    private float sumOfSellingRate = 0;

    NBPParserEngine(ConditionChecker conditionChecker, DataFetcher dataFetcher) {
        this.conditionChecker = conditionChecker;
        this.dataFetcher = dataFetcher;
        scanner = new Scanner(in);
    }

    void executeNBPParserEngine() {
        String startDateString = "2013-01-28";
        String endDateString = "2013-01-31";
        List<LocalDate> daysBetweenFirstAndSecondDate = findDaysBetweenFirstAndSecondDate(startDateString, endDateString);

        for (LocalDate iteratedDay : daysBetweenFirstAndSecondDate) {
            if (conditionChecker.checkIfGivenDayIsIncludedInCurrentYear(iteratedDay))
                try {
                    String DIR_SOURCE = "http://www.nbp.pl/kursy/xml/dir.txt";
                    String line = dataFetcher.findLineWithGivenDate(String.valueOf(iteratedDay), DIR_SOURCE);

                    sumBuyingAndSellingRate(line);
                } catch (IOException | SAXException | ParserConfigurationException e) {
                    e.printStackTrace();
                }
            else {
                try {
                    String iteratedDayString = iteratedDay.toString();
                    String[] iteratedStringArray = iteratedDayString.split("-");
                    String DIR_SOURCE = "http://www.nbp.pl/kursy/xml/dir" + iteratedStringArray[0] + ".txt";
                    String line = dataFetcher.findLineWithGivenDate(String.valueOf(iteratedDay), DIR_SOURCE);

                    sumBuyingAndSellingRate(line);

                } catch (IOException | SAXException | ParserConfigurationException e) {
                    e.printStackTrace();
                }
            }
        }
    }

    void sumBuyingAndSellingRate(String line) throws ParserConfigurationException, SAXException, IOException {
        String URL_SOURCE = "http://www.nbp.pl/kursy/xml/" + line + ".xml";
        Document doc = dataFetcher.getXML(URL_SOURCE);
        NodeList nList = doc.getElementsByTagName("pozycja");

        for (int temp = 0; temp < nList.getLength(); temp++) {
            Node nNode = nList.item(temp);

            if (nNode.getNodeType() == Node.ELEMENT_NODE) {
                Element eElement = (Element) nNode;
                if (eElement.getElementsByTagName(KOD_WALUTY).item(0).getTextContent().equals("USD")) {
                    buyingRate = getBuyingRateFromDOM(eElement);
                    sellingRate = getSellingRateFromDOM(eElement);
                }
            }
        }
        sumOfBuyingRate += buyingRate;
        sumOfSellingRate += sellingRate;
    }

    public float getSellingRateFromDOM(Element eElement) {
        return Float.parseFloat(eElement
                .getElementsByTagName("kurs_sprzedazy")
                .item(0)
                .getTextContent().replaceAll(",", "."));
    }


    float getBuyingRateFromDOM(Element eElement) {
        return Float.parseFloat((eElement
                .getElementsByTagName("kurs_kupna")
                .item(0)
                .getTextContent().replaceAll(",", ".")));
    }

    public List<LocalDate> findDaysBetweenFirstAndSecondDate(String startDateString, String endDateString) {
        LocalDate startDate = LocalDate.parse(startDateString);
        LocalDate endDate = LocalDate.parse(endDateString);

        Stream<LocalDate> dates = startDate.datesUntil(endDate.plusDays(1));
        return dates.collect(Collectors.toList());
    }
}
\$\endgroup\$
2
\$\begingroup\$

Naming Convention


To answer I think your method names are unnecessarily long and overly descriptive.

executeNbpParserEngine is a little more readable then executeNBPParserEngine as it doesn't hide Parse. When working with acronyms: One should take into account if the abbreviation is well known such as XML, HTTP, JSON. if the abbreviation is at the end ex. executeParserEngineFromNBP then it is even more readable. read more here

I.e When I read the function name findDaysBetweenFirstAndSecondDate It's difficult to read. Your function name should work with your arguments name and return type to give a more accurate description.

Code review:


1: Initialise the Scanner Object in line as it will improve Maintainability if you need multiple constructors in the future.

private Scanner scanner = new Scanner(in); 

2: Switch executeNBPParserEngine to executeNbpParserEngine it a bit cleaner 3: findDaysBetweenFirstAndSecondDate to getDaysBetween

Also, you do not have to declare a variable just run collect on the return from LocalDate: datesUntil.

public List<LocalDate> getDaysBetween(String startDateString, String endDateString) {
    LocalDate startDate = LocalDate.parse(startDateString);
    LocalDate endDate = LocalDate.parse(endDateString);
    return startDate.datesUntil(endDate.plusDays(1)).collect(Collectors.toList());
}

4: Combine getSellingRateFromDOM && getBuyingRateFromDOM as they are too similar. change to

  float getFieldFromDOM(Element eElement,String tag) {
return Float.parseFloat((eElement
        .getElementsByTagName(tag)
        .item(0)
        .getTextContent().replaceAll(",", ".")));

}

Also, add final strings that contain tag ex.kurs_kupna

private final String BUY_RATE_TAG = "kurs_kupna";
private final String SELL_RATE_TAG = "kurs_sprzedazy";
\$\endgroup\$
  • \$\begingroup\$ The last thing I want to ask is sumBuyingAndSellingRate is invalid name, as this method looks like this now: pastebin.com/EENPupFJ It is very similiar, but I sum buyingRate and add to list sellingRate as I need to calculate later standard deviation. What is the proper name for it? \$\endgroup\$ – pipilam Apr 29 at 20:19

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.