遷移 Course Connector 至 v3 與重構 Course Connector 實作細節

[ntut-xuan]

Description

Warning：這個 PR 是重構相關 PR， diff 過大非常抱歉，僅需確認功能性正常。

在這個 PR 上，我遷移了 CourseExtraJson 與其相關無用的 Json 架構，使 Course 與 Syllabus 來涵蓋大部分的無用 Json，並且遷移了 Course Connector 至 v3

Implementation

• Apply Course Object.
• Migrate course connector to dart v3.

Testing Instructions

• Test course table related function work properly.

[Xanonymous-GitHub]

Because I didn't understand the functional requirements, I only reviewed the pure code part.

Maybe some parts are copied and pasted from the original code. If so, please just ignore my comment of them.

[Xanonymous-GitHub]

There are several suggestions of the Course class:

-> prevent putting unneeded instance member into class declaration, or make them as private member.

There are many public members are just for type conversion in the constructor body.
for example, the idString. If we don't talk about other aspects, we may still assume that this variable won't be accessed by others. In other words,

final myCourse = Course(...);
final fooBar = myCourse.idString; // x

this won't happened.
So you may consider making it to a private member, or just remove it through other ways of implementation.

-> prevent using late

late keyword enforces a variable's constraints at runtime instead of at compile time and since the variable is not definitely initialized, every time it is read, a runtime check is inserted to make sure it has been assigned a value. If it hasn’t, an exception will be thrown.

So in this case, I'd suggest declare those to be a nullable type property. Regardless of other issues, if a class's member value is definitely not possible to be assigned during construction, declare it as a nullable property. Then, implement null checks among code. This may be because the concepts of null-safety is to clearly distinguish null and non null resource. Hence in this case, if we can not knows a variable's value, it make sense to let it become a null value.

for example:

- late int id;
+ final int? id;

-> prevent creating static only class

In Dart, we declare a method of a class to static may be because we need a named utility class which provide some utility methods. for example:

final class MathUtil {
  static double squareOf(double n) => n * n;
}

void foo() {
  log(MathUtil.squareOf(9.9).toString());
}

This often happened when we want to:

• Share a bundle of functions, variables across a large scope.
• Avoid initialization of the class.

However, this may also have the following issues:

• makes the code more difficult to maintain and test
  • If lots of static members, including functions and variables in code, it becomes harder to define the scope of those static things, making encapsulation failed.
• subclasses cannot override static methods (reduce versatility)

These are just some general opinions. Lets dive into the Course class.
In this class, there're two methods defined to be a static function, which are:

• _convertPeriodSlotsToCoursePeriods
• _isNullOrEmpty

As we have known, a static member is something that OTHERs can use, without initialize an instance.
Here, those methods are declared as private, and only used in the constructor body.
That means, they are only used by the instance itself, and not for others (private).

So it seems there is no any requirement for declaring them as static.

In addition, we can find there're tones of old code using static to declare a whole class.
This is advised to be avoided.

The official Dart doc has also mentioned this case here: https://dart.dev/tools/linter-rules/avoid_classes_with_only_static_members

So base on these concerns, I'd suggest always create a class for defining its instance behaviors, and use solo function or DI when we need something to be shared across code.

[ntut-xuan]

For the suggestion of the "prevent putting unneeded instance member into class declaration, or make them as private member.":

• The Course class is used to transfer the node from web crewler into data transfer object, it expected to transfer the dirty String into good value and assign into member.
• Since I expected to input the string and set the transfer value into class. I'm not sure how to deal with it, so I add late when it will transform into differnt type (string -> int or string -> List<String> w/ new line split).
• It should be mentioned that JsonSerializable seems that not support transfer the constructor with List<String> parameter 🤔, therefore the teacherString and teachers exists.

For the suggestion of the "prevent using late":

• I assume it is late binding the data into member. So I add it.

It may need some good implement to optimize the Course class but I just can do it with dirty code as far. I optimize it on dfcf849.

[ntut-xuan]

Optimized on dfcf849

[Xanonymous-GitHub]

I'd suggest not doing huge data content checks in the constructor, because:

1. Maybe cause unneeded performance waste.
   Although we expect some of data may include strange things, we can still do such check when we really need to use them (at other layer of architecture, separating Model and DTO). That means, if we only need variable a, then we just check a without checking b, if a is not based on b.

2. JsonSerializable will actually help us do most of things like this.
   In the generated .g.dart file, there will be several general solutions of string parsing be implemented, if we have clearly declared variable data type, even it is a nullable type.

[ntut-xuan]

As above: #221 (comment)

[ntut-xuan]

Optimized on dfcf849

[Xanonymous-GitHub]

