Your Preferred Python Writing Style - Followup

Game Engine Game Engine Support and Discussion
1

I was sifting through the dictionary thread wherein the discussion revolved around readability and overall neatness of code.

I’ve always been trying to write with the PEP8 style, however, looking back at my older pieces of code, I realized that I don’t have a definite style. My code is a mixture of my own style and others’ (PEP8 and BA python programmers). In other words, my code doesn’t look pretty. I am here in hopes of refining my writing style to improve on the readability of my code. Hopefully other users will take this as an opportunity to improve their code as well because, my goodness, some people really need to learn to write legibly… even more than I do.

Anyways, I would like some first hand tips on naming variables, methods, classes, etc.
How should I space my lines?
When should I make methods to encapsulate code and why?

I have an answer to all of these things but it’d be great to have confirmations and discussions about them as well.

Here’s an example of m most recent code:
http://www.pasteall.org/43812/python

2

I used to write in my own style (a mix of camel case and some other oddities), but after going through a uni programing course for python, I now write in PEP008 pretty consistently.

If you need something to check your style (though you would wish to kill it), have a look for pylint. It’ll tell you where you’ve made style mistakes.

3

I feel like what’s more important than any particular style is to be consistent. As long as you use a consistent, clear naming convention throughout your code, any competent programmer should be able to pick up on what you’re doing.

Of course, it’s still preferable to use more widely accepted styles like PEP8. I just don’t think it’s as important as consistency.

4

Mobious i’m 100% with you :smiley:

BTW: I prefer “Java” style convention which is basicaly PEP008, but I’m used to other conventions too. It is important that you can differenciate between Classes and instances. The same belongs to functions and variables.

When to encapsulate code is a different (and complex topic). You can find information about that in guides to OOP principles, Pattern and Clean Code.

5

I think that “style” can be split into two distinct categories:

Visual clarity - Mainly about how things look, and that’s what PEP8 is all about: underscore_syntax vs camelCase, tabs vs spaces, import patterns, max line length, and so on. I agree with most of what PEP8 suggests, and I try to follow the segments I agree with, but in the great scheme of things, it’s really not that important when compared to the second category:

Linguistic elegance - This is the difficult part, because it’s all about what you write, and whether it makes clear sense in the given context. Visual clarity (PEP8) is about being able to read the words; Linguistic elegance is about being able to easily understand them. This category is truly the core of the programming discipline, because it touches on virtually everything: What you write depends heavily on the abstractions you devise, and they in turn define the overall architecture of the system as a whole, which then affects how you think (and therefore write) within it.

It’s truly a “Zen” kind of thing, and that’s probably why it has no “PEP8” equivalent - it’s almost ephemeral (although, that doesn’t stop people from writing books about the subject … but it’s still very hard to quantify good taste).

Anyway, here’s a few quick tips:

Avoid words that don’t add relevant meaning:

In the code you linked to, “TheSoundMachine” is the most egregious violation of this principle. “The” adds nothing to the description, and “Machine” is equally pointless. Virtually anything can be thought of as a “Machine”, but that doesn’t really tell us anything about what it does. “SoundManager” would be far more descriptive.

Names should describe fully:

“SoundManger” seems like a fine name, but looking at the implementation, we can see that it doesn’t just manage sounds. There is a clear distinction between sounds and music, and it seems like they’re both managed within this one class. So, we should think of a name that would encompass both. I think “AudioManager” would fit better in that case.

Avoid redundant labeling and needless variables:

The most obvious example in your code: “PATH_MUSIC = PATHS.PATH_MUSIC”. The module name implies that it’s a path, so you can just use “PATHS.MUSIC”.

Split overly-long lines into shorter expressions:

Look at line 167 …

Don’t repeat yourself, and avoid needless indirection:

Look at “playMusic” and “handle_music”: Pretty obvious red flag, don’t you think?

From a first look, I would say that “handle_music” is completely unnecessary; “playMusic” should create and play the handle, and “setMusicVolume” can simply set the volume on the handle, without going through VOLUME_BGMUSIC (no need for indirection there).

A function with more than 3 arguments is too complex:

“playOnFrame” …

Learn the alternatives, so you can choose better:

This:


dict = sound.sound_dict
factory = dict[random.choice(list(dict.keys()))]

Is better written like this:


factories = list(sound.sound_dict.values())
factory = random.choice(factories)

Avoid deep nesting, along with overly-long/messy functions:

“handle_sounds” … I think it’s incredibly convoluted, but I can’t really offer any concrete advice, because I honestly don’t know what you’re trying to do with that code.

A lot of it just seems unnecessary: If you want to play a sound, just generate a handle and play it. If you want to play a sound based on some animation frame, do the same thing, when the frame is played.

It seems like far too much management for something that should be relatively straightforward, so I would recommend rewriting the whole class from scratch, but with a better picture of what you really need, and how simple it could actually be.

