6
\$\begingroup\$

I have fully functional code in class format, and as far as I am aware from my previous question, it conforms to PEP 8 and the code logic and implementation is suitably pythonic. My main concerns here is whether or not I have put self. in front of the correct variables, and some ambiguity surrounding my @staticmethod.

  1. Should I leave it as it is?
  2. Should I change the logic so it is not static?
  3. Should it be in the class at all?

# This imports some necessary libraries.
import webbrowser
import tempfile
import urllib.request
from tkinter import *


class Browser:
    """This creates a relay that allows a user to directly view data sent from a web server."""
    def __init__(self, master):
        """Sets up a browsing session."""
        # Explicit global declarations are used to allow certain variable to be used in all methods.
        global e1

        # Here we create some temporary settings that allow us to create a client that ignores proxy settings.
        self.proxy_handler = urllib.request.ProxyHandler(proxies=None)
        self.opener = urllib.request.build_opener(self.proxy_handler)

        # This sets up components for the GUI.
        Label(master, text='Full Path').grid(row=0)
        e1 = Entry(master)
        e1.grid(row=0, column=1)
        Button(master, text='Go', command=self.browse).grid(row=0, column=2)

        # This binds the return key to self.browse as an alternative to clicking the button.
        root.bind('<Return>', self.browse)

    @staticmethod
    def parsed(data):
        """Cleans up the data so the file can be easily processed by the browser."""
        # This removes removes all python-added special characters such as b'' and '\\n' to create understandable HTML.
        initial = str(data)[2:-1]
        lines = initial.split('\\n')
        return lines

    def navigate(self, query):
        """Gets raw data from the queried server, ready to be processed."""
        # This gets the opener to query our request, and submit the response to be parsed.
        response = self.opener.open(query)
        html = response.read()
        return html

    def browse(self):
        """Wraps all functionality together for data reading and writing."""
        # This inputs and outputs the necessary website data from user-specified parameters.
        raw_data = self.navigate(e1.get())
        clean_data = self.parsed(raw_data)

        # This creates a temporary file in which we store our HTML data, and open it in the default browser.
        with tempfile.NamedTemporaryFile(suffix='.html', delete=False) as cache:
            cache.writelines(line.encode('UTF-8') for line in clean_data)
            webbrowser.open_new_tab(cache.name)

# Creates a Tk() window that is always in front of all other windows.
root = Tk()
root.wm_attributes('-topmost', 1)

# Starts the program by initializing the Browser object and main-looping the Tk() window.
anon = Browser(root)
root.mainloop()
\$\endgroup\$
5
  • 2
    \$\begingroup\$ "Explicit global declarations are used to allow certain variable to be used in multiple methods." Huh? Instance attributes, eg. self.e1 can be used in all methods. \$\endgroup\$ Feb 19, 2014 at 18:43
  • \$\begingroup\$ This code is not actually PEP8-compliant: it has long lines that mean we have to scroll horizontally to read it here. \$\endgroup\$ Feb 19, 2014 at 19:19
  • \$\begingroup\$ @Gareth Rees PEP 8 says a maximum of 79 characters. My code fulfils that condition. \$\endgroup\$ Feb 19, 2014 at 19:22
  • 1
    \$\begingroup\$ @anongeneric: How are you counting? I find that line 31 has 119 characters, for example. (If you can get it within the PEP8 limits, you'll find that there's no need to scroll it here.) \$\endgroup\$ Feb 19, 2014 at 19:30
  • \$\begingroup\$ @Gareth Rees ok you're right, so there's that. Pycharm was using 120 as the limit for some reason.What about the actual concerns I mentioned? \$\endgroup\$ Feb 19, 2014 at 19:37

1 Answer 1

10
\$\begingroup\$

Class Scoping

So your comment afore your global declaration of e1 mentions how you wish for e1 to be accessible in all methods. If you want a variable to be accessible in all methods of a class, the conventional way is to assign it as a property of self:

class Foo():
     def __init__(self, bar):
         self.boz = bar * 3

     def baz(self):
         # boz is now accessible here
         print(self.boz)

Note that boz is not a global; if I create two Foo() objects, I'll end up with 2 different boz variables:

>>> f1 = Foo(3)
>>> f1.boz
9
>>> f2 = Foo(2)
>>> f2.boz
6
>>> f1.boz
9 # Still

Static Methods

To answer your (most conceptual) questions about the static method:

  1. Potentially
  2. The function takes some data, and then performs an operation on it. It does not rely on any other state to decide what operation to perform, so it is a perfect candidate for being static.
  3. No, it does not need to be static; a static method is (conceptually) a function in all but name, so you could write it as a function. The main reasons to have something be a static variable is usually organizational or conceptual, so if you think that applies here, keep it as a staticmethod. Otherwise, consider making it a bog standard function.

There is no definite rule for when to write a function and when to write a static method; it's something you learn over time. The two questions I ask myself when deciding to write a static method are "Could I write this as a function?" and "Is this code very tightly linked (conceptually) to the class?". If the answer to both of those questions is yes, then I might write it as a static method.

In this case, I think a function might be more useful.

General bugs

Just some little things I noticed:

  • Your parsed static method says that it removes cruft from a string, but it never checks to see if the characters it expects to be there actually exist. Personally, I would use a regexp to implement this function, but regardless, it might be wise to check that the characters you are deleting are the ones you expect to be there.
  • In __init__ you do root.bind(...), where you could do master.bind(...), which might be better, as it would avoid using a global variable.
  • I'd recommend using a new style class by replacing class Browser: with class Browser(object):. The benefits of the syntax difference are subtle, but noticeable (and it helps with forward compatibility iirc).
  • It might be a better idea to define a main function, and then call that instead of just doing loads of stuff in the global scope. You could also add a guard case of the form if __name__ == "__main__": main() to allow you to import the script into a REPL without running the code.
  • Some of your variable names kinda suck. Try and find more descriptive names than e1!
\$\endgroup\$
2
  • \$\begingroup\$ so would the main() function just go around the stuff after the class is finished? And what is a regexp and how would I implement it? \$\endgroup\$ Feb 20, 2014 at 9:52
  • \$\begingroup\$ So you'd have something like this for your main. A regexp is short for a regular expression, an incredibly powerful string parsing/extraction DSL (domain specific language). The Python docs have a tutorial on them. \$\endgroup\$ Feb 20, 2014 at 10:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.