Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Linked crawler with database #83

Merged
merged 5 commits into from
Nov 28, 2016
Merged

Linked crawler with database #83

merged 5 commits into from
Nov 28, 2016

Conversation

Rhymmor
Copy link
Collaborator

@Rhymmor Rhymmor commented Nov 26, 2016

Краулер теперь добавляет в бд нашедший мем или обновляет, если он уже есть #82. Добавлен базовый сервис с операцией сохранения и удаления, чтобы не работать напрямую с дао #81. Все сервисы перемещены в в отдельный пакет api/memo/services.

@Rhymmor
Copy link
Collaborator Author

Rhymmor commented Nov 26, 2016

Есть вопрос по моей реализации. В задаче #81 я описал вопрос. В итоге я сделал несколько сервиcов, но из-за этого получились довольно перегруженными своими параметрами классы ActionCrawler и CrawlerRunner. Может есть решение лучше, чем это?

@aonnikov
Copy link
Contributor

Проблема большого количества параметров решается вынесением этих параметров в отдельный класс, скажем CrawlingContext, который передается в ActionCrawler и CrawlerRunner.

@aonnikov
Copy link
Contributor

Небольшое уточнение, CrawlingContext должен содержать только необходимые сервисы и т.д. Данные необходимые для краулинга (искомую строку, список адресов и прочее) следует передавать в отдельном объекте.

@Rhymmor
Copy link
Collaborator Author

Rhymmor commented Nov 26, 2016

Ну да, я уже объединял параметры краулера в класс CrawlerSettings, просто подумал, что проблема может быть в проектировании.

<dependency>
<groupId>edu.uci.ics</groupId>
<artifactId>crawler4j</artifactId>
<version>LATEST</version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не стоит использовать LATEST версию, особенно для сторонних зависимостей. Рлюс версию нужно вынести в properties.

public class DummyMemoMatchingImpl implements MemoMatching {
@Override
public boolean match(Memo candidate) {
if (candidate.getCounter() > 5L) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Нужно вынести магические числа в константы

* Created by Anatoly on 26.10.2016.
*/
public interface MemoParser {
void parse(String text, Memo candidate);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

На мой взгялд, неудачный интерфейс. Его предназначение - загадка для меня. Что парсим и зачем? А как данные вернуть? Ниже в реализации интерфейса я вижу, что данные возвращаются через параметр candidate, но это очень неявно.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ну название видимо неудачное. Он нужен для того, чтобы из текста вытаскивать упоминания о меме каким-либо образом.


@Override
public void deleteMemo(Memo memo) {
List memosByTitle = searchMemoService.findMemosByTitle(memo.getTitle());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А давай сделаем так, чтобы SearchMemoService.findMemosByTitle возвращал List?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List<Memo> я имел ввиду

private BasicMemoService basicMemoService;
private SearchMemoService searchMemoService;

@Autowired
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А зачем здесь @Autowired?

@DVEfremov DVEfremov merged commit 5aa5fbc into master Nov 28, 2016
@sumboid sumboid deleted the issue#82 branch December 1, 2016 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants