Skip to content

Commit

Permalink
Refactor bank rules (#175)
Browse files Browse the repository at this point in the history 
* 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

rust-lang/rust-clippy#7692

* Set course status type outside of condition
  • Loading branch information
@benny-n
benny-n authored Jan 9, 2023
1 parent dca7835 commit 4a1ee69
Show file tree
Hide file tree
Showing 11 changed files with 137 additions and 184 deletions.
46 changes: 24 additions & 22 deletions packages/server/src/core/bank_rule/chain.rs
View file
Original file line number Diff line number Diff line change
@@ -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<String>) -> 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
}
}
24 changes: 11 additions & 13 deletions packages/server/src/core/bank_rule/elective.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::<f32>()
}
}
122 changes: 48 additions & 74 deletions packages/server/src/core/bank_rule/iterate_courses.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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<CourseId>,
courses: &HashMap<CourseId, Course>,
course_status: &mut CourseStatus,
course_id_in_list: &mut Option<String>,
replacements: &HashMap<CourseId, OptionalReplacements>,
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<String, Vec<String>>,
replacements_msg: fn(&Course) -> String|
-> Option<String> {
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,
Expand Down
35 changes: 16 additions & 19 deletions packages/server/src/core/bank_rule/malag.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::<f32>()
}
}
21 changes: 1 addition & 20 deletions packages/server/src/core/bank_rule/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,14 @@ pub mod iterate_courses;
pub mod malag;
pub mod specialization_groups;
pub mod sport;
#[allow(clippy::float_cmp)]
#[cfg(test)]
pub mod tests;

use std::collections::HashMap;

use crate::resources::{
catalog::OptionalReplacements,
course::{Course, CourseId, CourseStatus},
course::{Course, CourseId},
};

use super::degree_status::DegreeStatus;
Expand All @@ -29,21 +28,3 @@ pub struct BankRuleHandler<'a> {
pub catalog_replacements: &'a HashMap<CourseId, OptionalReplacements>,
pub common_replacements: &'a HashMap<CourseId, OptionalReplacements>,
}

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
}
}
}
24 changes: 10 additions & 14 deletions packages/server/src/core/bank_rule/sport.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::<f32>()
}
}
Loading

0 comments on commit 4a1ee69

Please sign in to comment.