Skip to main content
Code Review

Return to Answer

added 357 characters in body
Source Link
Richard Neumann
Richard Neumann
  • 6.2k
  • 21
  • 41
OPTIONAL_PROGRAMS = {
    "Google Chrome": install_google_chrome,
    "HMSS": install_hmss,
    "Kingdom of Cyprus": install_kingdom_of_cyprus,
    ...
}

[...]

class HMSoftwareInstaller:
    [...]
    
    def install(name: str, function: Callable[[], bool]) -> bool:
        print(f"Installing {name}...")
        
        if not (result := function()):
            self.failure_log.append(f"Install {name}")
        
        return result

    def run_non_essentials(self) -> bool:
        """ Run the installation processes. """
        
        return all(self[self.install(*program) for program in OPTIONAL_PROGRAMS.items()])

The same applies to run_essentials().

The functions invoked do not use self anyway, so they should be redefined as purefree functions.

The same applies to run_essentials().

Use built-ins where possible

The function upgrade_python() does unnecessarily store state. You can rewrite it as:

def upgrade_python():
    """ Install PIP3 and other useful Python hangers-on. """
    return all([
        install_via_apt("python3-pip"),
        pip3_install("pylint"),
        pip3_install("pytest")
    ])

OPTIONAL_PROGRAMS = {
    "Google Chrome": install_google_chrome,
    "HMSS": install_hmss,
    "Kingdom of Cyprus": install_kingdom_of_cyprus,
    ...
}

[...]

class HMSoftwareInstaller:
    [...]
    
    def install(name: str, function: Callable[[], bool]) -> bool:
        print(f"Installing {name}...")
        
        if not (result := function()):
            self.failure_log.append(f"Install {name}")
        
        return result

    def run_non_essentials(self) -> bool:
        """ Run the installation processes. """
        
        return all(self.install(*program) for program in OPTIONAL_PROGRAMS.items())

The functions invoked do not use self anyway, so they should be redefined as pure functions.

The same applies to run_essentials().

OPTIONAL_PROGRAMS = {
    "Google Chrome": install_google_chrome,
    "HMSS": install_hmss,
    "Kingdom of Cyprus": install_kingdom_of_cyprus,
    ...
}

[...]

class HMSoftwareInstaller:
    [...]
    
    def install(name: str, function: Callable[[], bool]) -> bool:
        print(f"Installing {name}...")
        
        if not (result := function()):
            self.failure_log.append(f"Install {name}")
        
        return result

    def run_non_essentials(self) -> bool:
        """ Run the installation processes. """
        
        return all([self.install(*program) for program in OPTIONAL_PROGRAMS.items()])

The same applies to run_essentials().

The functions invoked do not use self anyway, so they should be redefined as free functions.

Use built-ins where possible

The function upgrade_python() does unnecessarily store state. You can rewrite it as:

def upgrade_python():
    """ Install PIP3 and other useful Python hangers-on. """
    return all([
        install_via_apt("python3-pip"),
        pip3_install("pylint"),
        pip3_install("pytest")
    ])
Source Link
Richard Neumann
Richard Neumann
  • 6.2k
  • 21
  • 41

Don't repeat yourself

    def run_non_essentials(self):
        """ Run the installation processes. """
        result = True
        print("Installing Google Chrome...")
        if not self.install_google_chrome():
            self.failure_log.append("Install Google Chrome")
            result = False
        print("Installing HMSS...")
        if not self.install_hmss():
            self.failure_log.append("Install HMSS")
        print("Installing Kingdom of Cyprus...")
        if not self.install_kingdom_of_cyprus():
            self.failure_log.append("Install Kingdom of Cyprus")
            result = False
        print("Installing Chancery repos...")
        if not self.install_chancery():
            self.failure_log.append("Install Chancery repos")
            result = False
        print("Installing HGMJ...")
        if not self.install_hgmj():
            self.failure_log.append("Install HGMJ")
            result = False
        print("Installing other third party...")
        if not self.install_other_third_party():
            self.failure_log.append("Install other third party")
            result = False
        print("Changing wallpaper...")
        if not self.change_wallpaper():
            self.failure_log.append("Change wallpaper")
            # It doesn't matter too much if this fails.
        return result

Can be generalized into:

OPTIONAL_PROGRAMS = {
    "Google Chrome": install_google_chrome,
    "HMSS": install_hmss,
    "Kingdom of Cyprus": install_kingdom_of_cyprus,
    ...
}

[...]

class HMSoftwareInstaller:
    [...]
    
    def install(name: str, function: Callable[[], bool]) -> bool:
        print(f"Installing {name}...")
        
        if not (result := function()):
            self.failure_log.append(f"Install {name}")
        
        return result

    def run_non_essentials(self) -> bool:
        """ Run the installation processes. """
        
        return all(self.install(*program) for program in OPTIONAL_PROGRAMS.items())

The functions invoked do not use self anyway, so they should be redefined as pure functions.

The same applies to run_essentials().

Consider using pathlib.Path

...to manage paths, rather than the os.* functions.