As a result of mrtazz’s post on Taking Pride in Your Code I thought that since I’m the maintainer of his simplenote.py code I better pull my finger out and bring that in line.

After adding in a Code Climate badge I discovered a few issues with cyclomatic complexity and code duplication.

I am quite lazy with code quality tools. I’ve used HLint for Haskell before because I found it made genuinely good suggestions. And I’ve been through various code quality tools with Java in the past, but those made much more dubious “improvements”; Ok, I’m not an expert programmer, but I still think code quality tools should be used as an aid, not as an enforced tollgate. Generally though I don’t use them and I probably should.

Code Climate uses radon for Python code. Radon is very easy to install locally and very easy to use which means you don’t have to publicly embarrass yourself by pushing your code to Github and having Code Climate do the work. You can quickly try out simple changes locally to see if it affects the code score. Or not as was the case with my first attempts.

At first I thought the issues were due to lots of nested if and try statements that (I thought) were necessary and (I thought) would be difficult to remove. I.e. sections of code like:

params = 'auth={0}&email={1}&length={2}'.format(self.get_token(), self.username, NOTE_FETCH_LENGTH)
if since is not None:
	try:
		sinceUT = time.mktime(datetime.datetime.strptime(since, "%Y-%m-%d").timetuple())
		params += '&since={0}'.format(sinceUT)
	except ValueError:
		pass

# ***perform initial HTTP request***
try:
	request = Request(INDX_URL+params)
	response = urllib2.urlopen(request)
	notes_index = json.loads(response.read().decode('utf-8'))
	notes["data"].extend(notes_index["data"])
except IOError:
	status = -1
# ***end initial HTTP request***

while "mark" in notes_index:
	params = 'auth={0}&email={1}&mark={2}&length={3}'.format(self.get_token(), self.username, notes_index["mark"], NOTE_FETCH_LENGTH)
	if since is not None:
		try:
			sinceUT = time.mktime(datetime.datetime.strptime(since, "%Y-%m-%d").timetuple())
			params += '&since=%s' % sinceUT
		except ValueError:
			pass

	# ***perform the actual HTTP request***
	try:
		request = Request(INDX_URL+params)
		response = urllib2.urlopen(request)
		notes_index = json.loads(response.read().decode('utf-8'))
		notes["data"].extend(notes_index["data"])
	except IOError:
		status = -1
	# ***end actual HTTP request***

There is obvious duplication here from performing the initial request and then the actual one, but both are actually needed. However, there is indication of some obvious maintainer apathy at play here as I hadn’t even noticed this bit until this review:

params += '&since={0}'.format(sinceUT)

and

params += '&since=%s' % sinceUT

As to how and why I had no consistency here I don’t know.

Anyway, with a bit of effort it was possible (who knew!?) to improve this and add a private method for the HTTP request:

def __get_notes(self, notes, params, since):

	notes_index = {}

	if since is not None:
		try:
			sinceUT = time.mktime(datetime.datetime.strptime(since, "%Y-%m-%d").timetuple())
			params += '&since={0}'.format(sinceUT)
		except ValueError:
			pass
	# perform HTTP request
	try:
		request = Request(INDX_URL+params)
		response = urllib2.urlopen(request)
		notes_index = json.loads(response.read().decode('utf-8'))
		notes["data"].extend(notes_index["data"])
		status = 0
	except IOError:
		status = -1
	if "mark" not in notes_index:
		self.mark = False
	return notes, status

And then the original bit of code ends up something just like this:

self.mark = True

while self.mark:
	notes, status = self.__get_notes(notes, params, since)

I wasn’t sure whether to use the instance variable approach (self.mark) or pass it as a result/argument of the method. I.e:

def __get_notes(self, notes, params, since, mark):

# code...

return notes, status, mark

Both approaches work. I think self.mark is cleaner, even if the instance variable doesn’t make much sense externally. I could have called it _mark to indicate it is private, but it doesn’t actually make any difference to how things work.

When I first got the Code Climate results I just assumed it was going to require me to do some “proper” programming and so I put it on the back-burner for awhile, but in the end, in this case anyway, the code duplication and the cylcomatic complexity were linked; Fixing one fixed the other. And it wasn’t as tricky as I thought.

I’m going to try to start using some code metric/quality/review tools a bit more. If only because they force a fresh look on code you have glossed over for ages.

See here and here for the relevant commits.

[EDIT: 2016-04-03] And here. Sigh.

• Tags:
  • program