1
\$\begingroup\$

Scraping news articles from various web page is the main goal of this code. For starting all spiders(web crawlers) at the same time, I try to implement the facade design pattern. It works but I feel the constructor of SpiderFacade class can be improved. Is there better way to do it or better design pattern to do it ?

Abstract Spider

public abstract class NewsSpider {
    public void crawl() {
    }
}

Custom Spider1

public class NYTimesSpider extends NewsSpider {

   private final NewsService newsService;

   public NYTimesSpider(NewsService newsService) {
       this.newsService = newsService;
   }

   @Override
   public void crawl() {
       // crawl logic
   }
}

Custom Spider2

public class BbcSpider extends NewsSpider {

   private final NewsService newsService;

   public BbcSpider(NewsService newsService) {
       this.newsService = newsService;
   }

   @Override
   public void crawl() {
       // crawl logic
   }
}

Facade Class

public class SpiderFacade {

    private final List<NewsSpider> spiderList;

    public SpiderFacade(NewsService newsService) {
        spiderList = Arrays.asList(
                new NYTimesSpider(newsService),  // the wrong part in my opinion
                new BbcSpider(newsService)
        );
    }

    public void startCollectingNews() {
        spiderList.forEach(NewsSpider::crawl);
    }
}

Starting Spiders

 SpiderFacade spiderFacade = new SpiderFacade(newsService);
 spiderFacade.startCollectingNews();
\$\endgroup\$
1
  • \$\begingroup\$ The only "wrong" things I can notice is that you don't pass the services as constructor parameters (Inversion of control). From my knowledge I would say that it is not a facade but a composite and, thus, your SpiderFacade must implement/extend NewsSpider and accept an unbound number of NewsSpider. \$\endgroup\$ – gervais.b May 28 '20 at 6:51
1
\$\begingroup\$

From the high level I don't see anything wrong in your constructor. But I have some improvement to propose:

Use interface

Your NewsSpider is an abstract class with one empty method. If you want to force the implementation of the crawl method make it abstract. And if your class has no implementations, make it an interface.

interface NewsSpider {
    void crawl();
}

But interfaces with single methods are also function interfaces. In this case You can replace your NewsSpider with Runner because it also provide one method that returns nothing. But I guess you would at least returns a set of news. So your news spider can be seen has a Supplier<News>.

Inversion of control

Instead of creating the NYTImeSpider and BBCSpider inside the SpiderFacade pass a list or varargs of NewsSpider :

public SpiderFacade(NewsSpider... spiders) {
    this.spiderList = Arrays.asList(spiders);
}

By doing so you improve the evolvability of your code because you can easily provide another spider. You also improve the testability because you can pass mocks to your facade to test his behavior.

Composite

I have the feeling that what you are building is more a composite than a facade. With the composite your program will be much more modular because you can use a single crawler or many with the same interfaces.

CompositeSpider implements Supplier<Set<News>> {
   // ...
   CompositeSpider(Supplier<Set<News>>... components) {
       this.components = Arrays.asList(components);
   }

   public Set<News> get() {
        // get and aggregate news from all components
   }      
} 

By doing that you increase the modularity. You can later create another implementation that remove duplicates and compose all of them.

Set<News> uniqueNews = new DuplicatesPruner(
    new CompositeSpider(
        new NYTimeSpider(),
        new BBCSpider()
    )
).get();

And because all the crawlers share the same interfaces you can easily switch them and pass them to another service:

class NotificationService {
    void sendNews(Function<Set<News>> crawler)
}

class AsyncNewsService {
    CompletableFuture<Set<News>>> fetch(Function<Set<News>> crawler)
}
\$\endgroup\$
0
1
\$\begingroup\$
  1. Use interface instead of class with single method
  2. Each concrete implementation should be a singletone
  3. Each concrete implementation should contain only logic and accept your current news in crowler() method: do not recreate a strategy instance for each news.
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Welcome to CodeReview! Please also explain why your alterenatives are is better, so the OP can learn form it :) \$\endgroup\$ – RobAu May 28 '20 at 6:57

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.