Drawing a planetary system in Pygame 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  . 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()
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.