Remove teamscale findings in the database package by tschechlovdev · Pull Request #1577 · JabRef/jabref

tschechlovdev · 2016-07-12T14:45:46Z

Remove the teamscale finding in the database package, that is in the model package. This means refactoring of methods, that are too long, adding comments to public method/classes and used streams in some situations for better readability (and reducing code).

koppor · 2016-07-12T15:38:40Z

    }

+    /**
+     * Creates a KeyCollisionException with a given message and a given cause.


I don't think, it necessary to document trivial cases. Maybe, this teamscale finding should be ignored?

Ok I removed the unnecessary comments.

koppor · 2016-07-13T07:56:19Z

    // note: there is a good reason why we should not use a hashset but use hashmap instead
    //========================================================
-    public void removeKeyFromSet(String key) {
+    protected void removeKeyFromSet(String key) {


Still not consistent with addKeyToSet

But then I have to set addKeyToSet protected, or public, although it can be declared private. Shouldn't it have the lowest possible visibility level?

koppor · 2016-07-13T08:48:59Z


    private final EventBus eventBus = new EventBus();

+


Bitte keine obsoelten Leerzeilen

koppor · 2016-07-18T01:06:54Z

+    /**
+     *
+     * @param key
+     * @return list of entries that contains the given key


Please rework comment. "Collects entries having the specified BibTeX key and returns these entries as list. The order of the entries is the order they appear in the database"

boceckts · 2016-07-21T13:47:59Z

+ *
+ * This class handles the instantiations of {@link BibDatabase}.
+ *
+ */


Please remove the empty lines in the comment

koppor · 2016-08-12T09:07:40Z

 import net.sf.jabref.model.entry.EntryType;

