{"action":"created","comment":{"url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/pulls/comments/72499297","id":72499297,"diff_hunk":"@@ -8,113 +8,160 @@\n import pandas as pd\n import numpy as np\n \n+# aea: argparse here\n input_file = 'Real Estate Model - Candler Park.csv'","path":"RealEstateDataCleaning.py","position":5,"original_position":5,"commit_id":"5d9c0d80ba17da734ef72308a411f2e6e322cacb","original_commit_id":"5d9c0d80ba17da734ef72308a411f2e6e322cacb","user":{"login":"hkz-aarvesen","id":4959721,"avatar_url":"https://avatars.githubusercontent.com/u/4959721?v=3","gravatar_id":"","url":"https://api.github.com/users/hkz-aarvesen","html_url":"https://github.com/hkz-aarvesen","followers_url":"https://api.github.com/users/hkz-aarvesen/followers","following_url":"https://api.github.com/users/hkz-aarvesen/following{/other_user}","gists_url":"https://api.github.com/users/hkz-aarvesen/gists{/gist_id}","starred_url":"https://api.github.com/users/hkz-aarvesen/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/hkz-aarvesen/subscriptions","organizations_url":"https://api.github.com/users/hkz-aarvesen/orgs","repos_url":"https://api.github.com/users/hkz-aarvesen/repos","events_url":"https://api.github.com/users/hkz-aarvesen/events{/privacy}","received_events_url":"https://api.github.com/users/hkz-aarvesen/received_events","type":"User","site_admin":false},"body":"The instinct to put `input_file` and `output_file` into variables is a good one.  The next step up here is to use `argparse` to pass those variables on the command line.  I'd personally refactor this to something like:\r\n\r\n```\r\nif __name__ == '__main__':\r\n    import argparse\r\n    parser = argparse.ArgumentParser(usage=\"Process house data\")\r\n    parser.add_argument(\"-i\", dest=\"input_file\",\r\n                        help=\"input csv file\", type=str, \r\n                        default=\"Real Estate Model - Candler Park.csv\")\r\n    args = parser.parse_args()\r\n    data_frame = pd.read_csv(args.input_file)\r\n```","created_at":"2016-07-27T18:46:21Z","updated_at":"2016-07-27T18:46:21Z","html_url":"https://github.com/amazingvince/Real-Estate-Data-Cleaner/pull/1#discussion_r72499297","pull_request_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/pulls/1","_links":{"self":{"href":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/pulls/comments/72499297"},"html":{"href":"https://github.com/amazingvince/Real-Estate-Data-Cleaner/pull/1#discussion_r72499297"},"pull_request":{"href":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/pulls/1"}}},"pull_request":{"url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/pulls/1","id":79075404,"html_url":"https://github.com/amazingvince/Real-Estate-Data-Cleaner/pull/1","diff_url":"https://github.com/amazingvince/Real-Estate-Data-Cleaner/pull/1.diff","patch_url":"https://github.com/amazingvince/Real-Estate-Data-Cleaner/pull/1.patch","issue_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/issues/1","number":1,"state":"open","locked":false,"title":"Several suggestions","user":{"login":"hkz-aarvesen","id":4959721,"avatar_url":"https://avatars.githubusercontent.com/u/4959721?v=3","gravatar_id":"","url":"https://api.github.com/users/hkz-aarvesen","html_url":"https://github.com/hkz-aarvesen","followers_url":"https://api.github.com/users/hkz-aarvesen/followers","following_url":"https://api.github.com/users/hkz-aarvesen/following{/other_user}","gists_url":"https://api.github.com/users/hkz-aarvesen/gists{/gist_id}","starred_url":"https://api.github.com/users/hkz-aarvesen/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/hkz-aarvesen/subscriptions","organizations_url":"https://api.github.com/users/hkz-aarvesen/orgs","repos_url":"https://api.github.com/users/hkz-aarvesen/repos","events_url":"https://api.github.com/users/hkz-aarvesen/events{/privacy}","received_events_url":"https://api.github.com/users/hkz-aarvesen/received_events","type":"User","site_admin":false},"body":"Hey Vince,\r\n\r\nI made several changes and suggestions.  I'll put greater detail in the code.\r\n\r\nSome top level suggestions:\r\n\r\n- python is sensitive to formatting, and pythonistas are even more sensitive.  Take a gander through `PEP8` to see what all they want you to do.  I applied some minor \"fixes\" to the code (such as replacing tabs with spaces) to bring it in line with PEP8.  This is an annoying feature of python development but, hey, you will encounter it.\r\n- I added a unit test as an example.\r\n- I added the barebones necessary for a virtual environments.  Venvs are a great feature in python.  They let you avoid polluting your work machine with a bunch of junk you need for just one projet.\r\n- Added a `.gitignore` to avoid checking in python cache files.\r\n\r\nYou asked specifically about methods.  I assume you're also talking about the functions you're using here.  Proper decomposition is one of the hardest things to get right, so do not despair!\r\n\r\nFunctions do a few main things for you.  One is that they package up repeatable code.  This reduces defects since it's very common to make slight mistakes when copying code.  The second is that they allow us limited humans to comprehend what the code is trying to do.  A thousand line function is hard to understand; a function split up into something like\r\n\r\n```\r\nfilename = get_dictionary_filename()\r\nwords = read_dictionary_from_disk(filename)\r\nresults = []\r\nfor word in words:\r\n    if is_palindrome(word):\r\n        results.append(word)\r\nreturn results\r\n```\r\n\r\nis pretty clean on what it is doing.\r\n\r\nA third thing, which is not obvious, is that writing enough functions makes code testable.  This is because when you write a function you are constraining the state being passed to the function: it should be whatever your arguments are (with the obvious exception of objects that have internal state).  When you can control the state like that, things get a lot easier to test.  You can see how I pushed the `add_upper_and_main` code into a function, which just takes a data frame and a column name.  Then I can easily create a data frame in my test code and manipulate it rather than having to read in a csv file that I need to keep on disk.  One csv file isn't too bad, but you can imagine as you have more tests you have more csv files.\r\n\r\nYou'll develop a nose for your code and being to notice \"smells\".  Code smells are things that don't seem quite right in the code.  Honing this sense will help you decide when something should be a function.\r\n\r\nSome rules of thumb:\r\n- if you need to run the same code multiple times, consider a function\r\n- if you need a small testable unit of work, consider a function\r\n- if your current bit of code is more than a screen or two long, consider breaking it up into smaller functions (more than two pages of code is hard to keep in your head at once)\r\n- if you are passing in variables to one function that are only used once, or just used at the start or end of the function, consider if they belong in a different function (e.g. I refactored your `main` function to be ignorant of csv files, and do that just outside in the main body of the script)\r\n  ","created_at":"2016-07-27T18:38:51Z","updated_at":"2016-07-27T18:46:21Z","closed_at":null,"merged_at":null,"merge_commit_sha":"32840126a39be19e447612ae5ae17087a7926bb0","assignee":null,"assignees":[],"milestone":null,"commits_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/pulls/1/commits","review_comments_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/pulls/1/comments","review_comment_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/pulls/comments{/number}","comments_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/issues/1/comments","statuses_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/statuses/5d9c0d80ba17da734ef72308a411f2e6e322cacb","head":{"label":"hkz-aarvesen:AEA.suggestions","ref":"AEA.suggestions","sha":"5d9c0d80ba17da734ef72308a411f2e6e322cacb","user":{"login":"hkz-aarvesen","id":4959721,"avatar_url":"https://avatars.githubusercontent.com/u/4959721?v=3","gravatar_id":"","url":"https://api.github.com/users/hkz-aarvesen","html_url":"https://github.com/hkz-aarvesen","followers_url":"https://api.github.com/users/hkz-aarvesen/followers","following_url":"https://api.github.com/users/hkz-aarvesen/following{/other_user}","gists_url":"https://api.github.com/users/hkz-aarvesen/gists{/gist_id}","starred_url":"https://api.github.com/users/hkz-aarvesen/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/hkz-aarvesen/subscriptions","organizations_url":"https://api.github.com/users/hkz-aarvesen/orgs","repos_url":"https://api.github.com/users/hkz-aarvesen/repos","events_url":"https://api.github.com/users/hkz-aarvesen/events{/privacy}","received_events_url":"https://api.github.com/users/hkz-aarvesen/received_events","type":"User","site_admin":false},"repo":{"id":64332120,"name":"Real-Estate-Data-Cleaner","full_name":"hkz-aarvesen/Real-Estate-Data-Cleaner","owner":{"login":"hkz-aarvesen","id":4959721,"avatar_url":"https://avatars.githubusercontent.com/u/4959721?v=3","gravatar_id":"","url":"https://api.github.com/users/hkz-aarvesen","html_url":"https://github.com/hkz-aarvesen","followers_url":"https://api.github.com/users/hkz-aarvesen/followers","following_url":"https://api.github.com/users/hkz-aarvesen/following{/other_user}","gists_url":"https://api.github.com/users/hkz-aarvesen/gists{/gist_id}","starred_url":"https://api.github.com/users/hkz-aarvesen/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/hkz-aarvesen/subscriptions","organizations_url":"https://api.github.com/users/hkz-aarvesen/orgs","repos_url":"https://api.github.com/users/hkz-aarvesen/repos","events_url":"https://api.github.com/users/hkz-aarvesen/events{/privacy}","received_events_url":"https://api.github.com/users/hkz-aarvesen/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/hkz-aarvesen/Real-Estate-Data-Cleaner","description":null,"fork":true,"url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner","forks_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/forks","keys_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/keys{/key_id}","collaborators_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/teams","hooks_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/hooks","issue_events_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/issues/events{/number}","events_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/events","assignees_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/assignees{/user}","branches_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/branches{/branch}","tags_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/tags","blobs_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/git/refs{/sha}","trees_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/git/trees{/sha}","statuses_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/statuses/{sha}","languages_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/languages","stargazers_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/stargazers","contributors_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/contributors","subscribers_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/subscribers","subscription_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/subscription","commits_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/commits{/sha}","git_commits_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/git/commits{/sha}","comments_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/comments{/number}","issue_comment_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/issues/comments{/number}","contents_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/contents/{+path}","compare_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/compare/{base}...{head}","merges_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/merges","archive_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/downloads","issues_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/issues{/number}","pulls_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/pulls{/number}","milestones_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/milestones{/number}","notifications_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/labels{/name}","releases_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/releases{/id}","deployments_url":"https://api.github.com/repos/hkz-aarvesen/Real-Estate-Data-Cleaner/deployments","created_at":"2016-07-27T18:24:44Z","updated_at":"2016-07-27T18:24:45Z","pushed_at":"2016-07-27T18:25:11Z","git_url":"git://github.com/hkz-aarvesen/Real-Estate-Data-Cleaner.git","ssh_url":"git@github.com:hkz-aarvesen/Real-Estate-Data-Cleaner.git","clone_url":"https://github.com/hkz-aarvesen/Real-Estate-Data-Cleaner.git","svn_url":"https://github.com/hkz-aarvesen/Real-Estate-Data-Cleaner","homepage":null,"size":25,"stargazers_count":0,"watchers_count":0,"language":"Python","has_issues":false,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"amazingvince:master","ref":"master","sha":"098777349d6d813c43c87b56448f0140ec4eebac","user":{"login":"amazingvince","id":13532523,"avatar_url":"https://avatars.githubusercontent.com/u/13532523?v=3","gravatar_id":"","url":"https://api.github.com/users/amazingvince","html_url":"https://github.com/amazingvince","followers_url":"https://api.github.com/users/amazingvince/followers","following_url":"https://api.github.com/users/amazingvince/following{/other_user}","gists_url":"https://api.github.com/users/amazingvince/gists{/gist_id}","starred_url":"https://api.github.com/users/amazingvince/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/amazingvince/subscriptions","organizations_url":"https://api.github.com/users/amazingvince/orgs","repos_url":"https://api.github.com/users/amazingvince/repos","events_url":"https://api.github.com/users/amazingvince/events{/privacy}","received_events_url":"https://api.github.com/users/amazingvince/received_events","type":"User","site_admin":false},"repo":{"id":64280230,"name":"Real-Estate-Data-Cleaner","full_name":"amazingvince/Real-Estate-Data-Cleaner","owner":{"login":"amazingvince","id":13532523,"avatar_url":"https://avatars.githubusercontent.com/u/13532523?v=3","gravatar_id":"","url":"https://api.github.com/users/amazingvince","html_url":"https://github.com/amazingvince","followers_url":"https://api.github.com/users/amazingvince/followers","following_url":"https://api.github.com/users/amazingvince/following{/other_user}","gists_url":"https://api.github.com/users/amazingvince/gists{/gist_id}","starred_url":"https://api.github.com/users/amazingvince/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/amazingvince/subscriptions","organizations_url":"https://api.github.com/users/amazingvince/orgs","repos_url":"https://api.github.com/users/amazingvince/repos","events_url":"https://api.github.com/users/amazingvince/events{/privacy}","received_events_url":"https://api.github.com/users/amazingvince/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/amazingvince/Real-Estate-Data-Cleaner","description":null,"fork":false,"url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner","forks_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/forks","keys_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/keys{/key_id}","collaborators_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/teams","hooks_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/hooks","issue_events_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/issues/events{/number}","events_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/events","assignees_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/assignees{/user}","branches_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/branches{/branch}","tags_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/tags","blobs_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/git/refs{/sha}","trees_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/git/trees{/sha}","statuses_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/statuses/{sha}","languages_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/languages","stargazers_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/stargazers","contributors_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/contributors","subscribers_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/subscribers","subscription_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/subscription","commits_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/commits{/sha}","git_commits_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/git/commits{/sha}","comments_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/comments{/number}","issue_comment_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/issues/comments{/number}","contents_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/contents/{+path}","compare_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/compare/{base}...{head}","merges_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/merges","archive_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/downloads","issues_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/issues{/number}","pulls_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/pulls{/number}","milestones_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/milestones{/number}","notifications_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/labels{/name}","releases_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/releases{/id}","deployments_url":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/deployments","created_at":"2016-07-27T05:39:49Z","updated_at":"2016-07-27T05:42:39Z","pushed_at":"2016-07-27T18:38:51Z","git_url":"git://github.com/amazingvince/Real-Estate-Data-Cleaner.git","ssh_url":"git@github.com:amazingvince/Real-Estate-Data-Cleaner.git","clone_url":"https://github.com/amazingvince/Real-Estate-Data-Cleaner.git","svn_url":"https://github.com/amazingvince/Real-Estate-Data-Cleaner","homepage":null,"size":25,"stargazers_count":0,"watchers_count":0,"language":"Python","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":1,"mirror_url":null,"open_issues_count":1,"forks":1,"open_issues":1,"watchers":0,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/pulls/1"},"html":{"href":"https://github.com/amazingvince/Real-Estate-Data-Cleaner/pull/1"},"issue":{"href":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/issues/1"},"comments":{"href":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/issues/1/comments"},"review_comments":{"href":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/pulls/1/comments"},"review_comment":{"href":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/pulls/1/commits"},"statuses":{"href":"https://api.github.com/repos/amazingvince/Real-Estate-Data-Cleaner/statuses/5d9c0d80ba17da734ef72308a411f2e6e322cacb"}}}}
{"actor":{"display_login":"hkz-aarvesen"}}