HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Identify audio using Gracenote

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
usingidentifyaudiogracenote

Problem

This script records audio from the computer's output and passes it to a C program that fingerprints it and queries the Gracenote database. If it identifies the audio then it offers options to look it up on Discogs. I made this because I wanted a way to identify tracks in DJ mixes that didn't involve using Shazam on my phone.

Running the script requires setting up audio devices using Soundflower and installing SwitchAudioSource to programmatically switch device (details here).

I am particularly interested in comments on the design. I think the most significant problem is that the main() function is overly dense. I decided not to use a more object-oriented style because I wasn't sure of the benefit over using a few functions. If I were to do more with either Discogs or Gracenote I would probably use the Python API clients they both provide, so rather than recreate a partial implementation of them I just used several functions.

Would an OO-style have been preferable here?

The functions could easily be split into modules as they have quite distinct tasks. Does the length of this script justify breaking it into separate modules?

This is also the first time I have written something using .rc files and command-line flags (most of my experience is JS in the browser) so I am interested in views on whether I have used them sensibly.

```
#!/usr/bin/python

import wave
import time
import subprocess
import os.path
import json
import argparse
import webbrowser
import sys
import requests
import keyring
import pyaudio
from rauth import OAuth1Service

# ---------------- Config -----------------

CHUNK = 1024
FORMAT = pyaudio.paInt16
CHANNELS = 2
RATE = 44100
RECORD_SECONDS = 6
SAVE_PATH = os.path.expanduser("~") + "/Music/recordings/"
WAVE_OUTPUT_FILENAME = "temp_{}.wav".format(int(time.time()))
COMPLETE_NAME = os.path.join(SAVE_PATH, WAVE_OUTPUT_FILENAME)
CONFIG_PATH = os.path.expanduser("~") + "/.identifyaudiorc"

def load_user_config(path):
user_config = {}
try:

Solution

To OO or to not to OO

Your program is fairly small and well organized and easy to read.
I don't see any reason at this point to make it "more OO".
If in the future you write more programs like this you'll want to
organize the code into libraries to re-use common code. But at
this point what you have is perfectly fine.

Now on to some stylistic comments...

Global variables

You have two kinds of global variables:

  • constants like RECORD_LENGTH, CHUNK



  • values which invoke code and could vary from


run to run, e.g. WAVE_OUTPUT_FILENAME, CONFIG_PATH

  • values which are initialized to objects, e.g. p


and discogs

First of all as globals the p and discogs variables
should have ALL CAPS names (and p should be given a
more descriptive name -- it is even used?) This will
make it obvious to the reader that they are globals.

Secondly, I would move the initialization of
the path name globals to main:

def main():
    user_home = os.path.expanduser("~")
    config_path = os.path.join(user_home, ".identifyaudiorc")
    save_path = os.path.join(user_home, "/Music/recordings/")
    complete_name = os.path.join(save_path, "temp_{}.wav".format(int(time.time())))
    ...


In fact, none of these values need to be global since
main passes them to whatever function needs them - and that's
a good aspect of your code!

As an enhancement, consider adding command line options to set
these paths - I think being able to set the location of the recordings
directory would be convenient.

load_user_config()

What you have is fine, but be aware that there are tons of python libraries
to load (and save) configuration files. Just google, e.g., "python load config file".

long if-blocks

main has a very long spanning if-block, and to make the
logic more apparent I would organize it like this:

def main():
  ...
  if not subprocess.call(["SwitchAudioSource", ...):
    raise RuntimeError("Couldn't switch to multi-output device.")
  ...


Now the reader can easily see what happens when the call to SwitchAudioSource fails
without having to search for the matching else clause.

Another long if-block is the if match: statement. It would be more
readable if it was written:

if not match:
    return
  print json.dumps(resp["re...


In fact, now it is obvious that you aren't doing anything if
there isn't a match. Perhaps you want something like:

if not match:
    raise RuntimeError("Unable to read audio.")
  ...

Code Snippets

def main():
    user_home = os.path.expanduser("~")
    config_path = os.path.join(user_home, ".identifyaudiorc")
    save_path = os.path.join(user_home, "/Music/recordings/")
    complete_name = os.path.join(save_path, "temp_{}.wav".format(int(time.time())))
    ...
def main():
  ...
  if not subprocess.call(["SwitchAudioSource", ...):
    raise RuntimeError("Couldn't switch to multi-output device.")
  ...
if not match:
    return
  print json.dumps(resp["re...
if not match:
    raise RuntimeError("Unable to read audio.")
  ...

Context

StackExchange Code Review Q#128528, answer score: 2

Revisions (0)

No revisions yet.