21 nm concept composite learning units by AhmadAlamoush · Pull Request #15 · e-Learning-by-SSE/nm-skill-lib

AhmadAlamoush · 2024-02-16T12:01:41Z

For issue e-Learning-by-SSE/nm-concept/issues/21

Apply the new concept of composite learning units as discussed in issue #21 in nm-concept (Concept of CompositeUnits) Issue #21

Apply the new concept of composite learning units as discussed in issue #21 in nm-concept (Concept of CompositeUnits) (Missing updates) Issue #21

spark-sse

As mentioned during our previous discussion, my current familiarity with the composite units is limited, and due to time constraints, I'm unable to delve deeply into them at this moment. Consequently, while I may not be able to provide detailed feedback on the interfaces at this time, I've taken the opportunity to focus on aspects of code quality where I can offer meaningful insights.

spark-sse · 2024-02-19T09:46:58Z

+import { LearningUnit, Skill } from "./types";
+
+export function computeSkills(
+	LearningUnits: LearningUnit[]


use camel case instead of pascal case for variable names

spark-sse · 2024-02-19T09:47:30Z

+}
+
+function computeTaughtSkills(
+	LU: LearningUnit


dont use upper case var-names here

spark-sse · 2024-02-19T09:53:21Z

+	if (LU.children != undefined) {
+		LU.children.forEach(child => {
+			const teachingGoalsSkills = child.teachingGoals.filter(skill => !skillsString.includes(skill.id));
+			skills = skills.concat(teachingGoalsSkills);
+			skillsString = skillsString.concat(teachingGoalsSkills.map(skill => skill.id));
+		});
+	}


if not necessary. try to stick to our common practices and use optional chaining in simple cases:

learningUnits?.children.forEach(...)

spark-sse · 2024-02-19T09:55:49Z

+	suggestions = suggestions.filter(skill => !(LU.exportedRecommendedSkills!.map(sk => sk.skill.id).includes(skill.skill.id)
+											 && LU.exportedRecommendedSkills!.map(sk => sk.weight).includes(skill.weight)));


complexity of this line is too high. Try to seperate them, make it more explicit, or chain the filters.

spark-sse · 2024-02-19T09:59:52Z

+export function getRequiredSkills(): Skill[] {
+	const LU: LearningUnit = this;
+	let skills: Skill[] = [];
+	if (LU.children.length > 0) {
+		skills = LU.externalRequiredSkills!.concat(LU.exportedRequiredSkills!);
+	} else {
+		skills = LU.requiredSkills;
+	}
+
+	return skills;
+}
+
+export function getSuggestedSkills(): { weight: number; skill: Skill }[] {
+	const LU: LearningUnit = this;
+	let skills: { weight: number; skill: Skill }[] = [];
+
+	if (LU.children.length > 0) {
+		skills = LU.externalRecommendedSkills!.concat(LU.exportedRecommendedSkills!);
+	} else {
+		skills = LU.suggestedSkills;
+	}
+
+	return skills;
+}


duplicate code. Try to use a common method which helps you here

spark-sse · 2024-02-19T10:23:13Z

            }
        }
