Repository: obsidian-tasks-group/obsidian-tasks
Pull request #1398 (2022-12-15 11:18:24)

refactor: Convert Sorting from alias to class, to store comparator
<!--- Provide a general summary of your changes in the Title above -->

# Description

A series of safe refactorings to convert this:

```ts
// In Query.ts
type Sorting = {
    property: SortingProperty;
    reverse: boolean;
    propertyInstance: number;
};
```

in to this:

```ts
// In Sort.ts
export class Sorting {
    public readonly property: SortingProperty;
    public readonly comparator: Comparator;
    public readonly reverse: boolean;
    public readonly propertyInstance: number;

    constructor(property: SortingProperty, reverse: boolean, propertyInstance: number) {
        this.property = property;
        this.reverse = reverse;
        this.propertyInstance = propertyInstance;
        this.comparator = this.makeComparator();
    }

    public makeComparator() {
        const comparator = Sort.comparators[this.property];
        return this.reverse ? Sort.makeReversedComparator(comparator) : comparator;
    }
}
```

A side-effect is that test code that creating `Sorting`s is a lot clearer, as field names no longer need to be specified everywhere.

## Motivation and Context

This is a step towards moving all the comparator functions to their respective `Field` classes.

## How has this been tested?

- Running existing tests.

## Screenshots (if appropriate)

## Types of changes

<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->

Internal changes:

- [x] **Refactor** (prefix: `refactor` - non-breaking change which only improves the design or structure of existing code, and making no changes to its external behaviour)

## Checklist

<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->

