-
Notifications
You must be signed in to change notification settings - Fork 35
Onboarding Project: Maxwill #48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 23 commits
3a4e462
8a555a7
75dc47a
476cad9
b11aad3
6d8b880
ecf387c
441ca1d
c948341
700d931
090f7cd
9005d45
86fb0dd
3b5990f
db01e42
2764976
df647ea
f8f8d24
44b81a7
ab121b8
0f4c00b
088d73b
975bc24
3fb7c44
ad77763
8704cb7
cff0b57
3198ed0
530cef4
48d8158
53ca9e5
765552d
59da1fc
d9882f8
18ee383
ff94cd8
99ca15c
12164f0
43bef10
9b9bcc6
e2e0439
1d6c7ba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| { | ||
| "version": "0.2.0", | ||
| "configurations": [ | ||
| { | ||
| "type": "chrome", | ||
| "request": "launch", | ||
| "name": "Debug in Chrome", | ||
| "url": "http://localhost:2050/", | ||
| "webRoot": "${workspaceFolder}", | ||
| "sourceMapPathOverrides": { | ||
| "webpack:///./*": "${webRoot}/*" | ||
| } | ||
| } | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| { | ||
| "version": "2.0.0", | ||
| "tasks": [ | ||
| { | ||
| "type": "typescript", | ||
| "tsconfig": "tsconfig.json", | ||
| "option": "watch", | ||
| "problemMatcher": [ | ||
| "$tsc-watch" | ||
| ], | ||
| "group": "build", | ||
| "label": "tsc: watch - tsconfig.json" | ||
| } | ||
| ] | ||
| } |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please don't add your |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,30 @@ | ||||||||||
| export class ApiManager { | ||||||||||
| private mainUrl: string; | ||||||||||
|
Comment on lines
+1
to
+3
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Space inbetween. |
||||||||||
| constructor(mainUrl: string) { | ||||||||||
|
Comment on lines
+2
to
+5
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please leave an empty line between your class variables and |
||||||||||
| this.mainUrl = mainUrl; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| private fetchJson(mainUrl: string): Promise<any> { | ||||||||||
| return fetch(mainUrl) | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Feels incorrect calling it the main one since in this scope it is only one and can be anything. Main or not. |
||||||||||
| .then(res => res.text()) | ||||||||||
| .then(data => JSON.parse(data)) | ||||||||||
| .catch((err) => { | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either remove the |
||||||||||
| throw err | ||||||||||
| }) | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You need to check if your fetch was successful, as that does not get catch'd by the Please use Also, please add a |
||||||||||
| } | ||||||||||
|
|
||||||||||
| // This function will handle retrieving the records from the api | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please change this into a javadoc style comment (This is, sadly, currently not in our style guide.) Also, no need to say a function is a function. Rather just focus on the action. /**
* This function will punch a wall in the game world
* @param x horizontal coordinate of wall
* @param y vertical coordinate of wall
* @returns true if wall was successfully punched
*/
punchWall(x: number, y: number): booleanGood eg: /**
* Punches a wall
* @param x horizontal coordinate of wall
* @param y vertical coordinate of wall
* @returns true if wall was successfully punched
*/
punchWall(x: number, y: number): boolean |
||||||||||
| getRecords(fromID: number, toID: number): Promise<string[][]> { | ||||||||||
| return this.fetchJson(`${this.mainUrl}records?from=${fromID}&to=${toID}`); | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
| } | ||||||||||
|
|
||||||||||
| // This function will handle retrieving the columns from the api | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment as before about javadoc style. |
||||||||||
| getColumns(): Promise<string[]> { | ||||||||||
| return this.fetchJson(`${this.mainUrl}columns`); | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
| } | ||||||||||
|
|
||||||||||
| // This function will handle retrieving the record count from the api | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same function as before about javadoc style. |
||||||||||
| getRecordCount(): Promise<number> { | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I seriously want to ask you to cache the record count result. It is the type of call you can make once and trust that it won't change again. However, there is nothing wrong with what you did, as this is a good approach in some cases. |
||||||||||
| return this.fetchJson(`${this.mainUrl}recordCount`); | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
| } | ||||||||||
|
Comment on lines
+34
to
+49
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So |
||||||||||
| } | ||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't add your
.jsfiles to this PR.