class Student:
def credits(self):
return sum(c.credits for c in self.courses)
and use it in two places:
def add_course(self, course_id):
...
if self.credits() + course.credits > 24:
...
def print_course_list(self):
print(f{"{'Total Credits':54} {self.credits():2}")
- -5 applied the method in only one place.
Incorrect:
- Creating
available_course(course_id)
or similar. It occurs only 1 time in code.
The magic number is 24
. Create a constant, either:
- global scope (before class)
- class scope (class constant)
MAX_CREDITS = 24
class Student:
def add_course(self, course_id):
if self.credits() + course.credits > MAX_CREDITS:
print(
f"Total credits would exceed limit of {MAX_CREDITS}. Not enrolled.")
return False
Deductions:
- -4 Did not replace "24" inside the string message.
- -3 Non-descriptive name such as
CREDITS
orMAX
. - -3 Constant declared inside
add_course
instead of class or global scope. - -0 (but should deduct) Named constant not in uppercase, e.g.
max_credits
Anti-Refactoring (things that make the code harder to understand)
WIDTH_COURSE_NAME_FIELD = 54
WIDTH_CREDITS_FIELD = 2
WIDTH_COURSE_ID_FIELD = 8
The method to move is print_course_list
.
# utils.py
def print_course_list(student):
...
Why not move add_course
, drop_course
, etc?
- If you move one, you should move all these methods. Instructions said move one method.
- Breaks encapsulation (Feature Envy). The
courses
belong to Student. - Comparing "print a course list" and "manage my course list", I think "print a course list" is a much clearer violation of SRP.
- As mentioned in Movie Rental by Martin Fowler, there are many ways to print a course list: print plain text, print as HTML, or print as PDF. Separating this function from Student enables variations on "print course list".
This is identical to the exercise you did in Lab 11.
def __eq__(self, other) -> bool:
"""Two instances are equal if both are Students with the same name
and student id. Both objects must belong to the same class, so
don't use `isinstance` to test the type!
"""
if not other:
return False
# both must be Student or same subclass of Student
if type(other) != type(self):
return False
return self.name == other.name and self.student_id == other.student_id
The first test is unnecessary so you can simplify to:
def __eq__(self, other) -> bool:
# both must be Student or same subclass of Student
if type(other) != type(self):
return False
return self.name == other.name and self.student_id == other.student_id
Incorrect (checking type should be guard clause):
def __eq__(self, other) -> bool:
# both must be Student or same subclass of Student
return type(other) == type(self) and self.name == other.name and self.student_id == other.student_id
Blatent error. Instructions said do not use isinstance
but at least 6 students used it anyway.
def __eq__(self, other) -> bool:
# both must be Student or same subclass of Student
if not isinstance(other, type(self)):
return False
...
Anti-refactoring:
def __eq__(self, other) -> bool:
# both must be Student or same subclass of Student
if type(other) != type(self):
return False
check_name = (self.name == other.name)
check_student_id = (self.student_id == other.student_id)
return check_name and check_student_id
30 pt: Define a Serializer
class with CsvSerializer
and JsonSerializer
subclasses.
Deductions:
- -5 Registrar extracts the filetype itself and passes the filetype to Serializer, instead of passing whole filename.
- -10 Registrar chooses the concrete serializer itself.
- -20 Has Serializer subclasses that work, but Registrar does not use them.
- -2 Filename "Serializer.py" instead of "serializer.py" (Python naming convention).
- -0 Plural class and variable names:
class Serializers
orserializers = Serializers()
. Serializer is not a collection. The name should be singular.
10 pt: Serializer and Subclasses Code is Completely Correct.
This is not hard since you were given the code and tests for the two methods.
- -5
get_serializer(filename)
does not raise exception for filename without extension or unrecognized extension - -5 no
courses
parameter inserializer.write_courses(courses)
orserializer.write_courses(filename, courses)
. - -10 code contains errors, other than omission of
encoding='utf-8'
inopen
.
Note: you should move the read_courses
method to Serializer. No read_courses
in Registrar.
There is no reason for registrar.load_catalog
to call registrar.read_courses
.
This is a 5 point bonus. The idea is that Registrar
creates a base Serializer and calls read_courses
or write_courses
.
# Registrar
def load_catalog(self, filename):
"""Read course data from a file."""
serializer = Serializer()
self.courses = serializer.read_courses(filename)
or you can pass the filename to the constructor:
def load_catalog(self, filename):
"""Read course data from a file."""
serializer = Serializer(filename)
self.courses = serializer.read_courses()
The base Serializer methods delegate read_courses
and write_courses
to a subclass.
class Serializer:
def read_courses(self, filename: str) -> dict:
# Delegate to a subclass that is returned by get_serializer
return self.get_serializer(filename).read_courses(filename)
this is ugly code (passing filename
twice).
If filename
is a constructor parameter it is much cleaner (no filename parameter).