+/**
+ * In this class the detection of the {@link BibDatabaseMode} is handled.


Either remove this comment or expand it by explaining, what the intention of the mode is etc. What is the idea of the detection etc.

…chechlovdev/jabref into TeamscaleFindingsModelDatabasePackage # Conflicts: # src/main/java/net/sf/jabref/model/database/BibDatabase.java # src/main/java/net/sf/jabref/model/database/DuplicationChecker.java

…ModelDatabasePackage

oscargus · 2016-08-20T22:04:00Z

            entry.setCiteKey(key);
        }
-        return duplicationChecker.checkForDuplicateKeyAndAdd(oldKey, key);
+        return duplicationChecker.checkForDuplicateKeyAndAdd(entry.getCiteKeyOptional(), Optional.ofNullable(key));


entry.getCiteKeyOptional() will not contain the old key now. Both arguments will be identical so will break the function of the call.

Thanks a lot! I've searched at the wrong place for the failure ....

…ModelDatabasePackage # Conflicts: # src/main/java/net/sf/jabref/model/database/BibDatabase.java

stefan-kolb · 2016-08-23T09:15:02Z

+     *
+     * @return true, if there is a duplicate key, else false
+     */
+    public boolean checkForDuplicateKeyAndAdd(Optional<String> oldKey, Optional<String> newKey) {


I don't like Optionals as parameters here.

If I remember correctly, the caller may pass null for both oldKey and newKey. As JabRef moves away from null towards Optional, we thought, it would be a good idea to use Optional here, too.

Personnaly I think Optional is only a good thing for return types. I also posted an article some time ago.

Independent of Optional or not, wouldn't it be possible to write the code like:

boolean duplicate = false; if (!oldKey.equals(newKey)) { // If keys are not the same if (!oldKey.isPresent()) { // No old key duplicate = addKeyToSet(newKey.get()); } else { removeKeyFromSet(oldKey.get()); // Get rid of old key if (newKey.isPresent()) { // Add new key if any duplicate = addKeyToSet(newKey.get()); } } } if (duplicate) { ... }

(Replace with corresponding non-optional calls (Objects.equals() for the first call).

@stefan-kolb Do you have a deep link for the article? I checked https://github.com/JabRef/jabref/wiki/Code-Howtos, which points to https://github.com/cxxr/better-java#avoid-nulls, which uses Optional also in the parameters.

Not the exacr one but what about these: https://dzone.com/articles/use-of-optional-is-optional, https://dzone.com/articles/java-8-optional-how-use-it

Related discussion:
http://stackoverflow.com/questions/31922866/why-should-java-8s-optional-not-be-used-in-arguments

2016-08-26 8:25 GMT+02:00 Stefan Kolb notifications@github.com:

In src/main/java/net/sf/jabref/model/database/DuplicationChecker.java
#1577 (comment):

//############################################

// if the newkey already exists and is not the same as oldkey it will give a warning

// else it will add the newkey to the to set and remove the oldkey

public boolean checkForDuplicateKeyAndAdd(String oldKey, String newKey) {

// LOGGER.debug(" checkForDuplicateKeyAndAdd [oldKey = " + oldKey + "] [newKey = " + newKey + "]");

/**

\* Usage:

\* <br>

\* isDuplicate=checkForDuplicateKeyAndAdd( null, b.getKey() , issueDuplicateWarning);

*

\* If the newkey already exists and is not the same as oldkey it will give a warning

\* else it will add the newkey to the to set and remove the oldkey

*

\* @return true, if there is a duplicate key, else false

*/

public boolean checkForDuplicateKeyAndAdd(Optional oldKey, Optional newKey) {

Not the exacr one but what about these: https://dzone.com/articles/
use-of-optional-is-optional, https://dzone.com/articles/
java-8-optional-how-use-it

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
https://github.com/JabRef/jabref/pull/1577/files/f9d6a1943a7bc023d8042f02c3c3922c38f34a66#r76369435,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AATi5Fj7w1jmaRe-PdgqNDMDsD1MeDS2ks5qjobDgaJpZM4JKeSe
.

stefan-kolb · 2016-08-23T09:16:05Z

Just two small changes and I'm OK with it 👍

…ModelDatabasePackage

stefan-kolb · 2016-09-02T10:21:17Z

LGTM 👍

* master: Remove teamscale findings in the database package (JabRef#1577) French localization: Jabref_fr: empty strings translated + removal of unused header (JabRef#1911)

* Check integrity edition check * Conflict in Jabref_fr * Changed order Biblatex/Bibtex condition * Create own object * Rename variable * Extract checker to own file * Improved comment * French localization: Jabref_fr: empty strings translated + removal of unused header (#1911) * Remove teamscale findings in the database package (#1577) * Check integrity edition check * Changed order Biblatex/Bibtex condition * Create own object * Rename variable * Mark some methods as deprecated in BibEntry and BibDatabase (#1913) * Mark some methods as deprecated in BibEntry and BibDatabase * Rename getResolvedFieldOrAlias * Use flatmap * Fix location field not exported correctly to office 2007 xml (#1909) * Fix location field not exported to office 2007 xml * Add some test for exporting location and address field Address in xml field is now imported as location add some javadoc * Add possibility to remember password for shared databases. (#1846) * Add possibility to remember password. - Add checkbox to the open shared database dialog - Add SharedDatabasePreferences - Add password encrypting and decrypting methods - Update localization entries - Reorganize clearing methods for Preferences * Change prefs node and add class comment. * Relativate node path for password storage. * Fix LOGGER factory. * Improve password encryption using XOR. - Use username as one operand - Add new parameter to the constructor * Extract method. * Improve exception handling. * Improve password encryption and decryption. - Use CBC and padding - Hash the key before using - Simplify conversion * Fix modifier. Fix conflicts. * Extract checker to own file * Improved comment * Translation of shared (#1919) * Add missing entries in german localization * Resolved conflicts * Some more conflicts de sv * Expressions changed

tschechlovdev added stupro and removed stupro-ready-for-internal-review labels Jul 12, 2016

koppor reviewed Jul 12, 2016
View reviewed changes

tschechlovdev force-pushed the TeamscaleFindingsModelDatabasePackage branch 2 times, most recently from 982c239 to 72fae1a Compare July 12, 2016 16:19

koppor reviewed Jul 13, 2016
View reviewed changes

koppor added the status: devcall label Jul 13, 2016

koppor reviewed Jul 13, 2016
View reviewed changes

koppor removed the status: devcall label Jul 13, 2016

koppor changed the title ~~[WIP]remove teamscale findings in the database package~~ [WIP] Remove teamscale findings in the database package Jul 18, 2016

koppor reviewed Jul 18, 2016
View reviewed changes

tschechlovdev force-pushed the TeamscaleFindingsModelDatabasePackage branch from 6d9ac2e to a287147 Compare July 18, 2016 16:02

tschechlovdev added the stupro-ready-for-internal-review label Jul 21, 2016

boceckts reviewed Jul 21, 2016
View reviewed changes

koppor reviewed Aug 12, 2016
View reviewed changes

koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 12, 2016

tschechlovdev added 5 commits August 12, 2016 17:07

Adressed comments and introduced Optional at checkForDuplicateKeyAndAdd

7817f08

Adressed comments and introduced Optional at checkForDuplicateKeyAndAdd

378b1a1

Merge branch 'TeamscaleFindingsModelDatabasePackage' of github.com:ts…

c709b87

…chechlovdev/jabref into TeamscaleFindingsModelDatabasePackage # Conflicts: # src/main/java/net/sf/jabref/model/database/BibDatabase.java # src/main/java/net/sf/jabref/model/database/DuplicationChecker.java

Merge remote-tracking branch 'upstream/master' into TeamscaleFindings…

3b54cba

…ModelDatabasePackage

fix failing tests because of NPE

96aa7f5

oscargus reviewed Aug 20, 2016
View reviewed changes

tschechlovdev added 2 commits August 21, 2016 09:44

add oldKey

8eb9597

Merge remote-tracking branch 'upstream/master' into TeamscaleFindings…

f9d6a19

…ModelDatabasePackage # Conflicts: # src/main/java/net/sf/jabref/model/database/BibDatabase.java

tschechlovdev added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 21, 2016

stefan-kolb reviewed Aug 23, 2016
View reviewed changes

koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 25, 2016

tschechlovdev added 3 commits September 2, 2016 11:34

remove optionals as parameters and remove empty line

5587fa1

Merge remote-tracking branch 'upstream/master' into TeamscaleFindings…

ad65a76

…ModelDatabasePackage

remove empty line

bb67718

stefan-kolb added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 2, 2016

stefan-kolb merged commit 357be5c into JabRef:master Sep 3, 2016

tschechlovdev deleted the TeamscaleFindingsModelDatabasePackage branch September 3, 2016 13:35

tschechlovdev mentioned this pull request Oct 10, 2016

Fix DuplicationChecker and key generator #2151

Merged

Uh oh!

Conversation

tschechlovdev commented Jul 12, 2016

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!

stefan-kolb commented Aug 23, 2016

Uh oh!

stefan-kolb commented Sep 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants