-
Notifications
You must be signed in to change notification settings - Fork 36
Onboarding submission #33
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 7 commits
db05582
d1a4ab0
be18911
43c0c67
4b2adee
7278168
163d996
f59346a
1afa832
0dbb763
047f256
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,216 @@ | ||
| window.onload = () => { | ||
|
|
||
| let numColumns: number; | ||
| let columns: any; | ||
| let recordCount: number; | ||
| let startIndex = 0; // Default value = 0; | ||
| let tableRecordCount = 20; // Default value = 20; | ||
| let timer: number; | ||
|
|
||
| // Canvas: | ||
| let canvas = document.createElement("div"); | ||
| canvas.id = "canvas"; | ||
| $("body").append(canvas); | ||
|
|
||
| // Buttons and fields: | ||
| let searchbtn = document.createElement("button"); | ||
| searchbtn.innerText = "Search"; | ||
|
|
||
| let nextbtn = document.createElement("button"); | ||
| nextbtn.innerText = "Next Page"; | ||
|
|
||
| let prevbtn = document.createElement("button"); | ||
| prevbtn.innerText = "Previous Page"; | ||
|
|
||
| let fromField = document.createElement("input"); | ||
| fromField.placeholder = "from"; | ||
|
|
||
| // Window resizing (using a debouncing method): | ||
| window.onresize = () => { | ||
|
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. 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. |
||
| let time = 100; | ||
|
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. 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 ). |
||
| clearInterval(timer); | ||
| timer = setInterval(function () { | ||
| clearInterval(timer); | ||
| adjustTableRecordCount(); | ||
| let end = tableRecordCount - 1; | ||
| if (recordCount < startIndex + tableRecordCount) { | ||
| startIndex = recordCount - tableRecordCount; | ||
| } | ||
| getRecords(startIndex, startIndex + end); | ||
| }, time); | ||
| } | ||
|
|
||
| // on clicks: | ||
| searchbtn.onclick = function() { | ||
| let from = parseInt(fromField.value, 10); | ||
|
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. Behold, an extract from the es5 library mthod for Typescript /**
* Converts a string to an integer.
* @param s A string to convert into a number.
* @param radix A value between 2 and 36 that specifies the base of the number in numString.
* If this argument is not supplied, strings with a prefix of '0x' are considered hexadecimal.
* All other strings are considered decimal.
*/
declare function parseInt(s: string, radix?: number): number;I include this signature to illustrate that adding the 10 at the end is unnecessary. Please remove. |
||
| // validate field: | ||
| if (validate(from)) { | ||
| let end = tableRecordCount - 1; | ||
| startIndex = from; | ||
| if (recordCount < startIndex + tableRecordCount) { | ||
| startIndex = recordCount - tableRecordCount; | ||
| } | ||
| getRecords(startIndex, startIndex + end); | ||
| } | ||
| }; | ||
|
|
||
| nextbtn.onclick = function() { | ||
| let end = tableRecordCount - 1; | ||
|
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. Considering that you only use this as an offset right at the end (when you call getRecords), is it entirely necessary to declare this as a variable of its own? |
||
| if (recordCount > startIndex + tableRecordCount) { | ||
| startIndex += tableRecordCount; | ||
| if (recordCount < startIndex + tableRecordCount) { | ||
| startIndex = recordCount - tableRecordCount; | ||
| } | ||
|
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 would prefer you using a ternary operation for this e.g. startIndex = someConditional ? <true assign> : <false assign> |
||
| getRecords(startIndex, startIndex + end); | ||
| } | ||
| }; | ||
|
|
||
| prevbtn.onclick = function() { | ||
| if (startIndex >= tableRecordCount) { | ||
| startIndex -= tableRecordCount; | ||
| } else if (startIndex < tableRecordCount) { | ||
| startIndex = 0; | ||
| } | ||
| getRecords(startIndex, startIndex + tableRecordCount - 1); | ||
| }; | ||
|
|
||
| $("#canvas").append(fromField,searchbtn, prevbtn, nextbtn); | ||
|
|
||
| initiate(); | ||
|
|
||
| /** | ||
| * Initiates the HTTP requests to obtain the records and column values necessary for the table. | ||
| */ | ||
| function initiate() { | ||
|
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'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. 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. |
||
| // get number of records: | ||
| $.ajax({ | ||
| url: "http://localhost:2050/recordCount", | ||
| success: function (result) { | ||
| recordCount = parseInt(JSON.parse(result), 10); | ||
| // get columns: | ||
| $.ajax({ | ||
| url: "http://localhost:2050/columns", | ||
| success: function (result) { | ||
| columns = JSON.parse(result); | ||
| numColumns = getSize(columns); | ||
| // get first page of records and display them: | ||
| adjustTableRecordCount(); | ||
| getRecords(0, tableRecordCount - 1); | ||
| }, | ||
| error: function (err) { | ||
| $("body").text("Error: " + err.status + " " + err.statusText); | ||
| } | ||
| }); | ||
| }, | ||
| error: function (err) { | ||
| $("body").text("Error: " + err.status + " " + err.statusText); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Generates a table populated with data, distrubuted evenly. | ||
| * @param data The data records retrieved from the server, structured as a 2D object array. | ||
| * @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why remove the entire table and add everything back, when one need only remove the contents of the table prior to recreating the contents? |
||
| let dataSize = getSize(data); | ||
|
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. dataSize and perhaps a few other properties here don't change, redeclare as const. |
||
| let table = document.createElement("table"); | ||
| let tbdy = document.createElement("tbody"); | ||
| table.appendChild(generateHeadings(columns)); | ||
| for (let i = 0; i < tableRecordCount; i++){ | ||
| let tr = document.createElement("tr"); | ||
| for (let j = 0; j < numColumns; j++){ | ||
| let td = document.createElement("td"); | ||
| if (dataSize > i) | ||
| td.appendChild(document.createTextNode(data[i][j])); | ||
|
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 have performance concerns about N^2 operations like this, especially because I know that there is some bottleneck with working with Javascript's native DOM API that I am not entirely familiar with - leading me to conclude this to perhaps be a slightly expensive call. |
||
| tr.appendChild(td); | ||
| } | ||
| tbdy.appendChild(tr); | ||
| } | ||
| table.appendChild(tbdy); | ||
| canvas.appendChild(table); | ||
| } | ||
|
|
||
| /** | ||
| * Finds the number of entries within a data set. | ||
| * @param data An array of data, type is unknown (any). | ||
| */ | ||
| function getSize(data: any) { | ||
|
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. Your method signature seems to return a number, so I would advise that you append the return type to the method signature and leverage the type safety that Typescript makes available. function getSize(data: any): number {
// Implementation
}Also, it's not great practice to make your function accept any values, because then we are not leveraging the type safety that TS makes available to us. |
||
| let i = 0; | ||
| let entry; | ||
| for (entry in data) { | ||
|
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. Rather use for (let entry in data) { |
||
| i++; | ||
| } | ||
| return i; | ||
| } | ||
|
|
||
| /** | ||
| * Sends out an HTTP request to retrieve data records, coupled with generating a table to display the records. | ||
| * @param from The ID value from which to start searching. | ||
| * @param to The ID value from which to stop searching. | ||
| */ | ||
| function getRecords(from: number, to: number) { | ||
| $.ajax({ | ||
| url: "http://localhost:2050/records", | ||
| data: {from: from, to: to}, | ||
| success: function (result) { | ||
| let data = JSON.parse(result); | ||
| generateTable(data, columns); | ||
| }, | ||
| error: function (err) { | ||
| $("body").text("Error: " + err.status + " " + err.statusText); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Generates and returns a "thead" object containing column headings. | ||
| * @param columns An object array containing the column heading values. | ||
| */ | ||
| function generateHeadings(columns: any) { | ||
| let thead = document.createElement("thead"); | ||
| let tr = document.createElement("tr"); | ||
| for (let j = 0; j < numColumns; j++) { | ||
| let td = document.createElement("td"); | ||
| td.appendChild(document.createTextNode(columns[j])); | ||
| tr.appendChild(td); | ||
| } | ||
| thead.appendChild(tr); | ||
| return thead; | ||
| } | ||
|
|
||
| /** | ||
| * Validates that the field value is a number. | ||
| * @param from The ID value from which to start searching. | ||
| */ | ||
| function validate(from: 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 appreciate the existence of this method, but what does this function return? |
||
| if (isNaN(from)) { | ||
|
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 are leaving your null/undefined handling (in this method) to whatever comes out of the IsNaN method. Please add a case with the undefined or null instances. |
||
| alert("\"From\" field does not have a number value."); | ||
| return false; | ||
| } else if (from < 0) { | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.'); |
||
| return false; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * Adjust the number of records shown in the table according to the window size. | ||
| */ | ||
| function adjustTableRecordCount() { | ||
| let height = window.innerHeight; | ||
| let fontSize = getComputedStyle(document.documentElement).fontSize + ""; | ||
| let rowHeight = parseFloat(fontSize)*2.5; | ||
| if (rowHeight !== undefined){ | ||
|
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. rather do something like if (typeof rowHeight !== 'undefined') {
// Implementation
}But as a secondary aside, @FritzOnFire will probably end up telling you to invert the conditional so that you can get rid of the unnecessary indentation in the code by doing something like if (typeof rowHeight === 'undefined') {
return;
}and then continuing with the implementation after this single conditional. |
||
| tableRecordCount = Math.trunc(height/rowHeight)-2; | ||
| if (tableRecordCount < 1) | ||
|
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. consider changing to a ternary operator for concise writing. |
||
| tableRecordCount = 1; | ||
| } | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| input, button { | ||
| display: inline; | ||
| padding: 0.5rem; | ||
| } | ||
|
|
||
| #canvas { | ||
| width: 100vw; | ||
| height: 100vh; | ||
| } | ||
|
|
||
| table { | ||
| border-collapse: collapse; | ||
| width: 100%; | ||
| table-layout: fixed; | ||
| overflow: hidden; | ||
| font-size: 0.8rem; | ||
| } | ||
|
|
||
| td { | ||
| width: 9%; | ||
| height: 2rem; | ||
| padding: 0.2rem; | ||
| border-left: 1px darkgrey solid; | ||
| } | ||
|
|
||
| tr:hover { | ||
| background-color: lightgrey; | ||
| } | ||
|
|
||
| tr { | ||
| border: 1px darkgrey solid; | ||
| } | ||
|
|
||
| html { | ||
| overflow: hidden; | ||
| } | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| { | ||
| "name": "onboard-javascript", | ||
| "version": "1.0.0", | ||
| "description": "Onboarding", | ||
| "main": "index.js", | ||
| "dependencies": { | ||
| "jquery": "^3.4.1" | ||
|
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. Pls include typescript as an npm package, rather than with apt. The idea is that someone should be able to clone this repo, run npm install and have everything they need, rather than running into the error when they try to run tsc. |
||
| }, | ||
| "devDependencies": {}, | ||
| "scripts": { | ||
| "test": "echo \"Error: no test specified\" && exit 1", | ||
| "build": "tsc" | ||
| }, | ||
| "repository": { | ||
| "type": "git", | ||
| "url": "git+https://github.com/pierrehenrinortje/onboard-javascript.git" | ||
| }, | ||
| "author": "pierre", | ||
| "license": "ISC", | ||
| "bugs": { | ||
| "url": "https://github.com/pierrehenrinortje/onboard-javascript/issues" | ||
| }, | ||
| "homepage": "https://github.com/pierrehenrinortje/onboard-javascript#readme" | ||
| } | ||
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.
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.