Collection util rework #53

FelixBaensch · 2024-03-06T15:27:43Z

No description provided.

JonasSchaub · 2024-03-06T17:20:16Z

How about defining ascending/descending via a bool? This way, no JavaFx class would be used in this model class. Or can there be other sort orders, hypothetically?

JonasSchaub · 2024-03-06T17:29:31Z

The enum values (or their text representations) should then also be used in the GUI classes that define the cell value properties, right? Are these displayed and hence language-specific?

FelixBaensch · 2024-03-07T07:51:04Z

The enum values (or their text representations) should then also be used in the GUI classes that define the cell value properties, right?

Done

Are these displayed and hence language-specific?

No, they are not

JonasSchaub · 2024-03-07T08:56:30Z

Thank you, looks much better now!
I'm just still hung up on this:

How about defining ascending/descending via a bool? This way, no JavaFx class would be used in this model class. Or can there be other sort orders, hypothetically?

And the TableColumn.SortType enum appears to only have the two values ascending and descending anyway.

FelixBaensch · 2024-03-07T09:09:53Z

Thank you, looks much better now! I'm just still hung up on this:

How about defining ascending/descending via a bool? This way, no JavaFx class would be used in this model class. Or can there be other sort orders, hypothetically?

And the TableColumn.SortType enum appears to only have the two values ascending and descending anyway.

Unfortunately I overlooked that.
That would require a method to parse the TableColumn.SortType into a Boolean. I would rather argue in favor of moving the CollectionUtils to a controller package if you want to avoid the fx class in the model area.

JonasSchaub · 2024-03-07T09:17:26Z

That would require a method to parse the TableColumn.SortType into a Boolean.

It wouldn't be that complicated, would it? I'd name the boolean parameter "ascending" and the method call would be like "tmpSortType.equals(ASCENDING)". If false, the used order is descending. Am I missing something?

FelixBaensch · 2024-03-07T09:33:54Z

If it makes you happy

JonasSchaub · 2024-03-07T13:13:21Z

If it makes you happy

It does 🤗

JonasSchaub · 2024-03-07T13:21:09Z

src/main/java/de/unijena/cheminf/mortar/model/util/CollectionUtil.java

@@ -48,78 +48,69 @@ private CollectionUtil() {
    //</editor-fold>
    //
    //<editor-fold desc="public static methods" defaultstate="collapsed">
-    //TODO: check parameters, change params to enum values (and bools?), throw exceptions!
    /**
     * Sorts given list by property and sort type.
     *
     * @param aList List
     * @param aProperty String


more detailed doc needed

JonasSchaub · 2024-03-07T13:27:36Z

src/main/java/de/unijena/cheminf/mortar/model/util/CollectionUtil.java

     */
-    public static void sortGivenFragmentListByPropertyAndSortType(List<? extends MoleculeDataModel> aList, String aProperty, String aSortType) {
-        Collections.sort(aList, (m1, m2) -> {
+    public static void sortGivenFragmentListByPropertyAndSortType(List<? extends MoleculeDataModel> aList, String aProperty, boolean ascending) {


I have a problem with the parameter type. It must be objects extending MoleculeDataModel but except for the name and the unique SMILES, most of the sorting properties refer to FragmentDataModel. The given objects in the list are just implicitly cast to FragmentDataModel without any checks. Should we throw exceptions here if e.g. someone tries to sort MoleculeDataModels (or any other future class extending it) by some frequency, for example?

It could be surrounded by a try-catch-block to catch the ClassCastExcpetion but what should happen then?
The class could be checked before parsing and if it does not match 0 can be returned to not sort the list.
What do you expect here?

Returning 0 in that case would be fine, I guess. Consistent with the issue below.

Or throw IllegalArgumentException since you also do that below if the property cannot be parsed into an enum constant?

Then I will throw an IllegalArgumentExcpetion because returning 0 could be confusing.

Alright, thank you 👍

JonasSchaub · 2024-03-07T13:28:49Z

src/main/java/de/unijena/cheminf/mortar/model/util/CollectionUtil.java

+                    if (ascending)
+                        return f1.getParentMoleculeName().compareTo(f2.getParentMoleculeName());
+                    else
+                        return f2.getParentMoleculeName().compareTo(f1.getParentMoleculeName());


add default case or throw exception if the given sort type is not known? I can see that you return 0 in this case currently but does it make sense?

If Property or SortType (which should no longer be possible) are not known, 0 is returned to the sort method, which means that no sorting should be performed. Therefore this is fine.

I didn't know about this convention of returning 0 when sorting is not possible. Ok then, leave it this way, but please define it in the doc.

JonasSchaub · 2024-03-07T14:15:11Z

src/main/java/de/unijena/cheminf/mortar/model/util/CollectionUtil.java

-     * @param aProperty String
-     * @param aSortType String
+     * @param aList List of Molecule-/FragmentDataModel objects to be sorted.
+     * @param aProperty String property of Molecule-/FragmentDataModel by which to sort the list.


Define that the given string must correspond to the text property of a DataModelPropertiesForTableView enum constant.

It does not have to, only if, the list is sorted

You are throwing an IllegalArgumentException if it does not, or am I getting this wrong?

You are right, but this is described in the @throw tag

JonasSchaub · 2024-03-08T09:31:55Z

src/main/java/de/unijena/cheminf/mortar/model/util/CollectionUtil.java

+     * @param aList List of Molecule-/FragmentDataModel objects to be sorted.
+     * @param aProperty String property of Molecule-/FragmentDataModel by which to sort the list. Property must be part
+     *                  of {@link SimpleEnumConstantNameProperty}. If the property is part of
+     *                  {@link SimpleEnumConstantNameProperty} but not handled in this method the list will not be sorted.


I think you mean DataModelPropertiesForTableView, not SimpleEnumConstantNameProperty, right?

definitely, meetig-work :)

sonarqubecloud · 2024-03-08T10:39:46Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

JonasSchaub · 2024-03-08T13:03:19Z

Do you want to merge this or should I?

FelixBaensch added 3 commits March 6, 2024 15:03

Change sort type to enum of TableColumn

15080e8

Fix javadoc

27f008a

Add enum for data model properties

1d36dfc

FelixBaensch requested a review from JonasSchaub March 6, 2024 15:27

FelixBaensch mentioned this pull request Mar 6, 2024

Rework method and usage of CollectionUtil.sortGivenFragmentListByPropertyAndSortType() #50

Closed

Add javadoc

772dc32

Add enums to TableViews

115f1c6

Change SortType to boolean

1ef68f5

FelixBaensch added 2 commits March 7, 2024 10:36

Optimize imports

caac884

Fix Spotless requirements

edeb6c6

JonasSchaub requested changes Mar 7, 2024

View reviewed changes

Add javadoc

7db73c1

JonasSchaub reviewed Mar 7, 2024

View reviewed changes

FelixBaensch added 2 commits March 7, 2024 16:40

Refine javadoc

a064d1c

Add try-catch-block to sorting method

ab638ef

JonasSchaub reviewed Mar 8, 2024

View reviewed changes

Fix javadoc

d60c21d

JonasSchaub approved these changes Mar 8, 2024

View reviewed changes

FelixBaensch merged commit 8180e29 into production Mar 8, 2024
2 checks passed

FelixBaensch deleted the CollectionUtil_rework branch March 8, 2024 18:31

FelixBaensch restored the CollectionUtil_rework branch April 2, 2024 09:39

FelixBaensch deleted the CollectionUtil_rework branch April 2, 2024 10:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collection util rework #53

Collection util rework #53

FelixBaensch commented Mar 6, 2024

JonasSchaub commented Mar 6, 2024

JonasSchaub commented Mar 6, 2024

FelixBaensch commented Mar 7, 2024 •

edited

Loading

JonasSchaub commented Mar 7, 2024

FelixBaensch commented Mar 7, 2024 •

edited

Loading

JonasSchaub commented Mar 7, 2024

FelixBaensch commented Mar 7, 2024

JonasSchaub commented Mar 7, 2024

JonasSchaub Mar 7, 2024

JonasSchaub Mar 7, 2024

FelixBaensch Mar 7, 2024

JonasSchaub Mar 7, 2024

JonasSchaub Mar 7, 2024

FelixBaensch Mar 7, 2024

JonasSchaub Mar 7, 2024

JonasSchaub Mar 7, 2024

FelixBaensch Mar 7, 2024 •

edited

Loading

JonasSchaub Mar 7, 2024

JonasSchaub Mar 7, 2024

FelixBaensch Mar 7, 2024

JonasSchaub Mar 7, 2024

FelixBaensch Mar 7, 2024

JonasSchaub Mar 8, 2024

FelixBaensch Mar 8, 2024

sonarqubecloud bot commented Mar 8, 2024

JonasSchaub commented Mar 8, 2024

Collection util rework #53

Collection util rework #53

Conversation

FelixBaensch commented Mar 6, 2024

JonasSchaub commented Mar 6, 2024

JonasSchaub commented Mar 6, 2024

FelixBaensch commented Mar 7, 2024 • edited Loading

JonasSchaub commented Mar 7, 2024

FelixBaensch commented Mar 7, 2024 • edited Loading

JonasSchaub commented Mar 7, 2024

FelixBaensch commented Mar 7, 2024

JonasSchaub commented Mar 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FelixBaensch Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Mar 8, 2024

Quality Gate passed

JonasSchaub commented Mar 8, 2024

FelixBaensch commented Mar 7, 2024 •

edited

Loading

FelixBaensch commented Mar 7, 2024 •

edited

Loading

FelixBaensch Mar 7, 2024 •

edited

Loading