From 4a1ee694ec67ccea9d0c07e0dc73705f87c1ec3d Mon Sep 17 00:00:00 2001 From: Benny Nazimov <66024037+benny-n@users.noreply.github.com> Date: Tue, 10 Jan 2023 01:40:12 +0200 Subject: [PATCH] Refactor bank rules (#175) * Refactor bank rule code to be more rust-idiomatic * Refactor `chain` bank rule handler * Do not classify language courses as "malags" * Remove `float_cmp` allow, now default https://github.com/rust-lang/rust-clippy/pull/7692 * Set course status type outside of condition --- packages/server/src/core/bank_rule/chain.rs | 46 +++---- .../server/src/core/bank_rule/elective.rs | 24 ++-- .../src/core/bank_rule/iterate_courses.rs | 122 +++++++----------- packages/server/src/core/bank_rule/malag.rs | 35 +++-- packages/server/src/core/bank_rule/mod.rs | 21 +-- packages/server/src/core/bank_rule/sport.rs | 24 ++-- .../src/core/degree_status/compute_bank.rs | 20 +-- .../src/core/degree_status/compute_status.rs | 2 +- packages/server/src/core/mod.rs | 1 - packages/server/src/core/tests.rs | 4 - packages/server/src/resources/course.rs | 22 +++- 11 files changed, 137 insertions(+), 184 deletions(-) diff --git a/packages/server/src/core/bank_rule/chain.rs b/packages/server/src/core/bank_rule/chain.rs index 96cf171f..3b6eb30f 100644 --- a/packages/server/src/core/bank_rule/chain.rs +++ b/packages/server/src/core/bank_rule/chain.rs @@ -1,34 +1,36 @@ -use crate::core::types::Chain; +use crate::{ + core::types::Chain, + resources::course::{CourseId, CourseStatus}, +}; use super::BankRuleHandler; impl<'a> BankRuleHandler<'a> { pub fn chain(mut self, chains: &[Chain], chain_done: &mut Vec) -> f32 { let credit_info = self.iterate_course_list(); + let map_to_actual_course = |course_id: &CourseId| -> Option<&CourseStatus> { + credit_info + .handled_courses + .get(course_id) + .and_then(|course_id| self.degree_status.get_course_status(course_id)) + }; for chain in chains { - //check if the user completed one of the chains. - let mut completed_chain = true; - for course_id in chain { - if let Some(course_id) = credit_info.handled_courses.get(course_id) { - if let Some(course_status) = self.degree_status.get_course_status(course_id) { - if course_status.completed() { - chain_done.push(course_status.course.name.clone()); - } else { - completed_chain = false; - break; - } - } - } else { - completed_chain = false; - break; - } - } - if completed_chain { - return credit_info.sum_credit; - } else { - chain_done.clear(); + let chain_complete = chain.iter().all(|course_id| { + map_to_actual_course(course_id) + .map(|course_status| course_status.completed()) + .unwrap_or(false) + }); + if chain_complete { + *chain_done = chain + .iter() + .filter_map(|course_id| map_to_actual_course(course_id)) + .map(|course_status| course_status.course.name.clone()) + .collect(); + + break; } } + credit_info.sum_credit } } diff --git a/packages/server/src/core/bank_rule/elective.rs b/packages/server/src/core/bank_rule/elective.rs index 6d8f692c..e1b5f9de 100644 --- a/packages/server/src/core/bank_rule/elective.rs +++ b/packages/server/src/core/bank_rule/elective.rs @@ -2,18 +2,16 @@ use super::BankRuleHandler; impl<'a> BankRuleHandler<'a> { pub fn elective(self) -> f32 { - let mut sum_credit = self.credit_overflow; - for course_status in &mut self.degree_status.course_statuses { - if course_status.valid_for_bank(&self.bank_name) - && !(course_status.semester.is_none() && course_status.course.credit == 0.0) - { - Self::set_type_and_add_credit( - course_status, - self.bank_name.clone(), - &mut sum_credit, - ); - } - } - sum_credit + self.credit_overflow + + self + .degree_status + .course_statuses + .iter_mut() + .filter(|course_status| course_status.valid_for_bank(&self.bank_name)) + .filter(|course_status| { + course_status.semester.is_some() || course_status.course.credit != 0.0 + }) + .filter_map(|course_status| course_status.set_type(self.bank_name.clone()).credit()) + .sum::() } } diff --git a/packages/server/src/core/bank_rule/iterate_courses.rs b/packages/server/src/core/bank_rule/iterate_courses.rs index 7eb6b44b..e282858f 100644 --- a/packages/server/src/core/bank_rule/iterate_courses.rs +++ b/packages/server/src/core/bank_rule/iterate_courses.rs @@ -2,8 +2,7 @@ use std::collections::HashMap; use crate::core::messages; use crate::core::types::CreditInfo; -use crate::resources::catalog::OptionalReplacements; -use crate::resources::course::{Course, CourseId, CourseStatus}; +use crate::resources::course::Course; use super::BankRuleHandler; @@ -13,91 +12,66 @@ impl<'a> BankRuleHandler<'a> { let mut sum_credit = self.credit_overflow; let mut count_courses = self.courses_overflow; let mut handled_courses = HashMap::new(); // mapping between the course in the catalog to the course which was taken by the student (relevant for replacements) - for course_status in self.degree_status.course_statuses.iter_mut() { - let mut course_chosen_for_bank = false; - if course_status.valid_for_bank(&self.bank_name) { + self.degree_status + .course_statuses + .iter_mut() + .filter(|course_status| course_status.valid_for_bank(&self.bank_name)) + .filter_map(|course_status| { if self.course_list.contains(&course_status.course.id) { - course_chosen_for_bank = true; - handled_courses.insert( - course_status.course.id.clone(), - course_status.course.id.clone(), - ); + Some((course_status.course.id.clone(), course_status)) } else { - #[inline(always)] - fn check_if_replacement( - course_list: &Vec, - courses: &HashMap, - course_status: &mut CourseStatus, - course_id_in_list: &mut Option, - replacements: &HashMap, - replacements_msg: impl Fn(&Course) -> String, - ) { - for course_id in course_list { - if let Some(replacements) = &replacements.get(course_id) { - if replacements.contains(&course_status.course.id) { - let optional_course = courses.get(course_id); - course_status.set_msg(replacements_msg( - optional_course.unwrap_or(&Course { - id: course_id.clone(), - ..Default::default() - }), - )); - - *course_id_in_list = Some(course_id.into()); - break; - } - } - } - } - // check if course_status is a replacement for a course in course list - let mut course_id_in_list = None; - // First try to find catalog replacements - check_if_replacement( - &self.course_list, - self.courses, - course_status, - &mut course_id_in_list, + let mut find_replacement = |replacements: &HashMap>, + replacements_msg: fn(&Course) -> String| + -> Option { + self.course_list.iter().find_map(|course_id| { + replacements + .get(course_id) + .and_then(|replacements| { + replacements.contains(&course_status.course.id).then(|| { + course_status.set_msg(replacements_msg( + self.courses.get(course_id).unwrap_or(&Course { + id: course_id.clone(), + ..Default::default() + }), + )); + Some(course_id.into()) + }) + }) + .flatten() + }) + }; + // check if course_status is a common replacement or a catalog replacement for a course in course list + let course_id_in_list = find_replacement( self.catalog_replacements, messages::catalog_replacements_msg, - ); - - if course_id_in_list.is_none() { - // Didn't find a catalog replacement so trying to find a common replacement - check_if_replacement( - &self.course_list, - self.courses, - course_status, - &mut course_id_in_list, + ) + .or_else(|| { + find_replacement( self.common_replacements, messages::common_replacements_msg, - ); - } + ) + }); + if let Some(course_id) = course_id_in_list { - course_chosen_for_bank = true; - handled_courses.insert(course_id.clone(), course_status.course.id.clone()); + Some((course_id, course_status)) } else if course_status.r#type == Some(self.bank_name.clone()) { // The course is not in the list and not a replacement for any other course on the list // but its type is modified and its the current bank name. // Therefore the course should be added anyway. - course_chosen_for_bank = true; - handled_courses.insert( - course_status.course.id.clone(), - course_status.course.id.clone(), - ); + Some((course_status.course.id.clone(), course_status)) + } else { + None } } - } - - if course_chosen_for_bank - && Self::set_type_and_add_credit( - course_status, - self.bank_name.clone(), - &mut sum_credit, - ) - { - count_courses += 1; - } - } + }) + .for_each(|(course_id, course_status)| { + handled_courses.insert(course_id, course_status.course.id.clone()); + course_status.set_type(&self.bank_name); + if let Some(credit) = course_status.credit() { + sum_credit += credit; + count_courses += 1; + } + }); CreditInfo { sum_credit, diff --git a/packages/server/src/core/bank_rule/malag.rs b/packages/server/src/core/bank_rule/malag.rs index a430ba45..619c4fd7 100644 --- a/packages/server/src/core/bank_rule/malag.rs +++ b/packages/server/src/core/bank_rule/malag.rs @@ -3,25 +3,22 @@ use crate::resources::course::CourseId; use super::BankRuleHandler; impl<'a> BankRuleHandler<'a> { - // TODO: remove this when removing the condition in the if statement - #[allow(clippy::float_cmp)] pub fn malag(self, malag_courses: &[CourseId]) -> f32 { - let mut sum_credit = self.credit_overflow; - for course_status in &mut self.degree_status.course_statuses { - if course_status.valid_for_bank(&self.bank_name) - && (malag_courses.contains(&course_status.course.id) - // TODO: remove this line after we get the answer from the coordinates - || (course_status.course.id.starts_with("324") && course_status.course.credit == 2.0) - || course_status.r#type.is_some()) - // If type is not none it means valid_for_bank returns true because the user modified this course to be malag - { - Self::set_type_and_add_credit( - course_status, - self.bank_name.clone(), - &mut sum_credit, - ); - } - } - sum_credit + self.credit_overflow + + self + .degree_status + .course_statuses + .iter_mut() + .filter(|course_status| course_status.valid_for_bank(&self.bank_name)) + .filter(|course_status| { + malag_courses.contains(&course_status.course.id) + || course_status.r#type.is_some() + // TODO: maybe think of a better way to do this + || (course_status.course.id.starts_with("324") + && course_status.course.credit == 2.0 + && !course_status.is_language()) + }) + .filter_map(|course_status| course_status.set_type(&self.bank_name).credit()) + .sum::() } } diff --git a/packages/server/src/core/bank_rule/mod.rs b/packages/server/src/core/bank_rule/mod.rs index 9b404481..1c323e9b 100644 --- a/packages/server/src/core/bank_rule/mod.rs +++ b/packages/server/src/core/bank_rule/mod.rs @@ -6,7 +6,6 @@ pub mod iterate_courses; pub mod malag; pub mod specialization_groups; pub mod sport; -#[allow(clippy::float_cmp)] #[cfg(test)] pub mod tests; @@ -14,7 +13,7 @@ use std::collections::HashMap; use crate::resources::{ catalog::OptionalReplacements, - course::{Course, CourseId, CourseStatus}, + course::{Course, CourseId}, }; use super::degree_status::DegreeStatus; @@ -29,21 +28,3 @@ pub struct BankRuleHandler<'a> { pub catalog_replacements: &'a HashMap, pub common_replacements: &'a HashMap, } - -impl<'a> BankRuleHandler<'a> { - // set the type of the course, and add its credit to sum_credit if the user passed the course. - // returns true if the credit have been added, false otherwise. - pub fn set_type_and_add_credit( - course_status: &mut CourseStatus, - bank_name: String, - sum_credit: &mut f32, - ) -> bool { - course_status.set_type(bank_name); - if course_status.completed() { - *sum_credit += course_status.course.credit; - true - } else { - false - } - } -} diff --git a/packages/server/src/core/bank_rule/sport.rs b/packages/server/src/core/bank_rule/sport.rs index 32721a3c..0d31204b 100644 --- a/packages/server/src/core/bank_rule/sport.rs +++ b/packages/server/src/core/bank_rule/sport.rs @@ -2,19 +2,15 @@ use super::BankRuleHandler; impl<'a> BankRuleHandler<'a> { pub fn sport(self) -> f32 { - let mut sum_credit = self.credit_overflow; - for course_status in &mut self.degree_status.course_statuses { - if course_status.valid_for_bank(&self.bank_name) - && (course_status.is_sport() || course_status.r#type.is_some()) - // If type is not none it means valid_for_bank returns true because the user modified this course to be sport - { - Self::set_type_and_add_credit( - course_status, - self.bank_name.clone(), - &mut sum_credit, - ); - } - } - sum_credit + self.credit_overflow + + self + .degree_status + .course_statuses + .iter_mut() + .filter(|course_status| course_status.valid_for_bank(&self.bank_name)) + // If the course is valid for the bank, and it's type is set (Some), then it must be set to sport (or else it would be invalid for the bank) + .filter(|course_status| course_status.is_sport() || course_status.r#type.is_some()) + .filter_map(|course_status| course_status.set_type(&self.bank_name).credit()) + .sum::() } } diff --git a/packages/server/src/core/degree_status/compute_bank.rs b/packages/server/src/core/degree_status/compute_bank.rs index f1674eb4..0f76bf32 100644 --- a/packages/server/src/core/degree_status/compute_bank.rs +++ b/packages/server/src/core/degree_status/compute_bank.rs @@ -12,7 +12,7 @@ use super::DegreeStatusHandler; impl<'a> DegreeStatusHandler<'a> { pub fn compute_bank( &mut self, - bank: &CourseBank, + bank: CourseBank, course_list_for_bank: Vec, credit_overflow: f32, missing_credit_from_prev_banks: f32, @@ -34,11 +34,9 @@ impl<'a> DegreeStatusHandler<'a> { let mut count_courses = 0; // for accumulate courses rule let mut missing_credit = 0.0; // for all rule let mut completed = true; - let mut groups_done_list = Vec::new(); // for specialization groups rule - let mut chain_done = Vec::new(); // for chain rule let mut msg = None; - match &bank.rule { + match bank.rule { Rule::All => { let mut sum_credit_requirement = 0.0; sum_credit = bank_rule_handler.all(&mut sum_credit_requirement, &mut completed); @@ -55,20 +53,22 @@ impl<'a> DegreeStatusHandler<'a> { Rule::AccumulateCredit => sum_credit = bank_rule_handler.accumulate_credit(), Rule::AccumulateCourses(num_courses) => { sum_credit = bank_rule_handler.accumulate_courses(&mut count_courses); - count_courses = self.handle_courses_overflow(bank, *num_courses, count_courses); - completed = count_courses >= *num_courses; + count_courses = self.handle_courses_overflow(&bank, num_courses, count_courses); + completed = count_courses >= num_courses; } Rule::Malag => sum_credit = bank_rule_handler.malag(&self.malag_courses), Rule::Sport => sum_credit = bank_rule_handler.sport(), Rule::Elective => sum_credit = bank_rule_handler.elective(), - Rule::Chains(chains) => { + Rule::Chains(ref chains) => { + let mut chain_done = Vec::new(); sum_credit = bank_rule_handler.chain(chains, &mut chain_done); completed = !chain_done.is_empty(); if completed { msg = Some(messages::completed_chain_msg(chain_done)); } } - Rule::SpecializationGroups(specialization_groups) => { + Rule::SpecializationGroups(ref specialization_groups) => { + let mut groups_done_list = Vec::new(); sum_credit = bank_rule_handler .specialization_group(specialization_groups, &mut groups_done_list); completed = groups_done_list.len() >= specialization_groups.groups_number; @@ -86,10 +86,10 @@ impl<'a> DegreeStatusHandler<'a> { if let Some(bank_credit) = bank.credit { let new_credit = bank_credit - missing_credit + missing_credit_from_prev_banks; new_bank_credit = Some(new_credit); - sum_credit = self.handle_credit_overflow(bank, new_credit, sum_credit); + sum_credit = self.handle_credit_overflow(&bank, new_credit, sum_credit); completed &= sum_credit >= new_credit; } else { - sum_credit = self.handle_credit_overflow(bank, 0.0, sum_credit); + sum_credit = self.handle_credit_overflow(&bank, 0.0, sum_credit); }; self.degree_status diff --git a/packages/server/src/core/degree_status/compute_status.rs b/packages/server/src/core/degree_status/compute_status.rs index ec8f5b4b..6589f284 100644 --- a/packages/server/src/core/degree_status/compute_status.rs +++ b/packages/server/src/core/degree_status/compute_status.rs @@ -25,7 +25,7 @@ impl<'a> DegreeStatusHandler<'a> { } self.compute_bank( - &bank, + bank, course_list_for_bank, credit_overflow, missing_credit, diff --git a/packages/server/src/core/mod.rs b/packages/server/src/core/mod.rs index 687c48ef..ba5238f9 100644 --- a/packages/server/src/core/mod.rs +++ b/packages/server/src/core/mod.rs @@ -6,6 +6,5 @@ pub mod parser; pub mod types; pub mod catalog_validations; -#[allow(clippy::float_cmp)] #[cfg(test)] pub mod tests; diff --git a/packages/server/src/core/tests.rs b/packages/server/src/core/tests.rs index 569da84c..f9844ba9 100644 --- a/packages/server/src/core/tests.rs +++ b/packages/server/src/core/tests.rs @@ -795,10 +795,6 @@ async fn test_software_engineer_itinerary() { ); assert_eq!( degree_status.overflow_msgs[2], - messages::credit_overflow_msg(2.0, "בחירת העשרה", "בחירה חופשית") - ); - assert_eq!( - degree_status.overflow_msgs[3], messages::credit_leftovers_msg(0.0) ); } diff --git a/packages/server/src/resources/course.rs b/packages/server/src/resources/course.rs index 5b1e5a56..4844b31f 100644 --- a/packages/server/src/resources/course.rs +++ b/packages/server/src/resources/course.rs @@ -109,6 +109,10 @@ impl CourseStatus { self.state == Some(CourseState::Complete) } + pub fn credit(&self) -> Option { + self.completed().then_some(self.course.credit) + } + pub fn extract_semester(&self) -> f32 { self.semester .as_ref() @@ -143,21 +147,27 @@ impl CourseStatus { Some(CourseState::NotComplete) }); } - pub fn set_type(&mut self, r#type: String) -> &mut Self { - self.r#type = Some(r#type); + pub fn set_type(&mut self, r#type: impl AsRef) -> &mut Self { + self.r#type = Some(r#type.as_ref().to_owned()); self } - pub fn set_msg(&mut self, msg: String) -> &mut Self { - self.additional_msg = Some(msg); + + pub fn set_msg(&mut self, msg: impl AsRef) -> &mut Self { + self.additional_msg = Some(msg.as_ref().to_owned()); self } - pub fn set_specialization_group_name(&mut self, group_name: &str) { - self.specialization_group_name = Some(group_name.to_string()); + pub fn set_specialization_group_name(&mut self, group_name: impl AsRef) { + self.specialization_group_name = Some(group_name.as_ref().to_owned()); } pub fn is_sport(&self) -> bool { self.course.id.starts_with("394") } + + pub fn is_language(&self) -> bool { + let course_num = self.course.id.parse::().unwrap_or_default(); + (324600..=324695).contains(&course_num) || (324002..=324068).contains(&course_num) + } } #[derive(Clone, Debug, Deserialize, Serialize)]