3
\$\begingroup\$

I am new to programming and mostly self-taught by following a few MOOCs and doing little projects. I have reached the point where I am trying to develop more complicated programs (numerical models at work and games at home) and I would like to be able to give a structure to my programs to enhance readability.

I am following an Edx course called Systematic Program Design which offers methods on how to design functions, data and world in a consistent manner.

However, I have come to notice that the method seems to work mostly with functional programs rather than the object-oriented(ish) code I tend to write (especially for games). This course made me aware that my code was full of methods with almost exclusively side effects changing attributes. Which makes it hard to follow the method and makes the code look really chunky. I was wondering if anyone knows of a course/book on how to increase readability of OO code. I use Python 2.7.1.

As I said, I am self-taught, so I apologies if the answer seems trivial to some of you who have done CS.

EDIT:

The github link to my game is Far and Away v1

And this is the picture of my Game Design . The idea is that every object in the game has a game attribute linked to the Game() object which I use as a waypoint to access all objects in my game whilst keeping them in separate files.

EDIT END

To give you an example of what my code looks like and why I would like to have a method to improve it, here is one method of a game I've made:

    #Displays the map of planets and handles user interaction with the map
def view_solarsys(self,offset):
        planets_to_blit = []
        #checks for discovered planets and draws lines between them
        #as well as creating a list with them, that will be blitted to the screen
        for p in self.game.all_planets:
            if self.game.player.logbook[p.name].is_discovered == True:
                [pygame.draw.line(self.screen, (0,250,0), p.pos, p2.pos, 5) for p2 in p.planets_in_SOF if self.game.player.logbook[p2.name].is_explored and self.game.player.logbook[p.name].is_explored]
                planets_to_blit.append(p)
                #if mouse is over planet, the hoovered attribute is updated to the planet instance
                if p.rect.collidepoint(pygame.mouse.get_pos()) : self.hoovered = p
        #Blits discovered planets to screen. If they are not explored blits a red halo around them as well. If the player's location is on the planet blits a green halo around it.        
        for p in planets_to_blit:
            if self.game.player.logbook[p.name].is_discovered == True:
                if self.game.player.logbook[p.name].is_explored == False:
                    pygame.draw.circle(self.screen, (255,0,0), p.pos, int(p.rect.w*0.6), 0)
                if self.game.player.location == p.name:
                    pygame.draw.circle(self.screen, (0,255,0), p.pos, int(p.rect.w*0.75), 0)
                fn.blitc(self.screen, Data.images_planets[p.img_ref], p.pos)

            '''Mouse interaction'''
            if p.rect.collidepoint(pygame.mouse.get_pos()) and self.game.map_active:
                if pygame.mouse.get_pressed()[0]:
                    if self.game.player.logbook[p.name].is_explored == False:
                        p.explore(self.game.player)
                    else:
                        p.visit(self.game.player)
                elif pygame.mouse.get_pressed()[2] and self.game.pressed_right_clic == True:
                    p.search_in_SOF(self.game.player,True,30)
                    self.game.pressed_right_clic = False

            '''blitting planet info of hoovered planet'''
            self.view_planet()
\$\endgroup\$
  • \$\begingroup\$ Please consider if the first 4 paragraphs are really essential to your question. Seems 4 paragraphs just to basically state "I'm new to this, bear with me" is overdoing it. \$\endgroup\$ – boardrider Aug 19 '16 at 14:22
  • \$\begingroup\$ More code leads to better reviews, the code that you have here seems somewhat lacking, and so can be quite hard to review. \$\endgroup\$ – Peilonrayz Aug 19 '16 at 15:29
  • \$\begingroup\$ I agree with the other comments. For better advice, tell us less about you, and more about the program, and show more of your code. \$\endgroup\$ – 200_success Aug 20 '16 at 1:29
  • \$\begingroup\$ Hi. Sorry about that. On SO people tend to be allergic to large chunks of code that's why I only posted a sample. Plus I don't think putting a link to the github repo and asking the community to sort out the entire game would be fair. I will however update my post when I get back home. Especially on how I've designed the program. \$\endgroup\$ – Sorade Aug 20 '16 at 22:34
1
\$\begingroup\$

Your code is very procedural. Do this, do that, do another thing. There's nothing inherently wrong with this, but the only OOP in the code you've shown is that of pygame itself. There's a little bit hinted at by the game and player objects, and perhaps the logbook dictionary, which appears to hold some other kind of object. But the way you interact with all of these things is all very procedural.

So, how can you better leverage OOP?

The biggest change I would suggest is to avoid big long series of attribute accesses. Things like self.game.player.logbook[p.name].is_explored represent at best a misuse of OOP. What does this code try to do? It tries to answer if a player has explored a planet. How does it do so? By asking if the planet's name appears in the player's logbook, and if the resulting object has the right attribute set.

So how should this question look in code? It should mention as few of the other objects as possible. Ideally it would look like one of the following lines; choose one and stick with it:

player.has_explored(planet)
planet in player.explored_planets
planet.explored_by(player)
player in planet.explorers
game.planet_explored_by(planet, player)

Now, you might have to get your local planet or player variable from some other collection, but you don't want to have to know it's tracked in the game's player's logbook, indexed by planet's name, and stored in an an attribute called is_explored. Only the tiny function that answers that question should have to know any of that extra stuff, and then if you ever move the information, only that one function has to be updated.

Once you've chosen the new way to ask if a planet has been discovered and explored, update your code to use it. Then look for repetition, or otherwise unnecessary checks. They're distracting to read, and possibly slow your code down. For example, you currently create a list planets_to_blit that includes only planets that have been discovered. Then you iterate over this list to draw halos, but first check again whether the planet was discovered.

If you break the function into multiple pieces (say one helper that retrieves the planets, and another that draws halos), it's hard to know whether the check was performed before calling the second helper. However, instead of repeating the check as an if, consider adding it as an assert in the halo drawing helper. This documents to the reader of the code that the helper should only receive planets that have been discovered, and the resulting exception helps you to find callers that didn't make sure.

There are other small or cosmetic changes I would suggest:

  • Don't be explicit about comparing against True and False; prefer if x: ... or if not x: ...
  • Leverage some of the self-documenting features of python: in particular consider using docstrings instead of a comment before a function (on the flip side, consider avoiding the docstring-like string literals anywhere else in a function; use comments or extra functions if you need these)
  • Use whitespace a bit more liberally: tastes differ, but I like to group related lines into blocks and use empty lines before and after, and I like to have spaces after the # character
  • Be careful with spelling: in specific, hoover is a vacuum cleaner; hover is the term you're looking for

Now I want to circle back to your question about side effects. Once you've split up your code into smaller helper methods, a good split will often have some of the following properties for each method:

  • The method only relies on knowledge about one area - this is the first thing I talked about
  • The method will either only retrieve and filter data, or it will only modify data - for example, one method could retrieve all discovered planets, and another could draw lines for the planets that are passed to it
  • The method doesn't assume things that aren't in its name - for example a get_planets method wouldn't filter the results to just the discovered planets

If we look at your original code, the view_solarsys function violates all of these to some degree. It would be a lot nicer if it looked more like this:

def draw_solarsys(self):
    """Draw the discovered planets, and links between them."""

    discovered_planets = self.get_discovered_planets()
    for planet in discovered_planets:
        self.draw_SFO_for_planet(planet)

    mouse_pos = pygame.mouse.get_pos()

    for planet in discovered_planets:
        self.draw_planet_with_aura(planet)

        if planet.rect.collidepoint(mouse_pos):
            self.draw_planet_info(planet)
            if self.game.map_active:
                self.handle_clicks(planet)

Is that perfect? Of course not. But I think it helps.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the answer. I understand your idea. And I can see how it would improve my design. I actually have everything I need to do your 4th example. I however needed to run a lot of checks like explored by, discoveres by, discovery date, etc... and I wanted to have those related to a planet but specific to an explorer (one of which is the player). That's why I came up with the idea of a logbook dict attr for the explorers. With this extra information would you still recommend me adding attributes and methods specific to each test rather than centralising it all in the logbook ? \$\endgroup\$ – Sorade Aug 20 '16 at 22:29
  • \$\begingroup\$ It's certainly okay to collect all the data in one place. It may make sense to collect the methods in one place as well. But perhaps there should be a facade that hides the details of that storage, like in the last option game.planet_explored_by(planet, player), but perhaps on a different object. What you're striving for here is the principle of least knowledge. Dots aren't exactly the enemy, but the sheer number in your original code is a little suspect. \$\endgroup\$ – Michael Urman Aug 21 '16 at 11:52
  • \$\begingroup\$ Hi Michael, I've edited my question with a drawing of the game design and a link to the github repo. Just by looking at the drawing though, do you think I need a significant design overhaul ? \$\endgroup\$ – Sorade Aug 23 '16 at 10:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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