Дизайн утилитарного класса

 
 
 
Сообщения:7
Здравствуйте, написал простой парсер, который в дальнейшем будет встроен в собственный проект (телеграм бот). Суть его проста - получить на вход ссылку на веб-страницу и вернуть некоторые параметры.

То есть условно пользователь вводит ссылку в виде строки в определенном месте, далее полученная ссылка передается в метод parse. В результате записываем в поля результаты парсинга и возвращаем их в виде геттеров. Геттеры используются для отправки сообщения пользователю с помощью телеграм апи.

Возник вопрос с точки зрения дизайна. Нормальное ли это явление использовать статические приватные поля со статическими публичными геттерами? Можно ли придумать решение лучше? По сути класс создан для того, чтобы получить ссылку и вернуть некоторую информацию из полей, создавать объекты даже не нужно.

И ещё, если обратиться к геттерам раньше, чем к методу parse, я получу null'ы в списке. В принципе можно это условие обработать, только как? В виде выброса исключения, если в списке null? Хотелось бы какой-нибудь "запрет" повесить на вызов геттеров до вызова основного метода parse.
Может лучше будет сделать поля публичными и убрать геттеры? Или получится шило на мыло?

Есть ещё вариант (вроде неплохой). Просто возвращать из метода parse готовый список, который далее уже обрабатывать в другом месте.

import org.jsoup.HttpStatusException;
import org.jsoup.Jsoup;
import org.jsoup.nodes.Document;
import org.jsoup.select.Elements;
 
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
 
/**
 * Утилитарный класс, представляющий собой простой парсер, который собирает
 * некоторую информацию об игре на странице магазина Google Play.

 * @version 1.0
 *
 */
 
public final class GameInfoParser {
    private static Document document;
 
    /**
     * Список для хранения информации об игре.
     */
    private static List<String> features;
    /**
     * Хранит информацию о дате последнего обновления игры.
     */
    private static String lastUpdated;
    /**
     * Хранит информацию о текущей версии игры.
     */
    private static String currentVersion;
    /**
     * Хранит информацию о требуемой версии ОС Android для установки игры.
     */
    private static String requiresAndroid;
 
    /**
     * Геттеры для полей
     */
 
    public static String getLastUpdated() {
        return lastUpdated;
    }
 
    public static String getCurrentVersion() {
        return currentVersion;
    }
 
    public static String getRequiresAndroid() {
        return requiresAndroid;
    }
 
    /**
     * Запрещаем создавать объекты класса извне.
     */
    private GameInfoParser(){}
 
    /**
     * Главный метод, который принимает ссылку веб-страницы для парсинга.
     *
     * @param userId уникальный id пользователя
     * @param bot объект бота Telegram
     * @param URL ссылка на страницу игры в магазине Google Play
     */
    public static void parse(String userId, Bot bot, String URL) {
        try {
            connectToWebPage(URL);
        } catch (HttpStatusException e) {
            e.printStackTrace();
        } catch (IllegalArgumentException | IOException e) {
           e.printStackTrace();
        }
    }
 
    /**
     * Подключается (по крайней мере, пробует) по указанному URL-адресу.
     * Если ссылка уже "привязана" к русской локализации, просто подключаемся.
     * Если ссылка привязана к локализации, отличной от русской, принудительно
     * используем русскую локализацию путем изменения начального URL-адреса.
     *
     * @param URL ссылка на страницу игры в магазине Google Play
     *            
     * @throws IllegalArgumentException если введена некорректная ссылка
     * @throws IOException если не удается присоединиться к веб-странице с помощью jsoup            
     * @throws HttpStatusException если веб-страница в данный момент недоступна (404)
     */
    private static void connectToWebPage(String URL) throws IllegalArgumentException, IOException {
        if (URL.endsWith("&hl=ru")){
            document = Jsoup.connect(URL).get();
        }
        else if (!URL.contains("&hl=")){
            URL += "&hl=ru";
            document = Jsoup.connect(URL).get();
        }
        else if (URL.contains("&hl=")){
            String fixedURL = URL.replace
                    (URL.substring(URL.length()-6, URL.length()), "&hl=ru");
            document = Jsoup.connect(fixedURL).get();
        }
        features = crawlWebPage();
    }
 
    /**
     * Метод, который непосредственно парсит веб-страницу.
     * Заносит результат в список {@code features}.
     *
     * @return список с информацией об игре
     */
    private static List<String> crawlWebPage(){
        Elements spanElements = document.select("div[class=IQ1z0d] > span[class=htlgb]");
        features = new ArrayList<>();
        spanElements.forEach
                (element -> features.add(element.text()));
        if (checkFreeOrPaid(features))                      //проверяем, платная ли игра
            features.remove(0);
 
        allocateFeatures(features);
        return features;
    }
 
    /**
     * Распределяет элементы списка по соответствующим полям класса.
     *
     * @param game список с информацией об игре
     */
 
    private static void allocateFeatures(List<String> game){
        lastUpdated = game.get(0);
        currentVersion = game.get(3);
        requiresAndroid = game.get(4);
    }
 
    /**
     * Метод "проверяет", является ли игра платной. В зависимости от того, платная игра или нет,
     * в списке с информацией об игре в качестве первого элемента будет размещена либо дата
     * последнего обновления, либо  информация о возможности добавить игру в семейную библиотеку.
     * В свою очередь, это свидетельствует о том, что игра является платной. Чтобы свести платные
     * и бесплатные игры к одному варианту, метод проверяет длину {@code length} первого элемента
     * в списке. Если она больше указанного порога {@value = 35}, значит игра гарантированно платная,
     * поскольку строка, свидетельствующая об этом всегда содержит более 35 символов. Напротив, строка,
     * которая содержит в качестве первого элемента информацию о дате последнего обновления, никак не
     * может содержать в себе более 30 символов. Это всего лишь дата в формате "4 февраля 2020 г."
     *
     * @return {@code true} если длина строки первого элемента в списке
     * содержит более 35 символов, что свидетельствует
     */
 
    private static boolean checkFreeOrPaid(List<String> list){
        return list.get(0).length() > 35;
    }
}
Изменен:08 фев 2020 17:36
 
 
Сообщения:987
проблема здесь в многопоточности, поэтому утилиты лучше всего делать как одноразовые объекты.
в твоём случае передаёшь ссылку в конструктор, а результаты получаешь из объекта через геттеры.
 
 
Сообщения:7
windruf:
проблема здесь в многопоточности, поэтому утилиты лучше всего делать как одноразовые объекты.
в твоём случае передаёшь ссылку в конструктор, а результаты получаешь из объекта через геттеры.

Не синглтон, а именно одноразовые объекты? Для каждой новой ссылки (т.е. для каждого запроса, а значит и пользователя) - новый экземпляр класса, который в дальнейшем "умрет" после использования. Так?

Сделать паблик конструктор, который принимает ссылку и внутри сразу вызывает этот метод parse? Получается если брать в расчет многопоточность, лучше убирать статику и делать переменные экземпляра, а не класса? Но в таком случае придётся и методы сделать non static.
 
 
Сообщения:987
да.
сам использую статические классы, когда абсолютно точно известно, что доступ будет из одного потока. сильно упрощает жизнь.
но в Java EE на это даже не надейся.
 
 
Сообщения:320
Miroha:
По сути класс создан для того, чтобы получить ссылку и вернуть некоторую информацию из полей, создавать объекты даже не нужно.


Вот в этот момент вы подошли очень близко к решению задачи. И идея возвращать список из метода parse - тоже правильный путь. Но вы зачем-то потом начали усложнять решение.

Давайте по шагам. Сначала - практически дословно запишем ваши требования. Вернуть некоторую информацию.

final class SomeInformation {
    public final String lastUpdated;
    public final String currentVersion;
    // boolean? Или там значения вроде "иногда" бывают?
    public final String requiresAndroid;

    public SomeInformation(String lastUpdated, String currentVersion, String requiresAndroid) {
      this.lastUpdated = lastUpdated;
      this.currentVersion = currentVersion;
      this.requiresAndroid = requiresAndroid;
    }
}

final class GameInfoParser {
  public static SomeInformation parse(String userId, Bot bot, String URL) {
    return parseInformation(getInformationList(userId, bot, URL));
  }

  public SomeInformation parseInformation(items: List<String>) {
    // implementation
  }

  public List<String> getInformationList(String userId, Bot bot, String URL) {
    // ???
  }
}


Теперь нужно реализовать getInformationList. Желательно - чтобы это было совместимо с многопоточностью. Один из вариантов - создать экземпляр GameInfoParser (с приватным коснтрутором) внутри getInformationList и вызывать нужные методы. Но в данном кокнретном случае и это не нужно. Те небольшие части состояния, которые нужны методам для работы, можно передавать в них явно. Поэтому метод можно реализовать как

  public List<String> getInformationList(String userId, Bot bot, String URL) {
    final Document document = connectToWebPage(URL);
    return crawlWebPage(document);
  }

  private static Document connectToWebPage(String URL) { ... }
  /* Переименовать в extractFeatures? */
  private static List<String> crawlWebPage(Document document) { ... }


В этом случае в утилите вообще нет никакого состояния. Это автоматически делает код многопоточным (будут вопросы к многопоточности бота, но не утилиты). Никаких проблем с данными - до вызова метода parse никаких данных просто нет. Статические методы - относительно независимы друг от друга. Их можно в дальнейшем разнести по другим классам. У клиентов есть API для получения списка фич (списком) и для получения интерпретированного результата.

Объект данных можно делать по вкусу (getters/setters, mutable/immutable). Доступ ко внутренним методам GameInfoParser - тоже по вкусу, можно часть из них сделать private.
 
 
Сообщения:7
maxkar
Учел пожелания/замечания и решил сделать так. Комментарии и javadoc здесь все убрал, а то с ними слишком объемно.
Список возвращать смысла нет, так как в нем только "атрибуты" и половина из них не нужна.
Собственно сам объект игры, который я и буду возвращать в виде одной строки в телеграм боте:
package org.gplaybot.parsing;
 
final class Game {
    private final String lastUpdated;
    private final String currentVersion;
    private final String requiresAndroid;
    private final String genre;
    private final String IAP;
    private final String eSupport;
   

    public Game(String lastUpdated, String currentVersion, String requiresAndroid,
                 String genre, String IAP, String eSupport) {
        this.lastUpdated = lastUpdated;
        this.currentVersion = currentVersion;
        this.requiresAndroid = requiresAndroid;
        this.genre = genre;
        this.IAP = IAP;
        this.eSupport = eSupport;
    }

    @Override
    public String toString() {
        return "Дата последнего обновления: " + lastUpdated +
                "\nТекущая версия: " + currentVersion +
                "\nТребуемая версия Android: " + requiresAndroid +
                "\nКатегория: " + genre +
                "\nВнутриигровые покупки: " + ((IAP == null) ? "Отсутствуют" : IAP) +
                "\nСвязаться с разработчиком: " + eSupport;
 
    }
 
}

Простейший интерфейс (возможно, в будущем придется аналогичным образом парсить Apple Store):
public interface Parsable {
    Object parse(String userId, Bot bot, String URL);
}

И собственно сам парсер:
public final class GameParser implements Parsable {

    private Document document;

    private static List<String> features;

    private final static String EMAIL_REGEX = "[a-zA-Z0-9_.+-][email protected][a-zA-Z0-9-]+\\.[a-zA-Z0-9-.]+";


    public Game parse(String userId, Bot bot, String URL) {
        features = getGameInfo(userId, bot, URL);
        int size = features.size();
        return new Game(features.get(0), features.get(3), features.get(4),
                features.get(size-3), features.get(size-1), features.get(size-2));
    }

    private List<String> getGameInfo(String userId, Bot bot, String URL)  {
        try {
            document = connectToWebPage(URL);
        } catch (HttpStatusException e) {
            e.printStackTrace();
        } catch (IllegalArgumentException | IOException e) {
            e.printStackTrace();
        }
        return crawlWebPage(document);
    }

    private Document connectToWebPage(String URL) throws IllegalArgumentException, IOException{
        if (URL.endsWith("&hl=ru")){
            document = Jsoup.connect(URL).get();
        }
        else if (!URL.contains("&hl=")){
            URL += "&hl=ru";
            document = Jsoup.connect(URL).get();
        }
        else if (URL.contains("&hl=")){
            String fixedURL = URL.replace
                    (URL.substring(URL.length()-6, URL.length()), "&hl=ru");
            document = Jsoup.connect(fixedURL).get();
        }
        return document;
    }

    private List<String> crawlWebPage(Document document) {
        features = new ArrayList<>();
        Elements spanElements = document.select("div[class=IQ1z0d] > span[class=htlgb]");
        Elements aElements = document.getElementsByAttributeValue("itemprop", "genre");

        spanElements.forEach
                (element -> features.add(element.text()));
        features.add(aElements.text());
        if (checkFreeOrPaid(features))          //проверяет платная ли игра
            features.remove(0);
        features.add(getDevEmail(features));
        features.add(containsIAP(features));
        return features;
    }

    private static String getDevEmail(List<String> features){
        String email = null;
        for (String element : features){
            Matcher m = Pattern.compile(EMAIL_REGEX).matcher(element);
            while (m.find()) {
                email = m.group();
            }
        }
        return email;
    }

    private static String containsIAP(List<String> features){
        String IAP = null;
        for (String element : features){
            if (element.contains("\u20BD"))
                IAP = element;
        }
        return IAP;
    }
  
    private static boolean checkFreeOrPaid(List<String> list){
        return list.get(0).length() > 35;
    }
}


В результате условного вызова на одноразовом new GameParser().parse... получу:

Дата последнего обновления: 25 января 2018 г.
Текущая версия: 2.8.6
Требуемая версия Android: 4.1 и выше
Категория: Головоломки
Встроенные игровые покупки: От 65,00 ₽ до 5 999,00 ₽ за товар
Связаться с разработчиком: [email protected]

Этот объект и будет отправлен пользователю в виде одной объемной строки.
Стало лучше или хуже? :)
Изменен:11 фев 2020 18:46
 
 
Сообщения:468
Miroha:
а то с ними слишком объемно

можно добавить ломбок, тогда в Game останутся одни поля

Miroha:
public Game(String lastUpdated, String currentVersion, String requiresAndroid,
String genre, String IAP, String eSupport)

слишком перегружен параметрами, возможно, из следовало бы в объект упаковать

Miroha:
public Game parse(String userId, Bot bot, String URL)

очевидно, что параметры совсем не нужны - лучше передавать сразу один List<String> features в качестве параметра
и все персенье

Miroha:
return new Game(features.get(0), features.get(3), features.get(4),
features.get(size-3), features.get(size-1), features.get(size-2));

если size = 0, держите "меня 40 мужиков"

а если вообще null вернет загадочный вызов
Miroha:
features = getGameInfo(userId, bot, URL);


и еще, абсолютно не правильный подход с глобальностью поля features

Miroha:
String fixedURL = URL.replace
(URL.substring(URL.length()-6, URL.length()), "&hl=ru");

совсем неочевидный хардкод

Miroha:
features.add(getDevEmail(features));
features.add(containsIAP(features));

добавляются строки несущие неодинаковый смысл
к тому же они и могут отсутствовать
Miroha:
String email = null;

Miroha:
String IAP = null;


Miroha:
return list.get(0).length() > 35;

что ни строка кода, то ошибки на миллион

не подумайте, что я придираюсь к коду, но.. стоит хорошо над ним подумать
 
 
Сообщения:7
keekkenen:
можно добавить ломбок, тогда в Game останутся одни поля

Что такое ломбок?
keekkenen:
слишком перегружен параметрами,

Не встречал рекомендаций по количеству допустимых параметров. Для меня и товарища по крайней мере это читабельно и предельно понятно.
keekkenen:
очевидно, что параметры совсем не нужны - лучше передавать сразу один List<String> features в качестве параметра
и все персенье

Один единственный для доступа из вне метод parse без параметров? Ссылку и чат айди с луны брать в таком случае? Метод вызывается извне, из другого класса бота. Какой я ему список будут передавать? Мне список нужен для промежуточных действий (для парсинга), не более того. Возможно, вообще стоит убрать глобальную переменную списка.
keekkenen:
если size = 0, держите "меня 40 мужиков"

Не будет там 0. Если ссылка ведёт не на страницу с гугл плей, с которой невозможно спарсить, вылетит ошибка сразу и до списка дело даже не дойдет. Необходимо будет заново ввести ссылку. Все страницы в гугл плей содержат один и тот же код и структуру. Есть небольшое различие, с помощью которого можно отличить платную/бесплатную игру и всего лишь.

В принципе я могу дополнительно обработать на NPE и IndexOutOfBound.
keekkenen:
еще, абсолютно не правильный подход с глобальностью поля features

Буду рад услышать, как будет правильно.
keekkenen:
совсем неочевидный хардкод

В гугл плей можно принудительно переключать язык, дописав в конце страницы &hl=en/fr/jp/es/cn и т.д. Если ссылка подаётся в таком формате, этот кусок обрезается и добавляется принудительно &hl=ru. Ну а как мне ещё откусить и заменить в таком случае последние 6 символов?
keekkenen:
добавляются строки несущие неодинаковый смысл
к тому же они и могут отсутствовать

Все строки должны нести одинаковый смысл? Составляется объект игры, который содержит некоторую информацию. Я конечно понимаю, что версия игры и почта разработчиков это разные вещи, но объект игры содержит информацию об игре, не вижу здесь противоречий.
Отсутствовать почта не может. Все разработчики обязаны указывать на странице с игрой почту для связи.
IAP отсутствовать может, в таком случае да, запишется null. Но выведется совсем не null (метод toString).

Но наверное, тут надо было вместо null использовать значение пустой строки " ".
keekkenen:
что ни строка кода, то ошибки на миллион

Вам может быть не понятен смысл этих строк. Для меня же все предельно просто. В описании метода все прекрасно описано, в первом посте. Конечно, можно было бы обработать случай, когда список пуст например. Но я не вижу в этом смысла, так как если список пуст, вся работа парсера отвалится во время этапа подключения к веб-странице и до сюда дело точно не дойдет.

Метод "проверяет", является ли игра платной. В зависимости от того, платная игра или нет,
* в списке с информацией об игре в качестве первого элемента будет размещена либо дата
* последнего обновления, либо информация о возможности добавить игру в семейную библиотеку.
* В свою очередь, это свидетельствует о том, что игра является платной. Чтобы свести платные
* и бесплатные игры к одному варианту, метод проверяет длину {@code length} первого элемента
* в списке. Если она больше указанного порога {@value = 35}, значит игра гарантированно платная,
* поскольку строка, свидетельствующая об этом всегда содержит более 35 символов. Напротив, строка,
* которая содержит в качестве первого элемента информацию о дате последнего обновления, никак не
* может содержать в себе более 30 символов. Это всего лишь дата в формате "4 февраля 2020 г."



Да я не спорю, что это может выглядеть как говнокод. Я учусь 5 месяц и решил сделать проект, чтобы прочувствовать язык лучше.
Изменен:12 фев 2020 06:30
 
 
Сообщения:320
Miroha:
Стало лучше или хуже? :)


Стало лучше! У вас логика парсинга хорошо расположена в классе парсера. Т.е. отдельно - объект данных. Отдельно - логика (она может быть разной для разных магазинов).

Глобальные (уровня класса) изменяемые переменные желательно истребить. Для feautres уже почти все есть в вашем коде. Ее достаточно просто в двух местах объявить локальной:

        public Game parse(String userId, Bot bot, String URL) {
            List<String> features = getGameInfo(userId, bot, URL);
            // ...
       }

        private List<String> crawlWebPage(Document document) {
            List<String> features = new ArrayList<>();
            // ....
        }


После этого можно удалять поле класса. С document чуть сложнее. Основная идея - такая же - объявить в методах, где она используется:

        private List<String> getGameInfo(String userId, Bot bot, String URL)  {
            Document document; //???
            // остальной код
       }

        private Document connectToWebPage(String URL) throws IllegalArgumentException, IOException{
            Document document;
            // остальной код
       }


Сложнее - потому что компилятор станет ругаться на getGameInfo. Он будет говорить, что переменная document, возможно, не инициализирована на момент возврата (return) из метода. И это очень полезное замечание. Оно подсказывает, что нужно чуть более подробно продумать обработку ошибок. Допустим, у вас при первом запросе произошла сетевая ошибка. В этом случае исключение напечатается но код продолжит выполнение, передав в crawlWebPage значение null. Там производится работа с документом и будет NullPointerException. С одной стороны - это тоже ошибка. Но с другой - она скрывает основную причину IOException.

Вариантов исправления - много. И выбор зависит в первую очередь от того, какое именно вы хотите получит поведение в результате. Можно просто пробрасывать все исключения наверх (и добавить в сигнатуры). Можно завести свой класс исключения (ParseException) и выбрасывать его, при этом IOException будет использоваться в качестве причины (cause в конструкторе, посмотрите throwable). Можно даже попытаться сохранить текущее поведение - в случае ошибок возвращаются старые данные (если они были загружены). Это имеет смысл в определенных сценария. Только я бы в этом случае предложил разделить логику непосредственно загрузки и логику обработки ошибок (показываю только измененные места):

        private volatile Game lastSuccessfulGame;
  
        public Game parse(String userId, Bot bot, String URL) {
            try {
               Game result = doParse(userId, bot, URL);
               lastSuccessfulGame = result;
               return result;
            } catch (HttpStatusException e) {
                e.printStackTrace();
                return lastSuccessfulGame;
            } catch (IllegalArgumentException | IOException e) {
                e.printStackTrace();
                return lastSuccessfulGame;
            }

        /* Логика парсинга но не обработки ошибок. */
        public Game doParse(String userId, Bot bot, String URL) 
                throws HttpStatusException, IllelagArgumentException, IOException {
            List<String> features = getGameInfo(userId, bot, URL);
            int size = features.size();
            return new Game(features.get(0), features.get(3), features.get(4),
                    features.get(size-3), features.get(size-1), features.get(size-2));
        }
     
        private List<String> getGameInfo(String userId, Bot bot, String URL)  
                throws HttpStatusException, IllelagArgumentException, IOException {
            Document document = connectToWebPage(URL);
            return crawlWebPage(document);
        }


(при желании можно getGameInfo в отдельный метод не выделять. Можно в IDE сделать рефакторинг inline method). volatile для лучшей видимости в многопоточности (чтобы "последняя" по времени запись была всегда видна). В этом случае у нас парсер получается с состоянием. Но это в данном случае ровно то, что ожидается - в такой постановке задачи у нас есть понятие "последний успешный парсинг".

EMAIL_REGEX - это правильная глобальная константа. С ней не надо бороться :)

Все, что выше - я бы сделал. Ниже - необязательная мелочь, можно сделать по вкусу (очень зависит от задачи).

У вас парсер (интерфейс) возвращает Object. Может в вашем сценарии оно работает, но при наличии многих парсеров можно запутаться. Можно либо заставить Parsable (наверное все же Parser, Parsable - это то, что можно распарсить) возвращать Game. Либо сделать его Generic (если с ними не дружите пока - можно оставить как есть).

    public interface Parsable {
        Game parse(String userId, Bot bot, String URL);
    }

    // Или
    public interface Parsable<T> {
        T parse(String userId, Bot bot, String URL);
    }

    public final class GameParser implements Parsable<Game> {
      //...
    }     


Еще раз повторю - если вас текущий вариант устраивает (например, вы сразу на результате parse делаете toString и все) - можно оставить как есть и усложнять уже в дальнейшем.

В остальном - все хорошо. Можно попридираться к мелочам (вроде того, что будет, если у вас hl будет в середине url а не в конце). Но мне не кажется, что для вашего приложения это критично. Ограничение из кода видно. Меняться адрес будет не часто. На тестировании будет заметно. Так что смысла сильно усложнять решение я не вижу.
 
 
Сообщения:7
maxkar:
Для feautres уже почти все есть в вашем коде.

Убрал глобальную переменную features, действительно в ней нет никакого смысла.
А вот насчет document. Почему желательно тоже её убрать? Просто на данном этапе я не вижу к каким последствиям это может привести.
maxkar:
И выбор зависит в первую очередь от того, какое именно вы хотите получит поведение в результате.

Я думаю, если кривая ссылка, то мне нужно просто сообщить пользователю, чтоб тот повторил свой запрос.
Вариант с volatile интересен, потому что я в принципе не работал в подобном ключе и с таким сценарием.

Проверил такой вариант. Запустил с корректной ссылкой, а второй запрос с некорректной. По идее во втором запросе он должен вернуть результат первого запроса? Он возвращает null. Возможно из-за логики connectToWebPage. Там если передать любую неправильную ссылку, он автоматически лепит в конец ссылки &hl=ru. Т.е. если грубо говоря я прошу его перейти по ссылке https://play.google.com/ и распарсить её, то он лепит https://play.google.com/&hl=ru и выбивает HTTPStatusException (ошибка 404, страницы нет).

maxkar:
Можно завести свой класс исключения (ParseException) и выбрасывать его, при этом IOException будет использоваться в качестве причины (cause в конструкторе, посмотрите throwable).

Насколько я понимаю, речь идет о цепочке исключений? С ними я не сталкивался на практике.
Примерно так должно выглядеть мое исключение?
public class ParseException extends Exception{
    public ParseException(String message, Throwable cause) {
        super(message, cause);
    }
}

И далее я делаю что-то типа такого?
catch (IOException e){
            throw new ParseException("Invalid link", e)
        }

maxkar:
Либо сделать его Generic (если с ними не дружите пока - можно оставить как есть).

Вполне дружим, исправлю. :)
maxkar:
Еще раз повторю - если вас текущий вариант устраивает (например, вы сразу на результате parse делаете toString и все) - можно оставить как есть и усложнять уже в дальнейшем.

Возможно в будущем будет такая идея: сделать хешмапу в отдельном файле и записывать туда пару <String, Game>, где String ключ и отвечает за название игры, а Game - собственно объект с игрой. По ссылке отдельно парсить официальное название из магазина. Далее пользователь уже может без ссылки, а просто по названию игры получить информацию. Если выходит обновление игры, обновлять значение по ключу. Была идея отдельно сделать сеттеры для полей, но тогда надо придумывать команды для бота, да и копаться в них. По сути, для каждого сеттера отдельная команда. Наверное, проще снова спарсить страницу и обновить информацию, а далее закинуть новый объект. Другой вопрос, как отслеживать изменения. Просто: при каждом новом запросе на уже имеющуюся игру, сверять объекты и обновлять в случае необходимости. Сложно: отслеживать изменения в автоматическом режиме. Также дополнительно можно спарсить информацию об обновлении, что нового было добавлено.
Изменен:12 фев 2020 11:49
 
 
Сообщения:320
Miroha:
А вот насчет document. Почему желательно тоже её убрать?


Все та же потокобезопасность. Если вы на каждый запрос создаете парсер - это не проблема. А вот если один экземпляр используете - проблемы могут быть. Возьмем, например, getGameInfo. Я убрал обработку исключений, для примера гонки это не важно.

    private List<String> getGameInfo(String userId, Bot bot, String URL)  {
        // позиция 1
        document = connectToWebPage(URL);
        // позиция 2
        return crawlWebPage(document);
    }


Допустим у нас одновременно выполняется два потока - A и B. И, допустим, у нас один процессор (так проще). Поток A дошел до позиции 1 и ОС решила, что ему пока хватит выполняться. Она дает выполняться немного потоку B, который доходит до той же позиции. Планировщик (ОС) опять выполняет переключение потоков и выполняет A. Он выполняет метод и присваивание document значениe doc1. После чего планировщик переключается на поток B, который выполняет те же операции и присваивает document значение doc2. Теперь у нас оба потока в позиции 2. При этом в document записано значение doc2. Когда поток A продолжит выполнение, он передаст doc2 в crawlWebPage (это текущее значение document, оно перезаписано потоком B).

В реальной многопоточности гонка примерно такая же - достаточно, чтобы в "позиции 2" пришло два потока. Тогда _оба_ они прочитают значение, записанное последним. На самом деле в JVM гарантии еще хитрее (там еще более запутанные эффекты бывают), но для некорректного поведения уже и этого достаточно.

Возможные гонки - это основная причина, по которой в многопоточном коде стоит избегать изменяемого состояния.

Во многих случаях передача через глобальные переменные еще и сложнее в чтении. При вызове метода все его параметры видны. А вот с переменными класса можно долго искать место, где происходит инициализация. В случае сложных цепочек вызовов методов это может быть целое приключение. final поля (static и инициализируемые в конструкторе) таких сюрпризов не преподносят - значение всегда устанавливается в начале. Конкретно у вашего кода проблем с чтением нет - document по сути используется как локальная переменная (вы его уже везде передаете все равно, как и с features было). Но потенциальные проблемы с многопоточностью остаются.

Miroha:
Я думаю, если кривая ссылка, то мне нужно просто сообщить пользователю, чтоб тот повторил свой запрос.

Это да. Я говорил про случаи, когда временно пропал интернет или сайт сломался. Т.е. ссылка правильная но запрос не работает по независящим от пользователя причинам.

Miroha:
Он возвращает null.

Может быть я что-то пропустил :) Пока не вижу как он может вернуть null, но верю.

Miroha:
Насколько я понимаю, речь идет о цепочке исключений? Примерно так должно выглядеть мое исключение?

Да, тут все абсолютно правильно!

Дальнейшее направления развития тоже отлично! Вы все правильно делаете - получить что-нибудь рабочее и потом усложнять. Parseable - хороший интерфейс. Он не полагается на побочные эффекты (т.е. установку полей внутри) и просто возвращает данные. В дальнейшем какие-то вещи могут усложняться и меняться, но текущую реализацию все равно можно будет использовать для начальной загрузки данных. А как проверять обновления и прочее - уже решите по ходу разработки, это все извне базового парсера!
 
 
Сообщения:7
maxkar, приветствую :)

Решил немного переделать класс. Пришел к тому, что пусть лучше класс возвращает список, а конструировать непосредственно объект уже в другом классе. В том же классе собственно будет заведена хешмапа, в которую будут закидывать название игры и сам объект с игрой.
public List<String> parse(String userId, Bot bot, String URL) {
        return getGameInfo(userId, bot, URL);
    }

Дополнительно ввел ещё два метода: сократил crawlWebPage, который теперь только достает информацию со страницы и ничего более.
 private List<String> crawlWebPage(Document document) {
        Elements spanElements = document.select("div[class=IQ1z0d] > span[class=htlgb]");    //основная информация
        Elements genre = document.getElementsByAttributeValue("itemprop", "genre");             //парсинг жанра
        Elements title = document.getElementsByAttributeValue("itemprop", "name");               //парсинг названия
        return fillList(spanElements, genre, title);
    }

1:
private List<String> fillList(Elements spanElements, Elements genre, Elements title) {
        List <String> features = new ArrayList<>();
        spanElements.forEach
                (element -> features.add(element.text()));
        if (checkFreeOrPaid(features))          //проверяет платная ли игра
            features.remove(0);
        features.add(genre.text());
        features.add(getDevEmail(features));
        features.add(containsIAP(features));
        features.add(title.text());
        return modifyList(features);
    }

2:
private List<String> modifyList(List<String> features) {
        int size = features.size();
        if (size == 0)
            throw new IllegalStateException();      //здесь можно добавить в логгер
        List<String> modifiedList = new ArrayList<>();
        modifiedList.add(features.get(size-1));     //название игры
        modifiedList.add(features.get(0));          //дата последнего обновления
        modifiedList.add(features.get(3));          //текущая версия
        modifiedList.add(features.get(4));          //требуемая версия Android
        modifiedList.add(features.get(size-4));     //жанр
        modifiedList.add(features.get(size-2));     //встроенные покупки
        modifiedList.add(features.get(size-3));     //обратная связь
        return modifiedList;
    }

Собственно сначала я заполняю список, а далее уже модифицирую список таким образом, что в нем останется только нужная информация. По-другому конкретные позиции со span'а не достать. Конечно можно там каждый элемент проверять на типо contains, но по-моему проще все закинуть в список, а потом оттуда достать нужное. С индексами проблем не будет. Для каждой отдельно взятой игры расположение необходимых элементов в исходном списке (их индексы) остается неизменным. Это обеспечивается структурой и порядком элементов на странице в Google Play. Конечно, если в один прекрасный день они изменят структуру страницы, всё полетит к чертям, но такой случай предусмотреть кажется невозможным. Просто придется переделывать логику.

Возникли трудности с обработкой исключений, с их логикой:
private List<String> getGameInfo(String userId, Bot bot, String URL)  {
        Document document = null;
        try {
            document = connectToWebPage(URL);
        } catch (HttpStatusException e) {
            System.out.println("Страница не доступна");
        } catch (IllegalArgumentException | MalformedURLException | UnknownHostException e) {
            try {
                throw new GooglePlayParseException("Неверная ссылка!", e);
            } catch (GooglePlayParseException ex) {
                System.out.println("Неверная ссылка " + ex.getCause());
            }
        } catch (IOException e){
            e.printStackTrace();
        }
        if (document != null) {
            return crawlWebPage(document);
        }
        else throw new NullPointerException("СТОП!");
    }

Правильно ли я прикрутил своё исключение? По идее, если ссылка неверная, то я выбрасываю свое исключение, которое было вызвано тем, что юзер ввел неправильную ссылку. Неверная ссылка в разных ситуациях выплевывает IllegalArgument, MalformedURL или Host. Отдельный случай - HttpStatus. И выходит, что все эти исключения вызваны IOException? Ведь ошибка при вводе данных. Как сделать лучше: пробросить собственное исключение дальше или обработать сразу же в try/catch?
Сделал еще типо "проверку" на null, если его нет, тогда вызываем следующий метод иначе выбрасываю исключение.
И да, здесь сделал с sout'ами, а в проекте надо будет посылать сообщение пользователю через объект бота. Но вроде кардинально не должно ничего измениться.
Изменен:13 фев 2020 12:43
 
 
Сообщения:320
Miroha:
Решил немного переделать класс


Здесь все хорошо. Мне нравится, как там осталась минимально необходимая логика по разбору.

Miroha:

Правильно ли я прикрутил своё исключение? ...
И да, здесь сделал с sout'ами, а в проекте надо будет посылать сообщение пользователю через объект бота.


Я про планы на обработку изначально не знал, я думал, это просто заглушка временная (в реальных проектах просто вывод не делают обычно). А момент принципиальный, смотрите далее.

В вашем решении вы "обрабатываете" исключение путем вывода сообщения. Потом будет отправка сообщения - это тоже можно считать обработкой. В данном случае свое отдельное исключение не нужно - оно ничего не дает. Нет смысла его создавать и потом сразу ловить, можно использовать оригинальное исключение. Клиенты новое исключение не видят, вместо этого они получают NullPointerException (можно еще null вернуть и обязать их обрабатывать такую ситуацию). С точки зрения обработки исключений здесь все есть.

Но! Давайте теперь посмотрим, как распространяется информация. Если вдруг данные удалось получить, их в телеграм выводит клиент парсера (или кто-то еще выше по цепочке вызовов). А вот ошибки выводит уже сам парсер. При этом клиент все равно как-то обрабатывать ситуации, когда данные не доступны (null или NullPointerException). Даже если ситуацию просто игнорировать - это отдельный код. Поэтому у вас обработка отказов получается "размазана" по разным классам. Может быть удобнее не обрабатывать ошибки в парсере и делегировать это клиентам выше по стеку вызовов. В этом случае парсер не зависит ни от чего (его можно и для других проектов использовать). Взаимодействием с телеграмом (и в случае успеха, и в случае неудачи) в этом случае занимается клиент парсера.

Вот во втором случае свои исключения будут полезны - они пробрасываются наверх и позволяют клиенту не заботиться о том, как именно реализован парсер. Т.е. у вас код будет выглядеть примерно так:

    
        private List<String> getGameInfo(String userId, Bot bot, String URL)  
               throws BadUserInputException, CommunicationException {
            try {
                Document document = connectToWebPage(URL);
                return crawlWebPage(document);
            } catch (IllegalArgumentException | MalformedURLException | UnknownHostException e) {
                    throw new BadUserInputException("Неверная ссылка!", e);
            } catch (HttpStatusException | IOException e) {
                     throw new CommunicationException(e);
            }
        }


Какие исключения заводить? Зависит от того, что вы предполагаете будет полезно клиенту и предполагает разные способы обработки. Например, в моем примере я различаю две ситуации. Первая - неправильный адрес. Допустим, ссылка вводится пользователем - в этом случае можно будет выдать сообщение о том, что ссылка не валидна. Ситуация, когда недоступна сеть, другая. Пользователь не сможет "исправить опечатку" и ввести новую ссылку. Поэтому сообщение может быть другим. Клиент парсера может даже решить, что в случае ошибок связи попытку можно повторить (что не имеет смысла для невалидного URL). UnknownHostException в этом случае доставляет сложности - это может быть временная ошибка сети, а может - неправильный ввод. Приходится делать выбор.

Есть и другой вариант. Например, у вас ссылка берется из файла конфигурации и не меняется во время работы программы. Клиент парсера ничего не может сделать для исправления ошибки. Нужно исправлять сеть или звонить программистам, чтобы исправили настройки. В этом случае один IOException может покрыть все возможные варианты.

Поэтому ответы на следующие вопросы:

Miroha:
И выходит, что все эти исключения вызваны IOException?
HttpStatus и сам IOException - да, вызваны IOException (обычно HttpStatus наследник IOException). IllegalArument и MailformedURL - нет, не вызваны. Это отдельный класс ошибок конфигурации/ввода, хотя в некоторых случаях их можно обрабатывать так же, как и IOException. UnknownHost - спорный. Он может быть вызван и неправильным вводом, и временной недоступностью сети. К сожалению, надежных способов определить, кто именно виноват - нет. Придется делать выбор о том, как лучше обрабатывать.

Miroha:
Как сделать лучше: пробросить собственное исключение дальше или обработать сразу же в try/catch?

Пробрасывать. Выбрасывать и обрабатывать сразу же нет смысла. С тем же успехом можно обработать исходное исключение. Свои исключения нужны, чтобы облегчить жизнь клиентам сервиса, изолировать их от деталей реализации и при необходимости спрятать лишние варианты ошибок.
 
 
Сообщения:7
maxkar:
Здесь все хорошо. Мне нравится, как там осталась минимально необходимая логика по разбору.

Я похоже психанул, но переделал парсер еще раз заново. :D Когда пишешь с нуля, пишешь то, что первое взбрело в голову, а потом уже рождаются новые идеи..
Мне кажется я уже куда-то не туда лезу (главное вовремя остановиться), но рассказываю, что взбрело в голову мне на сей раз.
Сделал отдельный класс для подключения (как там в SOLID - один класс = одна ответственность?)
public final class GooglePlayConnection {

