Code Reviews Example 2 Sandi Metz PatentJob - herougo/SoftwareEngineerKnowledgeRepository GitHub Wiki

Sandi Metz video with timestamp: https://youtu.be/v-2yFMzxqwU?t=1007

Mentioned in "OOP is Embarassing": https://www.youtube.com/watch?v=IRTfhkiAqPw

Original Code (Adapted to Python)

class PatentJob:
  def run(self):
    temp = self._download_file()
    rows = self._parse(temp)
    self._update_patents(rows)

  def _download_file(self):
    raise NotImplementedError()

  def _parse(self):
    raise NotImplementedError()

  def _update_patents(self):
    raise NotImplementedError()

Criticism (by Sandi Metz)

• about the code
  • what if the ftp host/login/password changes?
  • what if I need to create another job like this?
• about the test
  • What if I don't want to ftp a file in every test (i.e. should be mockable)?

Praise

• short
• organizes the code in a class

My Version (Similar to Sandi Metz's version)

class Config:
  def __init__(self, opts):
    self.data = self._parse_opts(opts)

  def _parse_opts(self, opts):
    raise NotImplementedError()

class FtpDownloader:
  def __init__(self, config):
    self._config = config

  def download_file(self):
    raise NotImplementedError()

class PatentJob:
  def __init__(self, downloader):
    temp = downloader.download_file()
    rows = self._parse(temp)
    self._update_patents(rows)

  def _parse(self):
    raise NotImplementedError()

  def _update_patents(self):
    raise NotImplementedError()
    
def main():
  opts = {}
  config = Config(opts)
  downloader = FtpDownloader(config)
  job = PatentJob(downloader)
  job.run()

Praise

• downloader is mockable

Concluding Comments

I think it depends on your use case. If you're only making a simple job, the first example works. However, the 2nd example is not as bad as the guy in the other video said.