-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Added the feature of IcoRanking #13512
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
base: main
Are you sure you want to change the base?
Conversation
@@ -11,6 +11,7 @@ | |||
requires java.sql.rowset; | |||
|
|||
// region JavaFX | |||
requires javafx.swing; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The module requires both Swing and JavaFX dependencies, which violates the architectural decision to use only JavaFX for UI components. Swing dependencies should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the dep here
|
||
@Test | ||
void testEditorCreationDoesNotCrash() { | ||
assertNotNull(FieldEditors.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to modern testing practices, should assert specific contents/behavior rather than just checking for non-null conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this test - it is always true
public final String title; | ||
public final String acronym; | ||
public final String source; | ||
public final String rank; | ||
public final String note; | ||
public final String dblp; | ||
public final String primaryFor; | ||
public final String comments; | ||
public final String averageRating; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Class fields are declared as public, violating encapsulation principles. They should be private with getter methods to maintain proper encapsulation and control over data access.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Convert the class to record
assertTrue(repo.getRankingFor("NON_EXISTENT").isEmpty()); | ||
} | ||
@Test | ||
public void test1(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test method name is not descriptive enough. It should clearly indicate what is being tested, such as 'testFuzzyMatchForInternationalConferenceOnSoftwareEngineering'.
assertEquals(Optional.empty(), ConferenceUtil.extractAcronym(title)); | ||
} | ||
@Test | ||
void test1(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test method name is not descriptive enough and doesn't indicate what is being tested. Should describe the test scenario like 'testExtractAcronymFromConferenceWithParentheses'.
assertEquals(Optional.of("ICSE"),ConferenceUtil.extractAcronym(title)); | ||
} | ||
@Test | ||
void test2(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test method name is not descriptive enough and doesn't indicate what is being tested. Should describe the test scenario like 'testExtractAcronymFromConferenceWithoutParentheses'.
|
||
@Test | ||
void testEditorCreationDoesNotCrash() { | ||
assertNotNull(FieldEditors.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this test - it is always true
public void bindToEntry(BibEntry entry) { | ||
this.currentEntry = entry; | ||
|
||
// System.out.println("ENTRY booktitle = " + entry.getField(StandardField.BOOKTITLE).orElse("none")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use LOGGER.debug - see https://devdocs.jabref.org/code-howtos/logging.html
Optional<String> acronym = ConferenceUtil.extractAcronym(rawInput); // Extracting the acronym from our input field | ||
Optional<ConferenceRankingEntry> result = acronym.flatMap(repo::getFullEntry) | ||
.or(() -> repo.getFullEntry(rawInput)); // Finding if any matching entry present in csv file | ||
// If present then print the info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reformat the comments -- starting at column 1 is not ok
this.setSpacing(10); | ||
} | ||
|
||
// Deprecated method it only shows rank by puttin the acronym in the text field |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what this code does.
I think, this should be removed -- it is part of some logic in all cases.
Primary FoR: %s | ||
Comments: %s | ||
Average Rating: %s | ||
""", title, acronym, source, rank, note, dblp, primaryFor, comments, averageRating); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is OK to have an additional way of showing the contents...
However, in future, use IntelliJ's feature to create a toString
method
|
||
try (BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream, StandardCharsets.UTF_8))) { | ||
reader.lines().skip(1).forEach(line -> { | ||
String[] parts = line.split(",(?=(?:[^\"]*\"[^\"]*\")*[^\"]*$)", -1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No.
There are libraries for this. In JabRef, we use https://commons.apache.org/proper/commons-csv/
LOGGER.info("Fuzzy match fallback triggered for: " + key); | ||
return nameToRank.entrySet().stream() | ||
.filter(e -> similarity.editDistanceIgnoreCase(e.getKey(), key) < 0.01) | ||
.peek(e -> LOGGER.info("Fuzzy match candidate: " + e.getKey())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong logger format - use {}
as place holder. See other code in JabRef.
(also applies for all other logger statements)
public final String title; | ||
public final String acronym; | ||
public final String source; | ||
public final String rank; | ||
public final String note; | ||
public final String dblp; | ||
public final String primaryFor; | ||
public final String comments; | ||
public final String averageRating; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Convert the class to record
Maybe, you did not check out JabRef correctly. Execute |
@asifebrahim You screenshort shows |
You also need to do a preference migration -
Also add the field directly after |
Optional<String> finalBookTitle = bookTitle; | ||
String rawInput = icoreField.orElseGet(() -> finalBookTitle.orElse("Unknown")); | ||
|
||
Optional<String> acronym = ConferenceUtil.extractAcronym(rawInput); // Extracting the acronym from our input field |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment is trivial and can be derived from the code itself. The comment doesn't add any new information about why the acronym extraction is needed.
public ICoreRankingEditor(Field field) { | ||
this.field = field; | ||
this.textField = new TextField(); | ||
this.repo = new ICoreRankingRepository(); // Load once |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment 'Load once' is trivial and doesn't provide additional information. The single initialization in the constructor already implies this.
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
public class PreferencesMigrations { | ||
|
||
private static final Logger LOGGER = LoggerFactory.getLogger(PreferencesMigrations.class); | ||
private JabRefGuiPreferences preferences; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Field should be final to prevent modification after initialization, following immutability best practices and thread-safety principles of modern Java.
import java.util.List; | ||
import java.util.Map; | ||
import java.util.Optional; | ||
import java.util.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using wildcard imports instead of specific imports reduces code readability and can lead to naming conflicts. Each import should be explicitly declared.
String note, | ||
String dblp, | ||
String primaryFor, | ||
String averageRating |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using String for numerical data prevents proper validation and comparison. Should use Double or BigDecimal to ensure proper numerical handling and prevent invalid data entry.
final Map<String, String> acronymToRank = new HashMap<>(); | ||
private Map<String, String> nameToRank = new HashMap<>(); | ||
private Map<String, ConferenceRankingEntry> acronymMap = new HashMap<>(); | ||
private Map<String, ConferenceRankingEntry> nameMap = new HashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use modern Java data structures. Maps should be initialized using Map.of() for empty maps instead of new HashMap<>(). This improves code readability and follows modern Java practices.
private Map<String, ConferenceRankingEntry> acronymMap = new HashMap<>(); | ||
private Map<String, ConferenceRankingEntry> nameMap = new HashMap<>(); | ||
private StringSimilarity similarity = new StringSimilarity(); | ||
private Logger LOGGER = LoggerFactory.getLogger(ICoreRankingRepository.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logger should be declared as private static final following Java naming conventions. Constants should be in uppercase, and loggers are typically static final members.
// System.out.println("Loaded entries:"); | ||
// acronymToRank.forEach((key, val) -> System.out.println("Acronym: " + key + " -> " + val)); | ||
// nameToRank.forEach((key, val) -> System.out.println("Name: " + key + " -> " + val)); | ||
} catch (NullPointerException | IOException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NullPointerException should not be caught as it indicates a programming error. Additionally, try block covers too much code. It should be split into smaller, more focused blocks.
import java.util.regex.Pattern; | ||
|
||
public class ConferenceUtil { | ||
public static Optional<String> extractAcronym(String title) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method parameter 'title' lacks null check, which could lead to NullPointerException when calling matcher. Method should validate input parameters to ensure robustness.
import java.util.regex.Matcher; | ||
import java.util.regex.Pattern; | ||
|
||
public class ConferenceUtil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Utility class should have a private constructor to prevent instantiation, following effective Java principles for utility classes containing only static methods.
@@ -231,7 +231,7 @@ private static Set<Field> getAllFields() { | |||
* separate preferences object | |||
*/ | |||
public static List<Field> getDefaultGeneralFields() { | |||
List<Field> defaultGeneralFields = new ArrayList<>(Arrays.asList(StandardField.DOI, StandardField.CROSSREF, StandardField.KEYWORDS, StandardField.EPRINT, StandardField.URL, StandardField.FILE, StandardField.GROUPS, StandardField.OWNER, StandardField.TIMESTAMP)); | |||
List<Field> defaultGeneralFields = new ArrayList<>(Arrays.asList(StandardField.DOI, StandardField.ICORERANKING, StandardField.CROSSREF, StandardField.KEYWORDS, StandardField.EPRINT, StandardField.URL, StandardField.FILE, StandardField.GROUPS, StandardField.OWNER, StandardField.TIMESTAMP)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using ArrayList with Arrays.asList is an outdated pattern. Modern Java provides more concise and readable alternatives like List.of() for creating immutable lists.
MODIFICATIONDATE("modificationdate", FieldProperty.DATE), | ||
ICORERANKING("icoreranking"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The added field introduces unnecessary blank line before it, violating the consistent formatting pattern used in the enum declaration.
@asifebrahim Before requesting a review, please fix the tests. Although it seems, I have endless time, in reality, I don't. I like automation and that machines can support us all in making good code. Thus, please, look at the failing tests. |
|
||
public class ConferenceUtil { | ||
public static Optional<String> extractAcronym(String title) { | ||
Matcher matcher = Pattern.compile("\\((.*?)\\)").matcher(title); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pattern
needs to be a private static final variable to be really speed saving
You committed your code on the For this pull request, this is OK. For subsequent pull requests, please start with a different branch with a proper branch name. See CONTRIBUTING.md for more details. |
Note that your PR will not be reviewed/accepted until you have gone through the mandatory checks in the description and marked each of them them exactly in the format of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please adress my review comments - and also the commetns by trag-bot.
Expecially:
There are libraries for this. In JabRef, we use commons.apache.org/proper/commons-csv
@trag-bot didn't find any issues in the code! ✅✨ |
JUnit tests of You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide. |
Closes #13476
This is the PR request for issue no #13476
Steps to test
In the General field at the very bottom there sthe icoranking lookup fiield there one can search using a acronym and get the details about the conference.


NOTE
-The Abbreviation tests were failing before i added any code so there were a total of 76 failed test case sand after my addition it is still 76 and all of them are NullPointerException and I didn't fix that
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if change is visible to the user)