    public Document connectToGooglePlay(String URL) throws URISyntaxException, IOException, InvalidLinkException {
        Document document = null;
        final URI link = new URI(URL);
        if (GooglePlayCorrectURL.isLinkValid(link)){
            if (URL.endsWith("&hl=ru"))
                document = Jsoup.connect(URL).get();
            else {
                if (URL.contains("&hl=")){
                    URL = URL.replace(
                            URL.substring(URL.length()-6), "&hl=ru");
                    document = Jsoup.connect(URL).get();
                }
                else {
                    URL += "&hl=ru";
                    document = Jsoup.connect(URL).get();
                }
            }
        }
        else
            throw new InvalidLinkException();
        return document;
    }

}

Отдельный класс для проверки, а ведет ли ссылка в Google Play:
public class GooglePlayCorrectURL {
    private static final String VALID_HOST = "play.google.com";
    private static final String VALID_PROTOCOL = "https";
    private static final int VALID_PORT = -1;

    public static boolean isLinkValid(URI link){
        return (link.getHost().equals(VALID_HOST) &&
                link.getScheme().equals(VALID_PROTOCOL) &&
                link.getPort() == VALID_PORT);
    }
}

И собственно сам парсер:
public final class GooglePlayParser {
    private final static String EMAIL_REGEX = "[a-zA-Z0-9_.+-][email protected][a-zA-Z0-9-]+\\.[a-zA-Z0-9-.]+";

    public Elements getFeatures(Document document) {
        Elements features = document.select("div[class=IQ1z0d] > span[class=htlgb]");
        return features;
    }

    public String getGenre(Document document){
        Elements genre = document.getElementsByAttributeValue("itemprop", "genre");
        return genre.text();
    }

    public String getTitle(Document document){
        Elements title = document.getElementsByAttributeValue("itemprop", "name");
        return title.text();
    }

    public String getIAP(Elements features){
        String IAP = "Отсутствуют";
        for (String element : features.eachText()){
            if (element.contains("\u20BD"))
                IAP = element;
        }
        return IAP;
    }
    public String getDevEmail(Elements features){
        String email = "support.google.com";
        for (String element : features.eachText()){
            Matcher m = Pattern.compile(EMAIL_REGEX).matcher(element);
            while (m.find()) {
                email = m.group();
            }
        }
        return email;
    }
    public boolean isPaid(List<String> features){
        return features.get(0).length() > 35;
    }
}

По сути получилось что-то вроде набора геттеров. Сделал их на тот случай, если вдруг понадобится что-то отдельное достать, например почту разработчика.
Еще завел интерфейс Game (по сути маркерный, без всего) и сделал класс для объекта готового.
public final class GooglePlayGame implements Game {
    @SerializedName("Название игры")
    private final String title;
    @SerializedName("Дата последнего обновления")
    private final String lastUpdated;
    @SerializedName("Текущая версия")
    private final String currentVersion;
    @SerializedName("Требуемая версия Android")
    private final String requiresAndroid;
    @SerializedName("Категория")
    private final String genre;
    @SerializedName("Внутриигровые покупки")
    private final String IAP;
    @SerializedName("Связаться с разработчиком")
    private final String eSupport;
    @SerializedName("Ссылка на скачивание")
    private final String downloadLink;

    private GooglePlayGame(String title, List<String> features, String genre, String IAP, String eSupport, String downloadLink)  {
        this.title = title;
        this.lastUpdated = features.get(0);
        this.currentVersion = features.get(3);
        this.requiresAndroid = features.get(4);
        this.genre = genre;
        this.IAP = IAP;
        this.eSupport = eSupport;
        this.downloadLink = downloadLink;
    }

    public static Game factory (String URL, String downloadLink) throws InvalidLinkException, IOException, URISyntaxException {
        return constructGame(URL, downloadLink);
    }

    private static Game constructGame(String URL, String downloadLink) throws InvalidLinkException, IOException, URISyntaxException {
        Document document = new GooglePlayConnection().connectToGooglePlay(URL);
        Elements elements = new GooglePlayParser().getFeatures(document);
        List<String> features = new GooglePlayParser().getFeatures(document).eachText();
        if (new GooglePlayParser().isPaid(features)){
            features.remove(0);
        }
        String title = new GooglePlayParser().getTitle(document);
        String genre = new GooglePlayParser().getGenre(document);
        String IAP = new GooglePlayParser().getIAP(elements);
        String eSupport = new GooglePlayParser().getDevEmail(elements);
        return new GooglePlayGame(title, features, genre, IAP, eSupport, downloadLink);
    }

    @Override
    public String toString() {
        return "Название игры: " + title +
                "\nДата последнего обновления: " + lastUpdated +
                "\nТекущая версия: " + currentVersion +
                "\nТребуемая версия Android: " + requiresAndroid +
                "\nКатегория: " + genre +
                "\nВнутриигровые покупки: " + IAP +
                "\nСвязаться с разработчиком: " + eSupport +
                "\nСсылка на скачивание: " + downloadLink;
    }

}

Если я правильно понял, у меня получилась фабрика, точнее фабричный статический метод factory, который возвращает полноценный объект с игрой. Ну и пометил поля аннотациями для будущих удобств с сериализацией.

Короче итог: если хочешь готовый объект с игрой со всей заполненной информацией - вызываешь фабричный метод и получаешь объект. Если тебе нужна только почта, только название и т.д. - используешь "геттеры" из класса. Подключение вынесено в отдельный класс, все исключения пусть обрабатывает тот, кто меня вызывает.
Почему пошел по пути фабричного метода? Если делать делать через конструктор, ему надо сразу готовые параметры передавать для полей. А если хочу сразу получить готовый объект, как я про них узнаю? Сначала предварительно придется получить все эти параметры. Ну в общем, как-то скрыл всё это дело. Вызываешь метод и получаешь объект. Идея была в общем такой. :)

Ваши пояснения насчет исключений понял, принял. :)
Изменен:19 фев 2020 18:02
 
Модераторы:frymock
Сейчас эту тему просматривают:Нет