Conversation
nkosimarz
requested review from
FritzOnFire,
Winston-Guess,
bmharper,
jasonkofo and
nkosimarz
and removed request for
bmharper
app.ts
Outdated
| window.onload = () => GetColumnNames(); | ||
|
|
||
| //Variable declarations | ||
| let totalRecordCount; |
There was a problem hiding this comment.
Just as a matter of design, I would rather you not use these methods to write to global variables. It's difficult to see the danger of using global variables in the scope of this onboarding project; but imagine if you had 100 different files or editing the same variable (if you had chosen to export this variable and make it truly global) - or even if the file was bigger and all the different sections of the code were all making changes to this one lonely variable. Imagine the bugs that would break out right through your system.
The issue is not even that you are using them in this space, because here I would suppose they are fine, but the issue is that your methods are making changes to the same variable. While in Typescript/Javascript, this may not necessarily be a problem because the language is single threaded (you will learn what that means as you grow in your career), in other languages this could prove very challenging to solve bugs.
| $.ajax({ | ||
| url: "/recordCount", | ||
| type: "GET", | ||
| timeout: 12000, |
There was a problem hiding this comment.
Quite a long timeout you've got there, considering the server runs on the same machine as your front-end code - but I must commend the awareness that you are demonstrating.
app.ts
Outdated
| } | ||
|
|
||
| //Function to sync API calls | ||
| function Sync() { |
There was a problem hiding this comment.
I don't understand why you're wrapping this single method in another single method? Why not just call getActualRecords directly?
app.ts
Outdated
| } | ||
|
|
||
| //Function to sync API calls | ||
| function NextSync() { |
There was a problem hiding this comment.
Same as above, why not just call BuildTable directly?
| } | ||
|
|
||
| //Pages backwards through data in increments of 30 | ||
| function PreviousPage() { |
There was a problem hiding this comment.
once again, these methods make calls to the global variables in this scope even though it isn't class based.
app.ts
Outdated
| columnNames = responseJSON; | ||
| Sync(); | ||
| }) | ||
| .fail(function() { |
app.ts
Outdated
| timeout: 12000, | ||
| dataType: "JSON" | ||
| }) | ||
| .done(function(responseRecords: any) { |
app.ts
Outdated
| records = responseRecords; | ||
| NextSync(); | ||
| }) | ||
| .fail(function() { |
app.ts
Outdated
| url: "/columns", | ||
| type: "GET", | ||
| timeout: 12000, | ||
| dataType: "JSON" |
There was a problem hiding this comment.
Let's use lower case 'json' rather than upper case 'JSON'.
| $.ajax({ | ||
| url: "/records?from=" + from + "&to=" + to, | ||
| timeout: 12000, | ||
| dataType: "JSON" |
There was a problem hiding this comment.
lower case "json" rather than upper case "JSON"
| timeout: 12000, | ||
| dataType: "text" | ||
| }) | ||
| .done(function(responseText: any) { |
|
|
||
| //Pages backwards through data | ||
| function PreviousPage() { | ||
| let NumberOfRows = Math.floor((window.innerHeight - 50) / 24) - 1; |
There was a problem hiding this comment.
const same applies to quite a few other lets
For reference: https://medium.com/javascript-scene/javascript-es6-var-let-or-const-ba58b8dcde75
| } | ||
|
|
||
| function restrictAlphabets(e: any) { | ||
| var x = e.which || e.keycode; |
| } | ||
|
|
||
| //Cells are created and populated with data | ||
| for (let i = 0; i < records.length; i++) { |
There was a problem hiding this comment.
Use new for loop methods from style guide
i.e. for (const key in records) { ... }
or for (const record of records) { ... }
|
|
||
| //Variable declarations | ||
| let totalRecordCount: number; | ||
| let columnNames: any[]; |
3 participants
No description provided.