-        goalString.split(",").filter(skill => skill.length > 0).forEach(sk => {
+        goalsArray.filter(skill => skill.length > 0).forEach(sk => {


possible unnecessary filter.

in your case: if i assume if skill.length is zero, the !openListMap.has(skill) wont be true. If this is the case, then you can remove the filter here.

spark-sse · 2024-02-19T10:25:49Z

+    // If goalsArray still has some goals, then we check the skill groups in globalKnowledge
    // To remove parent skill if all his children is found (Reachable)
-    // We do loop over the goal skills, to void the order of the skills until no more changes on the goalString
-    if (goalString.length > 0) {
+    // We do loop over the goal skills, to void the order of the skills until no more changes on the goalsArray
+    if (goalsArray.length > 0) {
        while (true) {
-            const goalSkills = goalString.split(",").filter(skill => skill.length > 0);
+            const goalSkills = goalsArray.slice(0);
            let goalSkillsCount = goalSkills.length;
            goalSkills.forEach(sk => {
                const parentSkill = globalKnowledge.getAllParents().find(parentSkill => parentSkill.id == sk);
                if (parentSkill) {
                    let parentSkillCount = parentSkill.nestedSkills.length;
                    parentSkill.nestedSkills.forEach(skill => {
-                            if (goalString.includes(`,`.concat(skill).concat(`,`))) {
+                            if (goalsArray.includes(skill)) {
                                parentSkillCount--;
                            }
                        }
                    );
                    if (parentSkillCount == parentSkill.nestedSkills.length) {
-                        goalString = goalString.replace(`,${sk},`,`,`);
+                        goalsArray.splice(goalsArray.indexOf(sk), 1)
                        goalSkillsCount--;
                    }


hard to read code with a lot of complexity and nested loops and conditions. especially the while(true) could be hard to understand.

I'm aware that this was introduced via this pull requests, but this should be addressed in the future.

spark-sse · 2024-02-19T10:44:21Z

+		const missingSkills = previousUnit.getTeachingGoals()
 			.map(goal => goal.id)
 			// Do not copy hard constraints also to soft constraints
-			.filter(goalId => !currentUnit.requiredSkills.map(skill => skill.id).includes(goalId))
+			.filter(goalId => !currentUnit.getRequiredSkills().map(skill => skill.id).includes(goalId))
 			// Do not copy currently taught skills to avoid cycles
-			.filter(goalId => !currentUnit.teachingGoals.map(skill => skill.id).includes(goalId));
+			.filter(goalId => !currentUnit.getTeachingGoals().map(skill => skill.id).includes(goalId));


To improve readability and maintainability, I recommend introducing smaller helper variables or functions in this section. This approach not only makes the code more self-explanatory but also leverages a functional programming style that is both elegant and potentially more efficient. Here's an example to illustrate this:

const isNoTeachingGoal= goalId => !currentUnit.getTeachingGoals().some(skill => skill.id === goalId); const isNoHardConstraint= goalId => !currentUnit.getRequiredSkills().some(skill => skill.id === goalId); const missingSkills = previousUnit.getTeachingGoals() .map(goal => ({ id: goal.id })) .filter(isNoTeachingGoal) .filter(isNoHardConstraint);

With this refactoring, you achieve several improvements:

Enhanced Code Self-documentation: By assigning clear and descriptive names to variables, the code becomes self-explanatory, eliminating the need for additional comments. May think about the suggested variable names (isNoHardConstrait). It can be hard to guess what you're trying to do from the outside. Other suggestion would be doesNotMeetTeachingGoal and lacksRequiredSkill

Readability: The use of well-named helper functions makes the code flow easier to understand at a glance, adhering to a readable functional programming approach.

Efficiency: Utilizing Array.prototype.some allows for short-circuiting logic, which can be more performance-efficient, especially with larger arrays.

Note: remember to use teaching goals as property like currentUnit.teachingGoals in the future as mentioned in my other comments.

spark-sse · 2024-02-19T10:48:32Z

-    // We do loop over the goal skills, to void the order of the skills until no more changes on the goalString
-    if (goalString.length > 0) {
+    // We do loop over the goal skills, to void the order of the skills until no more changes on the goalsArray
+    if (goalsArray.length > 0) {


Your observation highlights a potential inconsistency in the code related to the type of length check being performed. Initially, it appears there was a check on the length of a string, but now the check has shifted to the length of an array. This change might indicate one of two scenarios:

Missed Bug: If the intent was to continue checking a string's length, this new check on an array's length could be an oversight or error that needs addressing.

Unnecessary Check: Alternatively, if the context has changed such that the original string length check is no longer applicable, and an array length check doesn't serve a clear purpose, then this new check might be redundant.

spark-sse · 2024-02-19T10:48:54Z

-    // Note: I used a string to avoid performance issues in finding and deleting (I will do more research to compare it with other alternatives)
-    let goalString = `,`.concat(initialState.learnedSkills.join(",").concat(`,`));
+    // We use goalArray to find the missing skills that do not have requirements and the skill that uses skill groups.
+    const goalsArray = initialState.learnedSkills.slice(0);


Incorporating the variable type into its name is generally discouraged as it does not align with best naming practices. Instead, opt for meaningful and descriptive names that reflect the variable's purpose or the data it represents.

For instance, within a concise function where the context is easily understood, a simple name like goals would be perfectly adequate. In scenarios involving more complex functions—which, ideally, should be avoided by aiming for smaller, more focused functions—choosing more descriptive names becomes essential. This not only enhances readability but also aids in understanding the code's logic at a glance. If you find yourself needing overly descriptive names due to the function's complexity, it might be a signal to consider refactoring the code into smaller, more manageable pieces.

AhmadAlamoush added 2 commits February 14, 2024 00:46

feat(new): 21-nm-concept-composite-learning-units

e3d0522

Apply the new concept of composite learning units as discussed in issue #21 in nm-concept (Concept of CompositeUnits) Issue #21

feat(new): 21-nm-concept-composite-learning-units

2bc35db

Apply the new concept of composite learning units as discussed in issue #21 in nm-concept (Concept of CompositeUnits) (Missing updates) Issue #21

spark-sse requested changes Feb 19, 2024

View reviewed changes

Navigation Menu

21 nm concept composite learning units - #15

21 nm concept composite learning units#15
AhmadAlamoush wants to merge 2 commits into
mainfrom
21-nm-concept-composite-learning-units

AhmadAlamoush commented Feb 16, 2024

Uh oh!

spark-sse left a comment

Uh oh!

spark-sse Feb 19, 2024

Uh oh!

spark-sse Feb 19, 2024

Uh oh!

spark-sse Feb 19, 2024

Uh oh!

spark-sse Feb 19, 2024

Uh oh!

spark-sse Feb 19, 2024

Uh oh!

spark-sse Feb 19, 2024

Uh oh!

spark-sse Feb 19, 2024

Uh oh!

spark-sse Feb 19, 2024

Uh oh!

spark-sse Feb 19, 2024

Uh oh!

spark-sse Feb 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		suggestions = suggestions.filter(skill => !(LU.exportedRecommendedSkills!.map(sk => sk.skill.id).includes(skill.skill.id)
		&& LU.exportedRecommendedSkills!.map(sk => sk.weight).includes(skill.weight)));

Conversation

AhmadAlamoush commented Feb 16, 2024

Uh oh!

spark-sse left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants