Application install code- Need to remove Nested Loops and make code more compact and readable. Code installs application and its version as per req

Question

This code installs the application, according to the type of application selected like alpha, beta, and released, also it checks for latest version in each type.

def install(
    self,
    app_version=None,
    timeout=20,
    cancel_button_verification=False,
    install_type=None,
    version_type=None,
    latest_check=True,
    select_isv_app_path_for_install: str = None,
):
    """install app"""
    is_installed = False
    is_cancel_button_check = False
    assert (
        self.open()
    ), "[ExchangeSearch] Failed to open app detailed card"  # open detailed card
    app_name = self.app_name
    app_version = self.select_version_and_channel(
        version_type, app_version, latest_check
    )
    self.log.info("[ExchangeSearch] {0} version:{1}".format(app_name, app_version))
    if self.is_installed:
        is_installed = True
        self.log.info("[ExchangeSearch] {0} is already installed".format(app_name))
    else:
        self.install_button.click()
        WebDriverWait(self.web_driver.driver, 10).until(
            lambda x: self.is_installing
        )
        assert (
            self.is_installing
        ), "Install button text not updated to 'INSTALLING' "
        end = time.time() + timeout * 150
        ae_show_all_arrow_click_count = False
        ae_installation_list_div_count = False
        while time.time() <= end:
            if self.is_installing:
                if app_name.lower() == "drive":
                    xpath = "div[contains(@class,'ProgressBar__StyledProgressBar')]//span[@title='Installing - Register']"
                    if self.elm_by_xpath(xpath):
                        time.sleep(3)
                        os.system(
                            'wmic process where name="drive-setup.exe" delete'
                        )
                if install_type == "Error":
                    return self.install_app_with_error(app_name, app_version, end)
                if (
                    "INSTALLING" in self.ae_installation_progress_bar_text
                    and cancel_button_verification
                    and not is_cancel_button_check
                ):
                    try:
                        self.ae_show_all_arrow.click()
                        ae_show_all_arrow_click_count = True
                        ae_installation_list_div_text = (
                            self.ae_installation_list_div.text.lower()
                        )
                        ae_installation_list_div_count = True
                        self.log.info(
                            "ae_installation_list_div_text for {0} is {1}".format(
                                app_name, ae_installation_list_div_text
                            )
                        )
                        assert (
                            "cancel" not in ae_installation_list_div_text
                        ), "Cancel button present while installing {}".format(
                            app_name
                        )
                        is_cancel_button_check = True
                    except (
                        AttributeError,
                        ElementNotInteractableException,
                        ElementClickInterceptedException,
                    ) as msg:
                        if not ae_show_all_arrow_click_count:
                            assert False, (
                                "Unable to locate arrow button on Exchange page hence unable "
                                "to check the cancel button (May be download and installation speed for"
                                " {} is too fast) during installation. Exception Msg: \n {}".format(
                                    app_name, msg
                                )
                            )
                        elif not ae_installation_list_div_count:
                            assert False, (
                                "Unable to locate installation div on Exchange page hence unable "
                                "to check the cancel button (May be download and installation speed for"
                                " {} is too fast) during installation. Exception Msg: \n {}".format(
                                    app_name, msg
                                )
                            )
                if (
                    "INSTALLING - REGISTER"
                    in self.ae_installation_progress_bar_text
                    and "unreal engine" in app_name.lower()
                ):
                    self.connector_unreal_engine_installation_additional_steps(app_name)
                elif (
                    "INSTALLING - REGISTER"
                    in self.ae_installation_progress_bar_text
                    and "kitware paraview" in app_name.lower()
                ):
                    self.connector_paraview_installation_additional_steps(
                        select_isv_app_path_for_install
                    )
            else:
                self.log.info(
                    "[ExchangeSearch] Finish {0} Installation Process ...".format(
                        app_name
                    )
                )
                is_installed = True
                break
        self.omniverse.screenshot("after_installing_{0}.png".format(app_name))
        if is_installed:
            self.log.info(
                "[ExchangeSearch] Success to install {0}".format(app_name)
            )
        else:
            self.log.info(
                "[ExchangeSearch] {0} is not installed properly after {1} minutes timeout".format(
                    app_name, timeout
                )
            )
            self.omniverse.screenshot("after_time_out_for_{}.png".format(app_name))
    self.close()
    return is_installed, app_version, app_name

Answer 1

You voiced several concerns:

1. Need to remove Nested Loops and
2. make code more compact and
3. readable.

I don't know what (1) is all about, since I only counted a single while loop. I don't know what (2) is about -- I imagine that any Extract Helper refactoring would expand to a few more lines of code rather than less code which is "compact". But perhaps by (2) your intent was "all functions should fit within a screenful of code", in which case I heartily agree. If you've written more than a hundred and twenty lines of code, without pausing to write another def, then you've definitely written Too Much. As far as (3) goes, yes, this illegible code would be far more readable if you did one or more Extract Helper refactors. And writing unit tests for those helpers would be very nice.

---

type hints

The signature is very nice, with clearly named identifiers. I thank you for it.

    select_isv_app_path_for_install: str = None,

Consider asking the caller to pass in a Path. It's just a more natural usage. It better reveals Author's Intent.

---

docstring

    """install app"""

Delete that. It isn't informative, it doesn't tell us anything new. Consider renaming to def install_app() if you want to spell that out more clearly.