Suggest following #221 (comment).

[Xanonymous-GitHub]

Suggested change

	note: courseRowData[16].text));
	note: courseRowData[16].text,),);

Suggest adding trailing commas, and do format & analysis.

[Xanonymous-GitHub]

@ntut-xuan For this comment: #221 (comment)

Based on our discussions, it seems that we need not only using stricter conversion for both num and List<String> type of the Course model, but also late-avoided constant class syntax, there might be a much feasible solution.
let's discuss in the following steps.

A better approach for replacing static-member class JsonInit

In json_serializable, there's actually an advance tool called JsonConverter described in here.
The JsonConverter has the following prototype:

abstract class JsonConverter<T, S> {
  const JsonConverter();

  T fromJson(S json);
  S toJson(T object);
}

which allows as to implement our custom json conversion rules and use it as an annotation to specific field of the model class.
Here the type S is what we expect the original type from json raw data, while the type T is what we going to get after conversion.

Hence, I'd suggest using this approach for better coding structure and readability.
Let's look each of the real case in the following sections.

List<String> conversion

It seems that the API response data will give us something like this foo \nbar \n and you expect to get these two strings separately then put them into a List<String> so that you tried to use regexp to remove all new-line char \n then trim() them.

So that means we can implement a custom converter like this:

class _SplitByNewLineConverter implements JsonConverter<List<String>, String?> {
  const _SplitByNewLineConverter();

  @override
  List<String> fromJson(String? json) {
    if (json == null) {
      return const [];
    }

    return json.trim().split(r'\n').map((element) => element.trim()).toList();
  }

  @override
  String? toJson(List<String> object) => object.join('\n');
}

In this implementation, we can put our special check and separation logics into fromJson (since they are not too big) directly.

Then, we add this annotation to our field which requires this conversion:

...
  @_SplitByNewLineConverter()
  final List<String> teachers;

  @_SplitByNewLineConverter()
  final List<String> classNames;

  @_SplitByNewLineConverter()
  final List<String> classrooms;
...

I use String? as the source json data type because the raw data is definitely a string data at the first time from the response, additionally, use nullable type to state that this field may probably not given.

Using a nullable type in a model can clearly show that this thing is "not exist" instead of "not assigned".
So for example, if there's a field called foo in the model but the api response doesn't contains this field, which means there's no "foo": "..." in the raw response json, foo should be null instead of "" (empty string) because if the api response contains "foo": "", it will conflicts with the previous case. (This is why we use null here!)

So if we use this annotation, the generated .g.dart file will looks like this:

      teachers: const _SplitByNewLineConverter().fromJson(json['teachers'] as String?),
      classNames: const _SplitByNewLineConverter().fromJson(json['classNames'] as String?),
      classrooms: const _SplitByNewLineConverter().fromJson(json['classrooms'] as String?),

which calls our custom converter function to process the data.

int & double conversion

For converting these two types here, I saw you implemented the following logics:

  static int intInit(String value) {
    return value.replaceAll(RegExp(r"\s"), "").isNotEmpty ? int.parse(value) : 0;
  }

  static double doubleInit(String value) {
    return value.replaceAll(RegExp(r"\s"), "").isNotEmpty ? double.parse(value) : 0.00;
  }

However, there might be another more straightforward approach.

• Use nullable type.
  Like I've mentioned above, set a variable's value to the default value of its type may not seems to be a recommended approach, since this would possibly cause data misunderstandings when the value is really equal to the default value of the type.

  final double foo = getFooDataFrom(response);
  print(foo); // 0.0

  Is it easy to distinguish whether foo is given from server or not ?

  So I'd suggest changing their type to this:

    final int? id;
    final int? periodCount;
    final double? stage;
    final double? credit;

• Use num.tryParse(...) instead
  If we aware that there's something not a number appears in the raw json data, we can call this method to help us done this. Here's the prototype:

    // dart 2.19
    static num? tryParse(String input) {
      String source = input.trim();
      return int.tryParse(source) ?? double.tryParse(source);
    }

  This method will return the actual number according to the raw json data, or a null value if there's anything cause conversion failed. So that means it helps us to check "whether there's only lots if \s in the raw data".

So based on above concepts, we can create another JsonConverter like this:

class _ExcludeSpacesInNumberConverter implements JsonConverter<num?, String?> {
  const _ExcludeSpacesInNumberConverter();

  @override
  num? fromJson(String? json) {
    if (json == null) {
      return null;
    }

    return num.tryParse(json);
  }

  @override
  String? toJson(num? object) => object?.toString();
}

However, we have both int and double case, and the json_serializable will use the custom converter only when the result type is definitely matching the field type that we declared in the model class. This means we can not use num? as our return type, since we declare our fields either int? or double?.
So for clearing specify a num? should be a int? or double? we can make _ExcludeSpacesInNumberConverter become an abstract class,

abstract class _ExcludeSpacesInNumberConverter<T extends num> implements JsonConverter<T?, String?> {
  const _ExcludeSpacesInNumberConverter();

  @override
  T? fromJson(String? json) {
    if (json == null) {
      return null;
    }

    return num.tryParse(json) as T?;
  }

  @override
  String? toJson(T? object) => object?.toString();
}

then inherit it by those two classes:

class _ExcludeSpacesInIntegerConverter extends _ExcludeSpacesInNumberConverter<int> {
  const _ExcludeSpacesInIntegerConverter();

  @override
  int? fromJson(String? json) => super.fromJson(json)?.toInt();
}

class _ExcludeSpacesInDoubleConverter extends _ExcludeSpacesInNumberConverter<double> {
  const _ExcludeSpacesInDoubleConverter();

  @override
  double? fromJson(String? json) => super.fromJson(json)?.toDouble();
}

and finally, we can add annotation to our field:

  @_ExcludeSpacesInIntegerConverter()
  final int? id;

  @_ExcludeSpacesInIntegerConverter()
  final int? periodCount;

  @_ExcludeSpacesInDoubleConverter()
  final double? stage;

  @_ExcludeSpacesInDoubleConverter()
  final double? credit;

And the generated .g.dart file will looks like this:

      id: const _ExcludeSpacesInIntegerConverter().fromJson(json['id'] as String?),
      stage: const _ExcludeSpacesInDoubleConverter().fromJson(json['stage'] as String?),
      credit: const _ExcludeSpacesInDoubleConverter().fromJson(json['credit'] as String?),
      periodCount: const _ExcludeSpacesInIntegerConverter().fromJson(json['periodCount'] as String?),

That's it, I hope these concepts and ideas helps us improve the project :)

---

P.S. the whole code of `Course`

@JsonSerializable()
class Course {
  final String name;
  final String? category;
  final String? applyStatus;
  final String? language;
  final String? syllabusLink;
  final String? note;

  @_ExcludeSpacesInIntegerConverter()
  final int? id;

  @_ExcludeSpacesInIntegerConverter()
  final int? periodCount;

  @_ExcludeSpacesInDoubleConverter()
  final double? stage;

  @_ExcludeSpacesInDoubleConverter()
  final double? credit;

  @_SplitByNewLineConverter()
  final List<String> teachers;

  @_SplitByNewLineConverter()
  final List<String> classNames;

  @_SplitByNewLineConverter()
  final List<String> classrooms;

  final List<CoursePeriod> coursePeriods;

  const Course({
    required this.name,
    this.id,
    this.stage,
    this.credit,
    this.periodCount,
    this.category,
    this.applyStatus,
    this.language,
    this.syllabusLink,
    this.note,
    List<String>? teachers,
    List<String>? classNames,
    List<String>? classrooms,
    List<CoursePeriod>? coursePeriods,
  })  : teachers = teachers ?? const [],
        classNames = classNames ?? const [],
        classrooms = classrooms ?? const [],
        coursePeriods = coursePeriods ?? const [];

  factory Course.fromJson(Map<String, dynamic> json) => _$CourseFromJson(json);

  Map<String, dynamic> toJson() => _$CourseToJson(this);
}

class _SplitByNewLineConverter implements JsonConverter<List<String>, String?> {
  const _SplitByNewLineConverter();

  @override
  List<String> fromJson(String? json) {
    if (json == null) {
      return const [];
    }

    return json.trim().split(r'\n').map((element) => element.trim()).toList();
  }

  @override
  String? toJson(List<String> object) => object.join('\n');
}

abstract class _ExcludeSpacesInNumberConverter<T extends num> implements JsonConverter<T?, String?> {
  const _ExcludeSpacesInNumberConverter();

  @override
  T? fromJson(String? json) {
    if (json == null) {
      return null;
    }

    return num.tryParse(json) as T?;
  }

  @override
  String? toJson(T? object) => object?.toString();
}

class _ExcludeSpacesInIntegerConverter extends _ExcludeSpacesInNumberConverter<int> {
  const _ExcludeSpacesInIntegerConverter();

  @override
  int? fromJson(String? json) => super.fromJson(json)?.toInt();
}

class _ExcludeSpacesInDoubleConverter extends _ExcludeSpacesInNumberConverter<double> {
  const _ExcludeSpacesInDoubleConverter();

  @override
  double? fromJson(String? json) => super.fromJson(json)?.toDouble();
}