{"comment":{"url":"https://api.github.com/repos/iPlantCollaborativeOpenSource/atmosphere/comments/16158122","html_url":"https://github.com/iPlantCollaborativeOpenSource/atmosphere/commit/17668ea10a728e7023ed0dafa4fd6cc04c6fb93d#commitcomment-16158122","id":16158122,"user":{"login":"lenards","id":5923,"avatar_url":"https://avatars.githubusercontent.com/u/5923?v=3","gravatar_id":"","url":"https://api.github.com/users/lenards","html_url":"https://github.com/lenards","followers_url":"https://api.github.com/users/lenards/followers","following_url":"https://api.github.com/users/lenards/following{/other_user}","gists_url":"https://api.github.com/users/lenards/gists{/gist_id}","starred_url":"https://api.github.com/users/lenards/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lenards/subscriptions","organizations_url":"https://api.github.com/users/lenards/orgs","repos_url":"https://api.github.com/users/lenards/repos","events_url":"https://api.github.com/users/lenards/events{/privacy}","received_events_url":"https://api.github.com/users/lenards/received_events","type":"User","site_admin":false},"position":149,"line":610,"path":"service/tasks/monitoring.py","commit_id":"17668ea10a728e7023ed0dafa4fd6cc04c6fb93d","created_at":"2016-02-17T20:18:40Z","updated_at":"2016-02-17T20:18:40Z","body":"This is not important for getting this PR completed. But one thing you can do to promote _aspects_ of inversion (mainly the idea of [DIP](https://en.wikipedia.org/wiki/Dependency_inversion_principle), dependency inversion) is to make it so the counting is done based on `apps_without_version` passed in. \r\n\r\nThis makes some people _rage_ because do this, you end up with a pattern of \"two\" methods - one that does the logic (here, _counting_) and a _wrapper_ method that does the query and passes it along. \r\n\r\nSo, to sketch it out a bit ... \r\n\r\n```python\r\ndef _remove_applications_without_versions(now=None):\r\n    if not now:\r\n       now = timezone.now()\r\n   apps_without_versions = Application.objects.filter(versions__isnull=True, end_date__isnull=True)\r\n   return _remove_applications_without_versions_impl(apps_without_versions, now)\r\n\r\ndef _remove_applications_without_versions_impl(apps_without_versions, now):\r\n    app_count = 0\r\n    for app in apps_without_versions:\r\n        app.end_date_all(now)\r\n        app_count += 1\r\n    return app_count\r\n```\r\n\r\nWith this, we've inverted the dependency one database. The _other_, wrapper method needs it. But the function contain the logic _does **not**_. We can write a unit test on `_remove_applications_without_versions_impl` that just creates use to pass in an _iterable_ that contains a model (or an instance that `hasattr` of `end_date_all`). \r\n\r\nAgain - this is just something to note as we review code. "}}