Remember the KISS principle.

6

Thank you all for the opinions and ideas! And thanks for all the feedback Goran. You actually looked at the code, much appreciated haha
I’ll be updating the code, and post here again… (:

7

I haven’t seen the link to code ;).

I confirm with goran’s notes.

These are my hints:

The important things (high level abstraction) should be mentioned first. E.g. your “main” is the highest abstraction, but is written as last! paragraph. ( I guess you already know my opinion regarding the term “main” :wink: -> and “always()” )

The code repeats the expression “self.current_bgmusic[“handle”]” incredibly often. There are several option to solve this:

A) store the value in a variable within the first usage e.g. handle = self.current_bgmusic[“handle”] (whatever handle means ;). )
B) store the value in an instance variable e.g. self.handle = self.current_bgmusic[“handle”]
C) use a function to get the handle (in combination with A) e.g. handle = getHandle() # or self.getHandle()
…

It seams your class plays music and manages bgmusic. These are two different things resulting in complex code. You can separate this into two classes. One that plays music and one that manages bgmusic. The one that plays music can get the bgmusic from the one that manages it. You partly do that already but bgmusic is “just” a dictionary and the “player class” needs to know the details of the bgemusic.

It seams dealing with music and dealing with sounds is pretty much the same. I suggest to look if you can manage them into one class. If necessary you can deal with the differences in subclasses.

I like the recommendation (from clean code) to start function names with verbs. This way you know what it is doing.
e.g.
playMusic vs. playingMusic. The first one is easily to guess … it plays music. The second one is … the same? If you add an “is” it is much easier to recognize as a question: isPlayingMusic()?

Does playMusic really play music? It looks more like it switches bgmusic to the next song. Maybe “setNextSong” fits better?

It seams “handle_music” is actually playing the music. So the name playMusic() fits better in this situation.

Why are the instance variables partly upper case and partly lower case?

You do not need to store the one and only instance of the class in bge.logic. Just hold it in the module itself


soundMachine = SoundMachine()
def play():
   soundMachine.play()

Hint: if you place the class definition in a separate module, you can concentrate the bge callable function in a very short and simple module. This allows you to define the class first (as you just import it) -> high level first.

Does “aud.Factory.file()” really return a factory as your dictionary key mentions? or is it a file

just some thoughts

8

Thank you Monster, I’ll take those things into consideration!
Specifics:

It seams “handle_music” is actually playing the music. So the name playMusic() fits better in this situation.

Why are the instance variables partly upper case and partly lower case?

This was just a mistake, I don’t know why I suddenly changed the names of methods.

You do not need to store the one and only instance of the class in bge.logic.

In my actual .blend I use the manager outside this script, which is why I put the object into a global variable.

Does “aud.Factory.file()” really return a factory as your dictionary key mentions? or is it a file

Yes, it returns a factory.
http://www.blender.org/documentation/blender_python_api_2_62_0/aud.html#aud.Factory

@Goran:
Before I start rewriting, I’m trying figure out how my “handle_sounds” is too complex. I honestly don’t see the problem. I need to run this method every frame to check whether or not a sound is playing an animation frame.

Here’s the code, I’ll explain through comments:

http://www.pasteall.org/43874/python (Just the “handle_sounds” method)

(The comments are messy on pasteall.org, I suggest copy and pasting the code into an editor)

Here’s the sound.py object, maybe that will help a bit.

See, I’m not sure at all how to make this more simple. If I have to run this method every frame, I need to make sure duplicate sounds aren’t playing.

9

I suggest to simply create an instance in this module it. This would be the natural place for such an instance. You can always access this instance by importing.

e.g.


from musicPlayer import soundMachine

Strange, factories are usually something else. But I see there is not much choice.

10

Bump; I still want to discuss :confused:

11

No, I understood what the code is literally doing, but it’s not easy to understand what you’re trying to do in general; What are the general specifications of this sound system?

Let’s imagine that you hired some other programmer to write the system for you: Describe what the system needs to do.

12

I want to have:

  • a central place to play sound effects and background music; to have control of all the sounds currently playing in the game.
  • a way to play a random sound effect from a list once.
  • a way to binds a random sound effect to an animation’s frames; a random sound from a list is played when the animation passes a frame in the frame list. Constantly calling this method acts like a “loop end” animation playing type.
  • a way to play background music

That’s basically what I need the system to do.

13

My suggestion:

(audio is music or sound effect)
Objects an their purpose

audio provider) provides a collection of available audios
audio player) plays ONE audio
audio selector) selects one audio from a collection of audios

You need one audio provider (singleton) to provide all audio data. Alternative you can provide one audio manager per audio type (music/sound effect).

You can determine one audio player to play the background music (I assume you want to have one at the time).
The audio player can get its audio from a random background music selector (audio selector that selects background randomly).

You can have as many audio players to play sound effects as you like.
The audio player can get its audio from a random sound effect music selector (audio selector that selects sound effects randomly).

how you can do this (Pseudo code):


class AudioProvider()
  getAudio()

class AudioSelector():
  setAvailableAudios()
  selectSound()

class AudioPlayer():
  setAudioSelector()
  setAudio()
  play()
  stop()
  next()

additional Audio can have some characteristics:


class Audio():
  def __init__():
   self.repeat = False
   self...

I hope you get the idea.

14

Let’s focus only on this (for now):

I think you should wrap aud.Factory:


class Factory:
    def __init__(self, path_to_audio):
        self.aud_fac = aud.Factory(path_to_audio)
        # ...
    
    def play(self):
        # ...

    # ... other control methods

audio = logic.audio = {"background": Factory(path_to_background)}

audio["background"].play()

So, audio can start as nothing more than a dictionary, with your custom factory objects, which wrap aud.Factory. Those objects can hold your control methods.

Try to make that first, as best you can, and then we can build up from there.

15

Cool, here’s what I have so far:


class Factory:
	def __init__(self, path_to_audio):
		self.aud_factory = aud.Factory(path_to_audio)
		self.factory_handle = None
		
	def play(self):
		device = aud.device()
		self.factory_handle = device.play(self.aud_factory)
	
	def is_playing(self):
		if self.factory_handle:
			if self.factory_handle.status:
				return True
		return False
	
	def stop(self):
		if self.is_playing():
			self.factory_handle.stop()
	
	def pause(self):
		if self.is_playing():
			self.factory_handle.pause()
	
	def resume(self):
		if self.is_playing():
			self.factory_handle.resume()
16

as style the spaces(of the first link i mean) here completely wrong. (i guess break basic guide of PEP)
def something(self,volume = 1):
def something(self, volume=1):

see the style of this, also if the code is complex and long, still readable
http://projects.blender.org/scm/viewvc.php/branches/soc-2013-bge/release/scripts/startup/bl_operators/object.py?root=bf-blender&r1=58161&r2=58160&pathrev=58161&diff_format=s

note the heavy usage of local variable and where here used the empty lines

17

Why does a factory play sounds?

A factory should create things. This is a common pattern.

To avoid confusion do not use this name for other purposes. I think it is already incorrect in the aud API. Even if they internally use a factory external it is something else.

Better name it Player or so. Because this is what you expect it to do.

18

Because playing a sound implies creating a new instance of a sound.

It makes perfect sense within the audaspace context.

@Linkxgl

Instead of “factory_handle”, you should just use “handle”; it’s on a factory, so that part is implied.

This repetition (where you’re basically making methods that already exist):


    def stop(self):
        if self.is_playing():
            self.factory_handle.stop()
    
    def pause(self):
        if self.is_playing():
            self.factory_handle.pause()
    
    def resume(self):
        if self.is_playing():
            self.factory_handle.resume()

Is a sure signal that you should just use the handle directly.

If there is some subsystem that, for some reason, uses the handle before one is initially created, you can create the default handle at initialization:


self.play()
self.handle.stop()

As long as the handle exists, its methods can be called regardless of status, making the “handle.status” check unnecessary, along with the is_playing method.

So, simplify based on that information.

19

Yeah, I was thinking the repetition wasn’t needed, I wasn’t sure how to bypass it.


class Factory:
	def __init__(self, path_to_audio):
		self.aud_factory = aud.Factory(path_to_audio)
		self.handle = None
		self.play()
		self.stop() # Wouldn't this also work instead of this? : self.handle.stop()
		
	def play(self):
		device = aud.device()
		self.handle = device.play(self.aud_factory)
	
	def stop(self):
		self.handle.stop()
	
	def pause(self):
		self.handle.pause()
	
	def resume(self):
		self.handle.resume()
20

self.stop() shouldn’t exist. :slight_smile:

I mean, look at this:


    def stop(self):
        self.handle.stop()
    
    def pause(self):
        self.handle.pause()
    
    def resume(self):
        self.handle.resume()

Pretty pointless, don’t you think? The handle carries those methods already, and the handle actually represents control for the currently playing instance, so it’s only natural to use it directly, like so:


audio["name"].handle.pause()

Calling pause on a factory doesn’t really make sense, because a factory is there only to produce instances of the given sound; A handle is used to control a particular instance.

Or, at least, this is how I reason about it.

If we simply lean on the abstractions provided by audaspace, the class simplifies to this:


class Factory:
    def __init__(self, path_to_audio):
        self.aud_factory = aud.Factory(path_to_audio)
        self.play()
        self.handle.stop()
        
    def play(self):
        device = aud.device()
        self.handle = device.play(self.aud_factory)

Now, before we get to the “random sound from a list” and “animation based sound”, is there any other capability that you need to have? If you do, try to build it from here, and we’ll see if/how it can be improved.