Or replace it with an English sentence describing this method's behavior, as pep-8 asks you to do. Feel free to add a blank line and more explanation if that seems helpful.

If you cannot write a single English sentence describing what it does, well, that's a hint that maybe it does too much. In which case breaking out helpers will push the details down into several other documented methods, so you can address this one at a higher level.

---

elide trivial comments

    assert (
        self.open()
    ), "[ExchangeSearch] Failed to open app detailed card"  # open detailed card

The def open... review context is missing. But it's clear from the diagnostic what is going on here. The # comment doesn't add anything. Delete it.

Consider doing an explicit raise, so we needn't worry that $ python -O can disable asserts. Similarly for is_installing and other assert checks.

Consider having def open raise a fatal error itself, rather than returning a useless None result in the error case.

---

f-strings

    self.log.info("[ExchangeSearch] {0} version:{1}".format(app_name, app_version))

We see many {0} references to format parameters. Prefer this more modern usage: f"[ExchangeSearch] {app_name} version:{app_version}". It's more readable.

---

early return

    if self.is_installed:
        ... [two lines]
    else:
        ... [dozens of lines]

Run those two lines and do an early return, to save one level of indent for that giant else clause.

OIC, there's a self.close() detail to address at the end. Either handle it with a finally, or do Extract Helper on the else logic so it becomes a one-liner.

Consider using a with context manager to ensure the .close() happens.

    is_installed = False

It would have been simpler to assign self.is_installed to that local variable.

But, why do we need a local variable at all? Just to save typing five characters? Embrace self already!

---

magic number

        end = time.time() + timeout * 150

I have no idea what that magic number means, at an app level. Please give it a name, and consider writing a comment which ties its value back to your product requirements.

It seems to suggest that if I walk away from a failing install, in fifty minutes it will begin some surprising new behavior? How frightening.

---

cryptic identifiers

        ae_show_all_arrow_click_count = False
        ae_installation_list_div_count = False

Those are probably well named, and should be retained. But give me a # hint about what "ae" denotes, please.

EDIT

OIC, it seems to refer to "attribute error". Easily addressed by adding a one-line comment.

---

hacks

                        time.sleep(3)
                        os.system( ...

That magic number seems rather hacky. It deserves an # explanation of what it's trying to achieve, and why it works on all host configurations.

---

Ok, I can't bear to read any more of this.

Do not merge this code to main, as it is not maintainable.

In priority order, you need to

1. Read and understand what single responsibility is all about.
2. Extract one or more helper methods, each having a Single Responsibility which the method name and """docstring""" explain.
3. Write a unit test for each helper.

Answer 2

I think more sentiments to support the advice by @J_H would be :

---

Nesting if statements

You may like statement nesting If you are a beginner just like me, But after some time when you take more projects and get more familiar with clean and pythonic code, After you take a look at old projects, You realize just how unmaintainable they are.

Instead of nesting if statements, You should familiarize yourself with guard clauses technique :

• Using guard clauses, your code can find unwanted exceptions in a faster way.

• It makes your code Easy to understand and read.

• It affects your code reusability In a great way.

Normal code :

def return_number(number : int) -> int:
    """Returns the number given

    # Args
       - number (int): ...
    # Returns:
       - int: The same number
    """

    if type(number) is int:
        return number
    
    else:
        raise ValueError(...)

Guard clauses:

def return_number(number : int) -> int:
    """Returns the number given

    # Args
       - number (int): ...
    # Returns:
       - int: The same number
    """

    if not type(number) is int:
        raise ValueError(...)
    
    return number

Remember that even If you couldn't apply this technique In your code, Avoid Using If , If If ... , Use elif instead.

---

Comments and docstrings Usage

You might have heard from a lot of people that you Should not use comments and docstrings excessively .

Remember these:

• Comments are PURE EVIL.

• Documentation Is not used for explaining , It's used for describing .

• If someone Is not a programmer and can't read code , Then he has no reason to look at it.

• Even if your code Is bright like the sun , Use docstrings, In the proper way.

Now a question might pop into your mind : Why comments are pure evil and docstrings are not ? Don't they do the same thing ?

• When you use docstrings , They are bound to your module or method with the __doc__ method , Which means They actually have a place to be and anytime someone or your future self tried to use that module or method, it can be easily accessed using your editor or.. , So you don't have to go to the module definition In order to do that, you can access the docstring even using Notepad, Plus it might be good to know that even docstrings will be compiled to binary while running.

• Unlike docstrings, Comments are not bound to the code. They are ignored by the interpreter and the compiler. They won't be compiled to binary and they don't help anyone understand your code. You can't easily describe your code and it returns patterns in one line of comment. And adding more comments turns your code I Into a REAL MESS , So the only case that I think they are useful , Is when you want to remind yourself of something that need to be done later ( To do , Warning , ETC. )

---

Using parameters as changeable values ( Just like variables )

• Remember that having more than 4 parameters is not proper and it is a bad habit to adopt. In this case you can fix it by changing it to an arbitrary keyword argument which is way more clear.

• You can take all these variables and throw them into a DataClass at the top of your file or another module. This is a design pattern that I use myself and iIt is very useful. In this case you only had around 10 variables, but when you want to extend the functionality of your program, you may want to reset all the variables’ value back to False or None and that is pretty much annoying to do since you can't write a function for it and you've got to do all of the assignments yourself.

We're programmers; We've got to think lazy!

---

I Hope that was helpful; Thanks for your attention