- [x] My code follows the code style of this project and passes `yarn run lint`.
- [ ] My change requires a change to the documentation.
- [ ] I have [updated the documentation](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CONTRIBUTING.md#updating-documentation) accordingly.
- [x] My change has adequate [Unit Test coverage](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CONTRIBUTING.md#maintaining-the-tests).

## Terms

<!--
By submitting this pull request, you must agree to follow our
[contributing guide](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CONTRIBUTING.md) and
[Code of Conduct](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CODE_OF_CONDUCT.md).
Put an x in the boxes to confirm you agree.
-->

- [x] My contribution follow this project's [contributing guide](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CONTRIBUTING.md)
- [x] I agree to follow this project's [Code of Conduct](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CODE_OF_CONDUCT.md)



Patch:
diff --git a/src/Query/Query.ts b/src/Query/Query.ts
index 07896f46..d8676c5f 100644
--- a/src/Query/Query.ts
+++ b/src/Query/Query.ts
@@ -2,7 +2,7 @@ import { LayoutOptions } from '../LayoutOptions';
 import type { Task } from '../Task';
 import type { IQuery } from '../IQuery';
 import { getSettings } from '../Config/Settings';
-import { Sort } from './Sort';
+import { Sort, Sorting } from './Sort';
 import type { TaskGroups } from './TaskGroups';
 import { parseFilter } from './FilterParser';
 import { Group } from './Group';
@@ -19,11 +19,6 @@ export type SortingProperty =
     | 'path'
     | 'description'
     | 'tag';
-type Sorting = {
-    property: SortingProperty;
-    reverse: boolean;
-    propertyInstance: number;
-};
 
 export type GroupingProperty =
     | 'backlink'
@@ -241,11 +236,13 @@ export class Query implements IQuery {
     private parseSortBy({ line }: { line: string }): void {
         const fieldMatch = line.match(this.sortByRegexp);
         if (fieldMatch !== null) {
-            this._sorting.push({
-                property: fieldMatch[1] as SortingProperty,
-                reverse: !!fieldMatch[2],
-                propertyInstance: isNaN(+fieldMatch[3]) ? 1 : +fieldMatch[3],
-            });
+            this._sorting.push(
+                new Sorting(
+                    fieldMatch[1] as SortingProperty,
+                    !!fieldMatch[2],
+                    isNaN(+fieldMatch[3]) ? 1 : +fieldMatch[3],
+                ),
+            );
         } else {
             this._error = 'do not understand query sorting';
         }
diff --git a/src/Query/Sort.ts b/src/Query/Sort.ts
index 638e446e..56dfe0d7 100644
--- a/src/Query/Sort.ts
+++ b/src/Query/Sort.ts
@@ -5,6 +5,25 @@ import type { Query, SortingProperty } from './Query';
 
 type Comparator = (a: Task, b: Task) => number;
 
+export class Sorting {
+    public readonly property: SortingProperty;
+    public readonly comparator: Comparator;
+    public readonly reverse: boolean;
+    public readonly propertyInstance: number;
+
+    constructor(property: SortingProperty, reverse: boolean, propertyInstance: number) {
+        this.property = property;
+        this.reverse = reverse;
+        this.propertyInstance = propertyInstance;
+        this.comparator = this.makeComparator();
+    }
+
+    public makeComparator() {
+        const comparator = Sort.comparators[this.property];
+        return this.reverse ? Sort.makeReversedComparator(comparator) : comparator;
+    }
+}
+
 export class Sort {
     static tagPropertyInstance: number = 1;
 
@@ -19,18 +38,17 @@ export class Sort {
 
         const userComparators: Comparator[] = [];
 
-        for (const { property, reverse, propertyInstance } of query.sorting) {
-            const comparator = Sort.comparators[property];
-            userComparators.push(reverse ? Sort.makeReversedComparator(comparator) : comparator);
-            if (property === 'tag') {
-                Sort.tagPropertyInstance = propertyInstance;
+        for (const sorting of query.sorting) {
+            userComparators.push(sorting.comparator);
+            if (sorting.property === 'tag') {
+                Sort.tagPropertyInstance = sorting.propertyInstance;
             }
         }
 
         return tasks.sort(Sort.makeCompositeComparator([...userComparators, ...defaultComparators]));
     }
 
-    private static comparators: Record<SortingProperty, Comparator> = {
+    public static comparators: Record<SortingProperty, Comparator> = {
         urgency: Sort.compareByUrgency,
         description: Sort.compareByDescription,
         priority: Sort.compareByPriority,
@@ -43,7 +61,7 @@ export class Sort {
         tag: Sort.compareByTag,
     };
 
-    private static makeReversedComparator(comparator: Comparator): Comparator {
+    public static makeReversedComparator(comparator: Comparator): Comparator {
         return (a, b) => (comparator(a, b) * -1) as -1 | 0 | 1;
     }
 


Test patch:
diff --git a/tests/Query.test.ts b/tests/Query.test.ts
index d8aa880d..87457f61 100644
--- a/tests/Query.test.ts
+++ b/tests/Query.test.ts
@@ -5,6 +5,7 @@ import moment from 'moment';
 import { Query } from '../src/Query/Query';
 import { Priority, Status, Task } from '../src/Task';
 import { resetSettings, updateSettings } from '../src/Config/Settings';
+import { Sorting } from '../src/Query/Sort';
 import { createTasksFromMarkdown, fromLine } from './TestHelpers';
 import { shouldSupportFiltering } from './TestingTools/FilterTestHelpers';
 import type { FilteringCase } from './TestingTools/FilterTestHelpers';
@@ -710,42 +711,22 @@ describe('Query', () => {
     });
 
     describe('sorting instructions', () => {
-        const cases: {
-            input: string;
-            output: {
-                property: string;
-                reverse: boolean;
-                propertyInstance: number;
-            }[];
-        }[] = [
+        const cases: { input: string; output: Sorting[] }[] = [
             {
                 input: 'sort by status',
-                output: [
-                    {
-                        property: 'status',
-                        reverse: false,
-                        propertyInstance: 1,
-                    },
-                ],
+                output: [new Sorting('status', false, 1)],
             },
             {
                 input: 'sort by status\nsort by due',
-                output: [
-                    {
-                        property: 'status',
-                        reverse: false,
-                        propertyInstance: 1,
-                    },
-                    { property: 'due', reverse: false, propertyInstance: 1 },
-                ],
+                output: [new Sorting('status', false, 1), new Sorting('due', false, 1)],
             },
             {
                 input: 'sort by tag',
-                output: [{ property: 'tag', reverse: false, propertyInstance: 1 }],
+                output: [new Sorting('tag', false, 1)],
             },
             {
                 input: 'sort by tag 2',
-                output: [{ property: 'tag', reverse: false, propertyInstance: 2 }],
+                output: [new Sorting('tag', false, 2)],
             },
         ];
         it.concurrent.each(cases)('sorting as %j', ({ input, output }) => {
diff --git a/tests/Sort.test.ts b/tests/Sort.test.ts
index 06553743..ac5b1d8f 100644
--- a/tests/Sort.test.ts
+++ b/tests/Sort.test.ts
@@ -5,7 +5,7 @@ import moment from 'moment';
 
 window.moment = moment;
 
-import { Sort } from '../src/Query/Sort';
+import { Sort, Sorting } from '../src/Query/Sort';
 import { resetSettings, updateSettings } from '../src/Config/Settings';
 import { DateParser } from '../src/Query/DateParser';
 import { fromLine } from './TestHelpers';
@@ -38,13 +38,7 @@ describe('Sort', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [
-                        {
-                            property: 'due',
-                            reverse: false,
-                            propertyInstance: 1,
-                        },
-                    ],
+                    sorting: [new Sorting('due', false, 1)],
                 },
                 [one, two, three],
             ),
@@ -52,13 +46,7 @@ describe('Sort', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [
-                        {
-                            property: 'due',
-                            reverse: false,
-                            propertyInstance: 1,
-                        },
-                    ],
+                    sorting: [new Sorting('due', false, 1)],
                 },
                 [two, three, one],
             ),
@@ -81,13 +69,7 @@ describe('Sort', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [
-                        {
-                            property: 'done',
-                            reverse: false,
-                            propertyInstance: 1,
-                        },
-                    ],
+                    sorting: [new Sorting('done', false, 1)],
                 },
                 [three, two, one],
             ),
@@ -95,13 +77,7 @@ describe('Sort', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [
-                        {
-                            property: 'done',
-                            reverse: false,
-                            propertyInstance: 1,
-                        },
-                    ],
+                    sorting: [new Sorting('done', false, 1)],
                 },
                 [two, one, three],
             ),
@@ -123,21 +99,9 @@ describe('Sort', () => {
             Sort.by(
                 {
                     sorting: [
-                        {
-                            property: 'due',
-                            reverse: false,
-                            propertyInstance: 1,
-                        },
-                        {
-                            property: 'path',
-                            reverse: false,
-                            propertyInstance: 1,
-                        },
-                        {
-                            property: 'status',
-                            reverse: false,
-                            propertyInstance: 1,
-                        },
+                        new Sorting('due', false, 1),
+                        new Sorting('path', false, 1),
+                        new Sorting('status', false, 1),
                     ],
                 },
                 [one, four, two, three],
@@ -166,18 +130,7 @@ describe('Sort', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [
-                        {
-                            property: 'description',
-                            reverse: false,
-                            propertyInstance: 1,
-                        },
-                        {
-                            property: 'done',
-                            reverse: false,
-                            propertyInstance: 1,
-                        },
-                    ],
+                    sorting: [new Sorting('description', false, 1), new Sorting('done', false, 1)],
                 },
                 [three, one, two, four],
             ),
@@ -205,18 +158,7 @@ describe('Sort', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [
-                        {
-                            property: 'description',
-                            reverse: true,
-                            propertyInstance: 1,
-                        },
-                        {
-                            property: 'done',
-                            reverse: false,
-                            propertyInstance: 1,
-                        },
-                    ],
+                    sorting: [new Sorting('description', true, 1), new Sorting('done', false, 1)],
                 },
                 [two, four, three, one],
             ),
@@ -237,17 +179,9 @@ describe('Sort', () => {
             Sort.by(
                 {
                     sorting: [
-                        {
-                            property: 'status',
-                            reverse: true,
-                            propertyInstance: 1,
-                        },
-                        { property: 'due', reverse: true, propertyInstance: 1 },
-                        {
-                            property: 'path',
-                            reverse: false,
-                            propertyInstance: 1,
-                        },
+                        new Sorting('status', true, 1),
+                        new Sorting('due', true, 1),
+                        new Sorting('path', false, 1),
                     ],
                 },
                 [six, five, one, four, three, two],
@@ -276,13 +210,7 @@ describe('Sort', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [
-                        {
-                            property: 'description',
-                            reverse: false,
-                            propertyInstance: 1,
-                        },
-                    ],
+                    sorting: [new Sorting('description', false, 1)],
                 },
                 [two, one, five, four, three],
             ),
@@ -378,13 +306,7 @@ describe('Sort by tags', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [
-                        {
-                            property: 'tag',
-                            reverse: false,
-                            propertyInstance: 1,
-                        },
-                    ],
+                    sorting: [new Sorting('tag', false, 1)],
                 },
                 [t1, t3, t5, t7, t6, t4, t2, t8, t9, t10],
             ),
@@ -409,13 +331,7 @@ describe('Sort by tags', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [
-                        {
-                            property: 'tag',
-                            reverse: true,
-                            propertyInstance: 1,
-                        },
-                    ],
+                    sorting: [new Sorting('tag', true, 1)],
                 },
                 [t1, t3, t5, t7, t6, t4, t2, t8, t9, t10],
             ),
@@ -432,13 +348,7 @@ describe('Sort by tags', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [
-                        {
-                            property: 'tag',
-                            reverse: false,
-                            propertyInstance: 2,
-                        },
-                    ],
+                    sorting: [new Sorting('tag', false, 2)],
                 },
                 [t4, t3, t2, t1, t5],
             ),
@@ -455,13 +365,7 @@ describe('Sort by tags', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [
-                        {
-                            property: 'tag',
-                            reverse: true,
-                            propertyInstance: 2,
-                        },
-                    ],
+                    sorting: [new Sorting('tag', true, 2)],
                 },
                 [t4, t3, t2, t1, t5],
             ),
@@ -492,13 +396,7 @@ describe('Sort by tags', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [
-                        {
-                            property: 'tag',
-                            reverse: false,
-                            propertyInstance: 1,
-                        },
-                    ],
+                    sorting: [new Sorting('tag', false, 1)],
                 },
                 [t1, t12, t3, t13, t5, t7, t6, t4, t2, t8, t9, t10, t11],
             ),
@@ -532,13 +430,7 @@ describe('Sort by tags', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [
-                        {
-                            property: 'tag',
-                            reverse: true,
-                            propertyInstance: 1,
-                        },
-                    ],
+                    sorting: [new Sorting('tag', true, 1)],
                 },
                 [t1, t12, t3, t13, t5, t7, t6, t4, t2, t8, t9, t10, t11],
             ),
@@ -565,13 +457,7 @@ describe('Sort by tags', () => {
         // Act
         const result = Sort.by(
             {
-                sorting: [
-                    {
-                        property: 'tag',
-                        reverse: false,
-                        propertyInstance: 2,
-                    },
-                ],
+                sorting: [new Sorting('tag', false, 2)],
             },
             [t4, t7, t5, t2, t3, t1, t8, t6],
         );
@@ -600,13 +486,7 @@ describe('Sort by tags', () => {
         // Act
         const result = Sort.by(
             {
-                sorting: [
-                    {
-                        property: 'tag',
-                        reverse: true,
-                        propertyInstance: 2,
-                    },
-                ],
+                sorting: [new Sorting('tag', true, 2)],
             },
             [t4, t7, t5, t2, t3, t1, t8, t6],
         );