Onboarding submission #33
Conversation
pierrehenrinortje
requested review from
FritzOnFire,
bmharper,
jasonkofo and
nkosimarz
pierrehenrinortje
force-pushed
the
master
branch
from
f362360 to
d1a4ab0
Compare
jasonkofo
left a comment
jasonkofo
left a comment
There was a problem hiding this comment.
So I've put a lot of comments calling for changes, but I am really pleased about the following:
- Formatting is looking good
- Logic is fairly easy to follow because the methods are well named, and compact in terms of functionality
- Your functions have documentation describing their purposes, I really appreciate these
So, I'll look once again once you've made these changes, but I think that it is looking good.
app.ts
Outdated
| /** | ||
| * Initiates the HTTP requests to obtain the records and column values necessary for the table. | ||
| */ | ||
| function initiate() { |
There was a problem hiding this comment.
I'm going to raise a few issues that I have with this paradiagm you have stuck to. These gripes also apply to the generateTable method.
Firstly, you defined this function using the function tag inside of a callback. There are 2 popular ways to define methods in Javascript which I will share below.
The first is using the function tag e.g.
function randomFunctionName(...randomArgs) {
// Implementation
}The second uses the arrow function declaration paradigm, e.g.
randomFunctionName = (...randomArgs) => {
// Implementation
}There are a few benefits to using the latter, most of which are vastly beyond the scope this javascript onboarding. However, I believe that the style guide requires you to use arrow functions, rather than the archaic function keyword.
You also declared these functions after you used them, as a matter of conventions (ala what I remember from the Typescript style guide) that isn't desirable
Finally, at least for now and from what I can gather, you only use this function once. Is it entirely necessary to declare this as a method or can we rather use something like a comment to demarcate specific logic?
| @@ -0,0 +1,216 @@ | |||
| window.onload = () => { | |||
There was a problem hiding this comment.
I have a mild issue with the entirety of your logic living in this onload event handler. Especially considering that you declare another event callback (namely window.onresize) within this callback.
My issue here is primarily one of separation of concerns, you want all these sections of your implementation to interact with both the DOM and other sections of your source code in a coherent and cohesive fashion - for the sake of clarity, some other interests, but primarily for the developer that comes after you.
However to change this would require you to think about how you will manage your other data fields like numColumns, columns etc. I know that in varsity they taught us to shy away from global variables, but don't be afraid, considering you are here embracing procedural programming :)
Also remember Fritz's complaint about the spacing in your IDE being changed to use tab sizes rather than spaces.
app.ts
Outdated
|
|
||
| // Window resizing (using a debouncing method): | ||
| window.onresize = () => { | ||
| let time = 100; |
There was a problem hiding this comment.
time doesn't look like it changes in this function. As a matter of convention at IMQS, we declare variables using the const keyword instead of let in order to let the developer know that the variable isn't going to be re-assigned (except in the case of arrays, because for some reason Typescript doesn't care whether an array is declared with const or let, but there you go.
Internally, it does not make much of a difference, because all of these consts and lets become vars in javascript upon transpilation, but I know that it is important to some people (ahem, ahem, @FritzOnFire ).
app.ts
Outdated
| fromField.placeholder = "from"; | ||
|
|
||
| // Window resizing (using a debouncing method): | ||
| window.onresize = () => { |
There was a problem hiding this comment.
Also, please do not declare this callback while you're declaring the onload callback, rather move it out into a fresh scope. Declaring the other onclick handlers for the HTML elements are fine, but declaring this new callback on the same object feels wrong to me.
app.ts
Outdated
| if (recordCount > startIndex + tableRecordCount) { | ||
| startIndex += tableRecordCount; | ||
| if (recordCount < startIndex + tableRecordCount) { | ||
| startIndex = recordCount - tableRecordCount; | ||
| } |
There was a problem hiding this comment.
I would prefer you using a ternary operation for this e.g.
startIndex = someConditional ? <true assign> : <false assign>
app.ts
Outdated
| */ | ||
| function generateTable(data: any, columns: any) { | ||
| $("table").remove(); // Remove previous table | ||
| let dataSize = getSize(data); |
There was a problem hiding this comment.
dataSize and perhaps a few other properties here don't change, redeclare as const.
app.ts
Outdated
| * @param columns An object array containing the column heading values. | ||
| */ | ||
| function generateTable(data: any, columns: any) { | ||
| $("table").remove(); // Remove previous table |
There was a problem hiding this comment.
Why remove the entire table and add everything back, when one need only remove the contents of the table prior to recreating the contents?
In this context, it doesn't mean much, because the function is only called once, but as a matter of practice, it could prove problematic.
app.ts
Outdated
| function getSize(data: any) { | ||
| let i = 0; | ||
| let entry; | ||
| for (entry in data) { |
There was a problem hiding this comment.
Rather use
for (let entry in data) {
app.ts
Outdated
| let rowHeight = parseFloat(fontSize)*2.5; | ||
| if (rowHeight !== undefined){ | ||
| tableRecordCount = Math.trunc(height/rowHeight)-2; | ||
| if (tableRecordCount < 1) |
There was a problem hiding this comment.
consider changing to a ternary operator for concise writing.
app.ts
Outdated
| alert("\"From\" value cannot be negative."); | ||
| return false; | ||
| } else if (from > recordCount-1) { | ||
| alert("\"From\" value exceeds the record count."); |
There was a problem hiding this comment.
For these text fields, you could have delimited the string using single quotes and then used the double quotes inside the string, to aid the readability of this line
alert('"From" value exceeds the record count.');2 participants
This is the first submission for the onboarding javascript task.
All specifications and requirements have been met.