Refactoring-Quiz.md

Problem 1. Extract Method or Property to eliminate duplicate code

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.

Problem 2. Replace Magic Number with Symbolic Constant

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 or MAX.
• -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

Problem 3. Move Method

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".

Problem 4. Replace Nested Conditional with Guard Clauses

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

Problem 5. Fix two Long Methods using Replace Conditional with Polymorphism using Subclasses

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 or serializers = 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 in serializer.write_courses(courses) or serializer.write_courses(filename, courses).
• -10 code contains errors, other than omission of encoding='utf-8' in open.

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.

Problem 6. Hide Delegate

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).