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

Group all checker which only check the value of one field #2437

Merged
merged 2 commits into from
Jan 15, 2017

Conversation

tobiasdiez
Copy link
Member

Most of the integrity checker only work on one field and thus I introduced a new abstract class which groups the common functionality.
(I know we don't want pure refactor PRs anymore, sorry for that)

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)
  • If you changed the localization: Did you run gradle localizationUpdate?

@tobiasdiez tobiasdiez added type: code-quality Issues related to code or architecture decisions status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Dec 28, 2016
@simonharrer
Copy link
Contributor

To me, the code is worse than before. Less clear, less self-contained. And just for saving a few lines of code which are very simple. Just my 2 cents.

@tobiasdiez
Copy link
Member Author

Thanks @simonharrer for your feedback. Besides "saving a few lines of code" I had the following motivation in mind:

  • Invoke these FieldChecker as validators in the entry editor (i.e. show errors directly while editing an entry)
  • Move from strings to valued types. Most of the checkers work with strings, instead of using data objects (for example, they use regular expressions to match pages instead of parsing the string to a Page object and let this class check if it's a normalized page string; the ISSN checker is an exception and in a style I prefer). My hope was that all this parsing can also be moved to the parent FieldChecker class, so that each checker only operates on a parsed object (like ISSN).

But probably all these things can also be accomplished without the FieldChecker as abstraction layer. @JabRef/developers if you think that it over-complicates things, just close this PR.

@simonharrer
Copy link
Contributor

Ok, I see. Thanks for clarification. The PR makes more sense to me now. :-)

In that case, I would vote for a FieldChecker interface that is implemented by each of these classes you refactored. Then, another class can bridge between a Checker and a FieldChecker like an adapter. The FieldCheckers can then be easily used within the entry editor.

Working with generics could be the way to go for your second point, but I am unsure how this will look. The idea behind this change could be to use the decorator pattern. On the outside, there is a String FieldChecker, and on the inside an ISSN FieldChecker, for instance.

@tobiasdiez
Copy link
Member Author

Thanks @simonharrer for this suggestion! I now implemented the FieldCheckers via the adapter pattern and think it is more clear than it was before.
Hopefully you finish your thesis quickly so that you have more time for JabRef again 😄

@tobiasdiez tobiasdiez merged commit f572318 into JabRef:master Jan 15, 2017
@tobiasdiez tobiasdiez deleted the refactorChecker branch January 15, 2017 14:50
Siedlerchr added a commit that referenced this pull request Jan 18, 2017
* upstream/master:
  Save deletion of current searchquery (#2469)
  Update dev dependencies (mockito, wiremock, assertj)
  Update BouncyCastle (1.55->1.56), ANTRL4 (4.5.3->4.6), jsoup (1.10.1->1.10.2)
  Group all checker which only check the value of one field (#2437)
  Follow up #2428 (#2438)
  Fix conversion of "'n" and "\'{n}" from LaTeX to Unicode (#2464)
  Fix failing tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers type: code-quality Issues related to code or architecture decisions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants