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

1st non-Ad search item #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

superburn
Copy link

Test3 commit

@caojiele
Copy link

caojiele commented Jul 8, 2019

Hi, I come from two aspects of code review, respectively from the Angle of mission requirements to realize and code implementation point of view to review.

First, requirements implementation point of view. I think have the following:

  • Need to define a mechanism provides the user to enter search keywords.My understanding is that there should be a simple web page, provide an input box, a search button.
  • Search results to show in the web page.
  • We all know that the UT coverage must > 80% if not 100%, Code test cases are too simple, coverage rate is far lower than 80%.

And then, code implementation point of view. I think have the following:

  • baidu.com page elements rules is variable, and suggested that "the first advertisement search results" matching rules designed to configure, instead of writing in the code.
  • WebPageUtil class responsibilities advice designed to general tools, rather than a specific business code coupling.
  • Considering the expansibility, KeyWordSearchService should be designed as a interface, to support different search sites of their implementation.

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