{"action":"created","comment":{"url":"https://api.github.com/repos/OpenRA/OpenRA/pulls/comments/24624176","id":24624176,"diff_hunk":"@@ -0,0 +1,55 @@\n+﻿using System.Collections.Generic;\n+using System.Linq;\n+\n+namespace OpenRA.Mods.Common.Traits\n+{\n+\tpublic class PathCacheStorage : ICacheStorage<List<CPos>>\n+\t{\n+\t\tclass CachedPath\n+\t\t{\n+\t\t\tpublic List<CPos> Result;\n+\t\t\tpublic int Tick;\n+\t\t}\n+\n+\t\tprivate const int MaxPathAge = 50;\n+\t\tprivate readonly IWorld world;\n+\t\tDictionary<string, CachedPath> cachedPaths = new Dictionary<string, CachedPath>();\n+\n+\t\tpublic PathCacheStorage(IWorld world)\n+\t\t{\n+\t\t\tthis.world = world;\n+\t\t}\n+\n+\t\tpublic void Remove(string key)\n+\t\t{\n+\t\t\tcachedPaths.Remove(key);\n+\t\t}\n+\n+\t\tpublic void Store(string key, List<CPos> data)\n+\t\t{\n+\t\t\tforeach (var cachedPath in cachedPaths.Where(p => world.WorldTick - p.Value.Tick > MaxPathAge).ToList())","path":"OpenRA.Mods.Common/Traits/World/PathCacheStorage.cs","position":30,"original_position":30,"commit_id":"1c363455341ac2e33144d9e1900fc34e8db9bbfc","original_commit_id":"1c363455341ac2e33144d9e1900fc34e8db9bbfc","user":{"login":"obrakmann","id":4331210,"avatar_url":"https://avatars.githubusercontent.com/u/4331210?v=3","gravatar_id":"","url":"https://api.github.com/users/obrakmann","html_url":"https://github.com/obrakmann","followers_url":"https://api.github.com/users/obrakmann/followers","following_url":"https://api.github.com/users/obrakmann/following{/other_user}","gists_url":"https://api.github.com/users/obrakmann/gists{/gist_id}","starred_url":"https://api.github.com/users/obrakmann/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/obrakmann/subscriptions","organizations_url":"https://api.github.com/users/obrakmann/orgs","repos_url":"https://api.github.com/users/obrakmann/repos","events_url":"https://api.github.com/users/obrakmann/events{/privacy}","received_events_url":"https://api.github.com/users/obrakmann/received_events","type":"User","site_admin":false},"body":"I think there's (a little) room for improvement here. I don't really know how costly those linq queries are in the great scheme of thing, but even in a moderately busy game, you can already have ~100 cached paths, and you need to walk the whole thing every time. Do you think it would make sense to keep track of the tick that the last cleanup took place, so that you don't do it multiple times in the same tick?\r\n\r\nIt's probably premature optimization, but I figure the busy games could need all the perf they can get.","created_at":"2015-02-12T21:53:51Z","updated_at":"2015-02-12T21:53:51Z","html_url":"https://github.com/OpenRA/OpenRA/pull/7358#discussion_r24624176","pull_request_url":"https://api.github.com/repos/OpenRA/OpenRA/pulls/7358","_links":{"self":{"href":"https://api.github.com/repos/OpenRA/OpenRA/pulls/comments/24624176"},"html":{"href":"https://github.com/OpenRA/OpenRA/pull/7358#discussion_r24624176"},"pull_request":{"href":"https://api.github.com/repos/OpenRA/OpenRA/pulls/7358"}}},"pull_request":{"url":"https://api.github.com/repos/OpenRA/OpenRA/pulls/7358","id":27577993,"html_url":"https://github.com/OpenRA/OpenRA/pull/7358","diff_url":"https://github.com/OpenRA/OpenRA/pull/7358.diff","patch_url":"https://github.com/OpenRA/OpenRA/pull/7358.patch","issue_url":"https://api.github.com/repos/OpenRA/OpenRA/issues/7358","number":7358,"state":"open","locked":false,"title":"Refactored the Pathfinder trait and algorithm to fit better the SOLID principles","user":{"login":"Rydra","id":5826108,"avatar_url":"https://avatars.githubusercontent.com/u/5826108?v=3","gravatar_id":"","url":"https://api.github.com/users/Rydra","html_url":"https://github.com/Rydra","followers_url":"https://api.github.com/users/Rydra/followers","following_url":"https://api.github.com/users/Rydra/following{/other_user}","gists_url":"https://api.github.com/users/Rydra/gists{/gist_id}","starred_url":"https://api.github.com/users/Rydra/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Rydra/subscriptions","organizations_url":"https://api.github.com/users/Rydra/orgs","repos_url":"https://api.github.com/users/Rydra/repos","events_url":"https://api.github.com/users/Rydra/events{/privacy}","received_events_url":"https://api.github.com/users/Rydra/received_events","type":"User","site_admin":false},"body":"The pathfinder algorithm was hard to understand and the PathFinder class violated heavily the Single Responsibility principle and the Open-Closed principle. So I refactored it using the Decorator pattern design so classes can focus on a single operation (caching, calculations, logging (on the way)) while being easily and isolately expandable instead of having one bloated and hard-to-reuse-and-extent class.\r\n\r\nI also modified the cachedpaths variable to use a Dictionary instead of a list, as order is not important in this case and insertions, deletions and lookups on a dictionary have cost O(1).\r\n\r\nThe Expand operation in PathSearch also was hard to understand, follow and unit test, so I've reorganized the responsibilities and extracted methods in an intent to make things easier to understand.","created_at":"2015-01-18T22:13:40Z","updated_at":"2015-02-12T21:53:51Z","closed_at":null,"merged_at":null,"merge_commit_sha":"e5b8770fba6756b89668719985d1542fcfa2c98b","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/OpenRA/OpenRA/pulls/7358/commits","review_comments_url":"https://api.github.com/repos/OpenRA/OpenRA/pulls/7358/comments","review_comment_url":"https://api.github.com/repos/OpenRA/OpenRA/pulls/comments/{number}","comments_url":"https://api.github.com/repos/OpenRA/OpenRA/issues/7358/comments","statuses_url":"https://api.github.com/repos/OpenRA/OpenRA/statuses/1c363455341ac2e33144d9e1900fc34e8db9bbfc","head":{"label":"Rydra:upstream/pathfinder","ref":"upstream/pathfinder","sha":"1c363455341ac2e33144d9e1900fc34e8db9bbfc","user":{"login":"Rydra","id":5826108,"avatar_url":"https://avatars.githubusercontent.com/u/5826108?v=3","gravatar_id":"","url":"https://api.github.com/users/Rydra","html_url":"https://github.com/Rydra","followers_url":"https://api.github.com/users/Rydra/followers","following_url":"https://api.github.com/users/Rydra/following{/other_user}","gists_url":"https://api.github.com/users/Rydra/gists{/gist_id}","starred_url":"https://api.github.com/users/Rydra/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Rydra/subscriptions","organizations_url":"https://api.github.com/users/Rydra/orgs","repos_url":"https://api.github.com/users/Rydra/repos","events_url":"https://api.github.com/users/Rydra/events{/privacy}","received_events_url":"https://api.github.com/users/Rydra/received_events","type":"User","site_admin":false},"repo":{"id":19907485,"name":"OpenRA","full_name":"Rydra/OpenRA","owner":{"login":"Rydra","id":5826108,"avatar_url":"https://avatars.githubusercontent.com/u/5826108?v=3","gravatar_id":"","url":"https://api.github.com/users/Rydra","html_url":"https://github.com/Rydra","followers_url":"https://api.github.com/users/Rydra/followers","following_url":"https://api.github.com/users/Rydra/following{/other_user}","gists_url":"https://api.github.com/users/Rydra/gists{/gist_id}","starred_url":"https://api.github.com/users/Rydra/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Rydra/subscriptions","organizations_url":"https://api.github.com/users/Rydra/orgs","repos_url":"https://api.github.com/users/Rydra/repos","events_url":"https://api.github.com/users/Rydra/events{/privacy}","received_events_url":"https://api.github.com/users/Rydra/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/Rydra/OpenRA","description":"An open-source implementation of the Red Alert engine using .NET/mono and OpenGL. Runs on Windows, Linux and Mac OS X.","fork":true,"url":"https://api.github.com/repos/Rydra/OpenRA","forks_url":"https://api.github.com/repos/Rydra/OpenRA/forks","keys_url":"https://api.github.com/repos/Rydra/OpenRA/keys{/key_id}","collaborators_url":"https://api.github.com/repos/Rydra/OpenRA/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/Rydra/OpenRA/teams","hooks_url":"https://api.github.com/repos/Rydra/OpenRA/hooks","issue_events_url":"https://api.github.com/repos/Rydra/OpenRA/issues/events{/number}","events_url":"https://api.github.com/repos/Rydra/OpenRA/events","assignees_url":"https://api.github.com/repos/Rydra/OpenRA/assignees{/user}","branches_url":"https://api.github.com/repos/Rydra/OpenRA/branches{/branch}","tags_url":"https://api.github.com/repos/Rydra/OpenRA/tags","blobs_url":"https://api.github.com/repos/Rydra/OpenRA/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/Rydra/OpenRA/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/Rydra/OpenRA/git/refs{/sha}","trees_url":"https://api.github.com/repos/Rydra/OpenRA/git/trees{/sha}","statuses_url":"https://api.github.com/repos/Rydra/OpenRA/statuses/{sha}","languages_url":"https://api.github.com/repos/Rydra/OpenRA/languages","stargazers_url":"https://api.github.com/repos/Rydra/OpenRA/stargazers","contributors_url":"https://api.github.com/repos/Rydra/OpenRA/contributors","subscribers_url":"https://api.github.com/repos/Rydra/OpenRA/subscribers","subscription_url":"https://api.github.com/repos/Rydra/OpenRA/subscription","commits_url":"https://api.github.com/repos/Rydra/OpenRA/commits{/sha}","git_commits_url":"https://api.github.com/repos/Rydra/OpenRA/git/commits{/sha}","comments_url":"https://api.github.com/repos/Rydra/OpenRA/comments{/number}","issue_comment_url":"https://api.github.com/repos/Rydra/OpenRA/issues/comments/{number}","contents_url":"https://api.github.com/repos/Rydra/OpenRA/contents/{+path}","compare_url":"https://api.github.com/repos/Rydra/OpenRA/compare/{base}...{head}","merges_url":"https://api.github.com/repos/Rydra/OpenRA/merges","archive_url":"https://api.github.com/repos/Rydra/OpenRA/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/Rydra/OpenRA/downloads","issues_url":"https://api.github.com/repos/Rydra/OpenRA/issues{/number}","pulls_url":"https://api.github.com/repos/Rydra/OpenRA/pulls{/number}","milestones_url":"https://api.github.com/repos/Rydra/OpenRA/milestones{/number}","notifications_url":"https://api.github.com/repos/Rydra/OpenRA/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/Rydra/OpenRA/labels{/name}","releases_url":"https://api.github.com/repos/Rydra/OpenRA/releases{/id}","created_at":"2014-05-18T09:46:30Z","updated_at":"2015-02-05T14:58:22Z","pushed_at":"2015-02-12T20:51:13Z","git_url":"git://github.com/Rydra/OpenRA.git","ssh_url":"git@github.com:Rydra/OpenRA.git","clone_url":"https://github.com/Rydra/OpenRA.git","svn_url":"https://github.com/Rydra/OpenRA","homepage":"http://open-ra.org","size":80427,"stargazers_count":2,"watchers_count":2,"language":"C#","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":2,"default_branch":"bleed"}},"base":{"label":"OpenRA:bleed","ref":"bleed","sha":"e79239311a3aa3a235df4f137a64c99f1515366b","user":{"login":"OpenRA","id":409046,"avatar_url":"https://avatars.githubusercontent.com/u/409046?v=3","gravatar_id":"","url":"https://api.github.com/users/OpenRA","html_url":"https://github.com/OpenRA","followers_url":"https://api.github.com/users/OpenRA/followers","following_url":"https://api.github.com/users/OpenRA/following{/other_user}","gists_url":"https://api.github.com/users/OpenRA/gists{/gist_id}","starred_url":"https://api.github.com/users/OpenRA/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/OpenRA/subscriptions","organizations_url":"https://api.github.com/users/OpenRA/orgs","repos_url":"https://api.github.com/users/OpenRA/repos","events_url":"https://api.github.com/users/OpenRA/events{/privacy}","received_events_url":"https://api.github.com/users/OpenRA/received_events","type":"Organization","site_admin":false},"repo":{"id":959908,"name":"OpenRA","full_name":"OpenRA/OpenRA","owner":{"login":"OpenRA","id":409046,"avatar_url":"https://avatars.githubusercontent.com/u/409046?v=3","gravatar_id":"","url":"https://api.github.com/users/OpenRA","html_url":"https://github.com/OpenRA","followers_url":"https://api.github.com/users/OpenRA/followers","following_url":"https://api.github.com/users/OpenRA/following{/other_user}","gists_url":"https://api.github.com/users/OpenRA/gists{/gist_id}","starred_url":"https://api.github.com/users/OpenRA/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/OpenRA/subscriptions","organizations_url":"https://api.github.com/users/OpenRA/orgs","repos_url":"https://api.github.com/users/OpenRA/repos","events_url":"https://api.github.com/users/OpenRA/events{/privacy}","received_events_url":"https://api.github.com/users/OpenRA/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/OpenRA/OpenRA","description":"An open-source implementation of the Command & Conquer: Red Alert engine using .NET/Mono and OpenGL. Runs on Windows, Linux and Mac OS X.","fork":false,"url":"https://api.github.com/repos/OpenRA/OpenRA","forks_url":"https://api.github.com/repos/OpenRA/OpenRA/forks","keys_url":"https://api.github.com/repos/OpenRA/OpenRA/keys{/key_id}","collaborators_url":"https://api.github.com/repos/OpenRA/OpenRA/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/OpenRA/OpenRA/teams","hooks_url":"https://api.github.com/repos/OpenRA/OpenRA/hooks","issue_events_url":"https://api.github.com/repos/OpenRA/OpenRA/issues/events{/number}","events_url":"https://api.github.com/repos/OpenRA/OpenRA/events","assignees_url":"https://api.github.com/repos/OpenRA/OpenRA/assignees{/user}","branches_url":"https://api.github.com/repos/OpenRA/OpenRA/branches{/branch}","tags_url":"https://api.github.com/repos/OpenRA/OpenRA/tags","blobs_url":"https://api.github.com/repos/OpenRA/OpenRA/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/OpenRA/OpenRA/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/OpenRA/OpenRA/git/refs{/sha}","trees_url":"https://api.github.com/repos/OpenRA/OpenRA/git/trees{/sha}","statuses_url":"https://api.github.com/repos/OpenRA/OpenRA/statuses/{sha}","languages_url":"https://api.github.com/repos/OpenRA/OpenRA/languages","stargazers_url":"https://api.github.com/repos/OpenRA/OpenRA/stargazers","contributors_url":"https://api.github.com/repos/OpenRA/OpenRA/contributors","subscribers_url":"https://api.github.com/repos/OpenRA/OpenRA/subscribers","subscription_url":"https://api.github.com/repos/OpenRA/OpenRA/subscription","commits_url":"https://api.github.com/repos/OpenRA/OpenRA/commits{/sha}","git_commits_url":"https://api.github.com/repos/OpenRA/OpenRA/git/commits{/sha}","comments_url":"https://api.github.com/repos/OpenRA/OpenRA/comments{/number}","issue_comment_url":"https://api.github.com/repos/OpenRA/OpenRA/issues/comments/{number}","contents_url":"https://api.github.com/repos/OpenRA/OpenRA/contents/{+path}","compare_url":"https://api.github.com/repos/OpenRA/OpenRA/compare/{base}...{head}","merges_url":"https://api.github.com/repos/OpenRA/OpenRA/merges","archive_url":"https://api.github.com/repos/OpenRA/OpenRA/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/OpenRA/OpenRA/downloads","issues_url":"https://api.github.com/repos/OpenRA/OpenRA/issues{/number}","pulls_url":"https://api.github.com/repos/OpenRA/OpenRA/pulls{/number}","milestones_url":"https://api.github.com/repos/OpenRA/OpenRA/milestones{/number}","notifications_url":"https://api.github.com/repos/OpenRA/OpenRA/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/OpenRA/OpenRA/labels{/name}","releases_url":"https://api.github.com/repos/OpenRA/OpenRA/releases{/id}","created_at":"2010-10-04T07:38:05Z","updated_at":"2015-02-12T14:43:20Z","pushed_at":"2015-02-12T14:43:19Z","git_url":"git://github.com/OpenRA/OpenRA.git","ssh_url":"git@github.com:OpenRA/OpenRA.git","clone_url":"https://github.com/OpenRA/OpenRA.git","svn_url":"https://github.com/OpenRA/OpenRA","homepage":"http://www.openra.net","size":157923,"stargazers_count":1719,"watchers_count":1719,"language":"C#","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":603,"mirror_url":null,"open_issues_count":700,"forks":603,"open_issues":700,"watchers":1719,"default_branch":"bleed"}},"_links":{"self":{"href":"https://api.github.com/repos/OpenRA/OpenRA/pulls/7358"},"html":{"href":"https://github.com/OpenRA/OpenRA/pull/7358"},"issue":{"href":"https://api.github.com/repos/OpenRA/OpenRA/issues/7358"},"comments":{"href":"https://api.github.com/repos/OpenRA/OpenRA/issues/7358/comments"},"review_comments":{"href":"https://api.github.com/repos/OpenRA/OpenRA/pulls/7358/comments"},"review_comment":{"href":"https://api.github.com/repos/OpenRA/OpenRA/pulls/comments/{number}"},"commits":{"href":"https://api.github.com/repos/OpenRA/OpenRA/pulls/7358/commits"},"statuses":{"href":"https://api.github.com/repos/OpenRA/OpenRA/statuses/1c363455341ac2e33144d9e1900fc34e8db9bbfc"}}}}