Lenta grabber by ilchuk96 · Pull Request #9 · flame-stream/FlameNews

ilchuk96 · 2018-10-16T17:38:46Z

No description provided.

mrMakaronka

Основное: нет отправки XMPP серверу

mrMakaronka · 2018-10-17T11:59:49Z

+        <dependency>
+            <groupId>org.apache.httpcomponents</groupId>
+            <artifactId>httpclient</artifactId>
+            <version>4.2.3</version>


Версии мы задаем в parent.pom

mrMakaronka · 2018-10-17T12:00:42Z

+
+public class LentaGrabber {
+
+    private static void addNewFile (String dirPath, News news) {


Это функционал, который не относится напрямую к LentaGrabber. Давайте сделаем для него интерфейс и вынесем отдельно.

mrMakaronka · 2018-10-17T12:01:13Z

+                System.out.println("Directory created");
+            }
+            catch(SecurityException e){
+                System.out.println(e.getMessage());


Давайте логировать не sout-ами, а Logger-ом

в этом случае дальнейшее исполнение вряд ли имеет смысл, поэтому лучше бросить RuntimeException

catch(SecurityException e){ throw new RuntimeException(e); }

mrMakaronka · 2018-10-17T12:03:50Z

+            try {
+                num = Integer.parseInt(name.substring(0, name.lastIndexOf(".")));
+            } catch (NumberFormatException e) {
+                continue;


Если мы здесь обломались, наверное что-то пошло не так и продолжать не стоит

Я имел в виду, что если что-то (зачем-то) положил в директорию ../news/ что-то странное с дурацким именем, то он, конечно, человек нехороший, но ну и пусть. Но если не пусть, то подправлю.

mrMakaronka · 2018-10-17T12:04:10Z

+            writer.flush();
+        }
+        catch (IOException e) {
+            System.out.println(e.getMessage());


То же, что и выше

mrMakaronka · 2018-10-17T13:16:06Z

+public class LentaGrabber {
+
+    private static void addNewFile (String dirPath, News news) {
+        int curNameOfFile = 0;


Между объявлением переменной и ее использованием как-то слишком много левого кода :)

mrMakaronka · 2018-10-17T13:18:15Z

+    public static void main(String[] args) throws IOException, JAXBException, InterruptedException {
+        Item oldItem = null;
+        for(;;) {
+            List<News> news = new ArrayList<>();


Зачем нужен этот список?

может быть несколько новостей за последнюю минуту

mrMakaronka · 2018-10-17T13:19:59Z

+            Unmarshaller jaxbUnmarshaller = jaxbContext.createUnmarshaller();
+            RSS newrss = null;
+            if (oldItem == null) {
+                RSS rss = (RSS) jaxbUnmarshaller.unmarshal(xml);


Если нет oldItem мы просто забиваем и ничего не делаем?

подразумевается что это происходит один раз на первой итерации

mrMakaronka · 2018-10-17T13:21:33Z

+                List<Item> newItems = newrss.getChannel().getItems();
+                int num = 0;
+                for (int i = 0; i < newItems.size(); i++) {
+                    if (newItems.get(i).getPubDate().equals(oldItem.getPubDate())) {


А pubDate точно уникальная?

неизвестно

добавил еще проверку заголовка

mrMakaronka · 2018-10-17T13:23:53Z

+                        System.out.println(newrss.getChannel().getItems().get(i).getTitle());
+                    }
+                }
+                for (Item i : newrss.getChannel().getItems().subList(0, num)) {


Я не очень понимаю, зачем здесь нужен subList

в newrss 200 последних новостей, это чтобы выбрать только не обработанные ранее

mrMakaronka · 2018-10-17T20:33:05Z

+    final String URL_LENTA = args[0]; // "https://lenta.ru/rss/news";
+    final String directory = args[1]; //  "../news/";
+
+    List<News> news = new ArrayList<>();


Почему мы не можем каждую новость обрабатывать без записи в список? Мы же просто записываем в него, а потом сразу читаем и чистим. Какой в этом смысл?

mrMakaronka · 2018-10-17T20:33:30Z

+    final String directory = args[1]; //  "../news/";
+
+    List<News> news = new ArrayList<>();
+    StreamSource xml = new StreamSource(new StringReader(sendRequest(URL_LENTA)));


Некоторые переменные все еще не final

mrMakaronka · 2018-10-17T20:34:58Z

+      RSS newrss = null;
+      if (oldItem == null) {
+        RSS rss = (RSS) jaxbUnmarshaller.unmarshal(xml);
+        oldItem = rss.getChannel().getItems().get(0);


Это не круто, я не хочу ждать минуту при тестировании

mrMakaronka · 2018-10-17T20:39:31Z

+    Unmarshaller jaxbUnmarshaller = jaxbContext.createUnmarshaller();
+    Item oldItem = null;
+    for(;;) {
+      RSS newrss = null;


Зачем здесь объявлять переменную?

mrMakaronka · 2018-10-17T20:42:13Z

+        for (int i = 0; i < newItems.size(); i++) {
+          if (newItems.get(i).getPubDate().equals(oldItem.getPubDate())
+                  && newItems.get(i).getTitle().equals(oldItem.getTitle())) {
+            num = i;


Зачем нам вообще нужен num? Почему нельзя в этом цикле просто вызывать addNewFile(directory, new News(...));?

mrMakaronka · 2018-10-17T20:44:14Z

-            writer.flush();
+        if (num > 0) {
+          log.info("news: " + num);
+          for (int i = 0; i < num; i++) {


этот цикл тогда не понадобится

mrMakaronka · 2018-10-17T20:44:26Z

-        String line = "";
-        while ((line = rd.readLine()) != null) {
-            resp.append(line);
+        for (Item i : newrss.getChannel().getItems().subList(0, num)) {


И этот тоже

mrMakaronka · 2018-10-17T20:46:02Z

+    }
+  }
+
+  static String sendRequest(String url) throws IOException {


У этого метода ровно одно использование, почему он не заинлайнен?

mrMakaronka · 2018-10-17T20:46:28Z

+
+  static String sendRequest(String url) throws IOException {
+    HttpClient client = new DefaultHttpClient();
+    HttpGet request = new HttpGet(url);


chugaev and others added 3 commits October 16, 2018 14:27

add lenta grabber

1774c38

add mvn

da7f1e9

Saves news to lenta-grabber/news

5e36938

ilchuk96 requested a review from mrMakaronka October 16, 2018 17:38

mrMakaronka requested changes Oct 17, 2018

View reviewed changes

mrMakaronka and others added 2 commits October 17, 2018 16:36

Add lenta-grabber as module

fbb70e8

fix issues

5ddb9d1

mrMakaronka requested changes Oct 17, 2018

View reviewed changes

chugaev and others added 7 commits October 18, 2018 13:30

add dependency to parent

fe9a54e

fix issues

890fe34

small fix

bdb9349

added saving interface

2775d54

added news saver

19ca3bf

Slight fixes

e35f044

added dateTime and title check

c737907


		public class LentaGrabber {

		private static void addNewFile (String dirPath, News news) {

Conversation

ilchuk96 commented Oct 16, 2018

Uh oh!

mrMakaronka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants