Updated Project by ThokozaniNqwili · Pull Request #47 · IMQS/onboard-javascript

ThokozaniNqwili · 2023-08-11T14:49:26Z

No description provided.

app.ts

DevinFledermaus · 2023-08-28T09:02:40Z

app.ts

+	}
+}
+
+let resizeTimeout: number;


Another global variable

app.ts

DevinFledermaus · 2023-08-28T09:12:39Z

app.ts

+			$('#page').empty();
+			$('#page').append(`Showing record: ${firstNumber} - ${count}`);
+			$('#loader').hide()
+


remove space

app.ts

DevinFledermaus · 2023-08-28T09:13:13Z

app.ts

+			}
+			if (records[r].includes(inputValue)) {
+
+				lastRow.css('background-color', '#DDC0B4'); // hightlights the searched row


remove spaces around

DevinFledermaus · 2023-08-28T09:16:19Z

app.ts

+}
+fetchColumns()
+
+async function adjustRowsByScreenHeight(): Promise<number> { // calculates the number of rows that can fit the screen


Why is this an async?

I made a mistake because of my understanding, I will work on it.

DevinFledermaus · 2023-08-28T09:20:14Z

app.ts

+		if (firstNumber < 0) {
+			firstNumber = 0;
+			lastNumber = firstNumber + (calculatedRows - 1);
+		}


move the else on the same line as the } please

DevinFledermaus · 2023-08-28T09:24:25Z

app.ts

+			lastNumber = firstNumber
+
+		}
+		if (!lastNumber) {


this and the else does exactly the same

DevinFledermaus · 2023-08-28T09:26:13Z

app.ts

@@ -0,0 +1,199 @@
+import { ajax, css } from "jquery";
+var firstNumber: number = 0;
+var lastNumber: number;


would be nice to have a default

DevinFledermaus · 2023-08-28T09:28:48Z

index.html

+	</table>
+
+
+	<script src="app.js"></script>


can move script tag into the head tag

DevinFledermaus

Some comments as initial changes

DevinFledermaus · 2023-08-28T11:42:15Z

app.ts

@@ -0,0 +1,183 @@
+import { ajax, css } from "jquery";
+let firstNumber: number = 0;
+let lastNumber: number = 0;


No need to set the variable and the type, either give a type, or set it

DevinFledermaus · 2023-08-28T11:42:40Z

app.ts

+		if (!recordCount.ok) {
+			throw new Error('Failed to fetch record count');
+		}
+		const data = await recordCount.json()


Suggested change

const data = await recordCount.json()

const data = JSON.parse(recordCount)

DevinFledermaus · 2023-08-28T11:45:04Z

app.ts

+		.then((columns: string[]) => {
+			const colArray = columns;
+
+			for (let c = 0; c < colArray.length; c++) {


use for..of loop please

DevinFledermaus · 2023-08-28T11:47:49Z

app.ts

+		if (calculatedRows === 0) {
+			lastNumber = firstNumber
+		}
+		if (firstNumber < 0) {


else if and move in the same line as }

DevinFledermaus · 2023-08-28T11:59:05Z

app.ts

+	if (firstRow) { // checks if the first row exists
+		const cells = firstRow.querySelectorAll("td");
+		const firstRecord: string[] = [];
+		cells.forEach((cell) => {


Rather make use of a for..of loop, even a for loop is fine, but for of is preferred

DevinFledermaus · 2023-09-01T09:42:13Z

app.ts

+import { ajax, css } from "jquery";
+let firstNumber = 0;
+let lastNumber = 0;
+let backend: string = "http://localhost:2050";


Try for 2 global variables

DevinFledermaus · 2023-09-01T09:43:09Z

app.ts

+	} catch (error) {
+		throw new Error('Error fetching the record count')
+	};
+};


No need for ; after declaring a function

DevinFledermaus · 2023-09-01T09:45:15Z

app.ts

+		const jsonText = await response.text();
+		const columns: string[] = await JSON.parse(jsonText);


Suggested change

const jsonText = await response.text();

const columns: string[] = await JSON.parse(jsonText);

const columns: string[] = JSON.parse(response.text());

DevinFledermaus · 2023-09-01T09:46:58Z

app.ts

+	};
+};
+
+function adjustRowsByScreenHeight() {


no need to use the function keyword and provide a return type

DevinFledermaus · 2023-09-01T09:55:21Z

app.ts

+async function displayRecords(): Promise<void> {
+	try {
+		$('#loader').show();
+		let count = await fetchRecordCount() - 1;


still not done

DevinFledermaus · 2023-09-01T10:13:41Z

app.ts

+	try {
+		const response = await fetch(`${backend}/records?from=${from}&to=${to}`);
+		if (!response.ok) {
+			throw new Error("Sorry, there's a problem with the network");
+		}
+		return response.json();
+	} catch (error) {
+		throw new Error('Error fetching records from server');
+	};


No need for the async anymore, and can remove the try and catch

Suggested change

try {

const response = await fetch(`${backend}/records?from=${from}&to=${to}`);

if (!response.ok) {

throw new Error("Sorry, there's a problem with the network");

}

return response.json();

} catch (error) {

throw new Error('Error fetching records from server');

};

return fetch(`${backend}/records?from=${from}&to=${to}`)

.then(res => {

if (!res.ok) {

throw "Sorry, there's a problem with the network";

}

return res.json();

}).catch(err => {

throw 'Error fetching records from server ' + err;

});

FritzOnFire · 2023-09-08T11:44:31Z

app.ts

+import { ajax, css } from "jquery";
+class GlobalVariables {


Please leave an empty line between your imports and class definitions.

FritzOnFire · 2023-09-08T11:45:30Z

app.ts

+	resizeTimeout = 0;
+	//fetches the number of records from localhost
+	fetchRecordCount(): Promise<number> {


Please leave and empty line between your class attributes and your functions.

FritzOnFire · 2023-09-08T11:45:46Z

app.ts

+	firstNumber = 0;
+	lastNumber = 0;
+	backend = "http://localhost:2050";
+	resizeTimeout = 0;


Please add a type to each of these variables.

FritzOnFire · 2023-09-08T11:47:36Z

app.ts

+	lastNumber = 0;
+	backend = "http://localhost:2050";
+	resizeTimeout = 0;
+	//fetches the number of records from localhost


Please change this to a javadoc style comment. and instead of saying localhost, rather say "the backend attribute on this class"

FritzOnFire · 2023-09-08T11:48:20Z

app.ts

+	//fetches the number of records from localhost
+	fetchRecordCount(): Promise<number> {
+		return fetch(`${this.backend}/recordCount`)
+			.then((res) => {


Either remove the () around the res, or specify the correct type for res

FritzOnFire · 2023-09-08T12:26:40Z

app.ts

+		let inputValue = $('#searchInput').val() as number;
+		$('#page').empty();
+		//calls to calculate the range once button is clicked
+		await updateRecordsAndResize(inputValue);


This function can throw an error, please catch it and handle it.

FritzOnFire · 2023-09-08T12:27:04Z

app.ts

+		rightArrow();
+	})
+	$('.arrow-left').on('click', () => {
+		leftArrow();


This function can throw and error, please catch it and handle it.

FritzOnFire · 2023-09-08T12:29:20Z

style.css

+  }
+
+.loader-spinner {
+  border: calc(200px / 10) solid #97694F;


calc is really really really bad, please don't use it.

FritzOnFire · 2023-09-08T12:29:39Z

style.css

+	justify-content: center;
+	align-items: center;
+	z-index: 9999;
+  }


Something happend with your formatting here.

FritzOnFire · 2023-09-08T12:29:51Z

style.css

+  height: 100px;
+  animation: spin 2s linear infinite;
+  background-color: #f7e7ce;
+  }


Formatting issue here.

CelesteNaude

Please make sure that all events that the user can fire off is debounced, i.e. search, resize, and buttons.

CelesteNaude · 2023-09-11T15:17:08Z

app.ts

@@ -0,0 +1,265 @@
+import { ajax, css } from "jquery";
+
+class Myclass {


Please move the class into a separate file.

CelesteNaude · 2023-09-11T16:14:26Z

app.ts

+	const halfRange = Math.floor(calculatedRows / 2);
+	myClass.firstNumber = Math.max(0, inputValue - halfRange);
+	myClass.lastNumber = Math.min(recordCount, myClass.firstNumber + (calculatedRows - 1));


If I understand correctly, you show the searched record in the middle of the grid and highlight it. In IMQS we show the most relevant record we're searching for at the top of the grid without formatting. But you can still keep the formatting because it's cool.

Soooooo... I can't find an example that @CelesteNaude is talking about... I can only find two examples where we do it the way that you are doing it. But that has a lot to do with the fact that most of the time we filter instead of search.

(BTW @CelesteNaude, the two places is user-management, where we highlight and scroll to the user you just edited and the grids on the map, when you click on something on the map, and highlight and scroll to the feature you clicked on)

This I remember from the advice that was given to me for my own onboarding project, weird.

CelesteNaude · 2023-09-11T16:18:35Z

app.ts

+		/** determines te value in the last row */
+		const lastID = parseFloat(lastRecord[0]);
+		/** checks if the last value is within range */
+		if (0 <= lastID && lastID <= (recordCount)) {


Don't need the () around recordCount

CelesteNaude · 2023-09-11T16:19:24Z

app.ts

+		}
+		/** determines te value in the first row */
+		const firstID = parseFloat(firstRecord[0]);
+		if (0 <= firstID && firstID <= (recordCount)) {


Same as above

FritzOnFire · 2023-09-12T09:15:53Z

app.ts

@@ -0,0 +1,230 @@
+import { ajax, css } from "jquery";
+
+const myClass = new Myclass();


Global variables are bad, please remove them.

FritzOnFire · 2023-09-12T09:16:49Z

app.ts

@@ -0,0 +1,230 @@
+import { ajax, css } from "jquery";
+
+const myClass = new Myclass();


Also... please... please give it a better name... This name creates the idea that the Dev that made it isn't even aware of what it is used for.

FritzOnFire · 2023-09-12T09:24:20Z

app.ts

+const myClass = new Myclass();
+
+/** Initializes the table head */
+function createTable(): Promise<string[]> {


I don't think this is the right name for your function. It seems to be adding headings to a dom element with the class head. Which does not sound like it is "creating a table".

I also don't know how I feel about returning an array of strings? Or anything for that matter. In my head a create* function is either creating something and returning it, or creating something somewhere and returning a reference to it. With the "returning a reference" being completely optional, and I your case I don't think you should be returning anything.

FritzOnFire · 2023-09-12T09:25:51Z

app.ts

+}
+
+/** calculates the number of rows that can fit the screen */
+const calculatingRows = (): number => {


... why make a constant variable that points to a function? You can just make a function. Please change this to just be a normal function.

FritzOnFire · 2023-09-12T09:30:55Z

app.ts

+					}
+				})
+				.catch(err => {
+					throw new Error('Error occured while resizing' + err);


This is the last line of defense, you have to handle the error.

FritzOnFire · 2023-09-12T11:33:26Z

myClass.ts

@@ -0,0 +1,48 @@
+class Myclass {


As mentioned, please rename the class

FritzOnFire · 2023-09-12T11:37:01Z

myClass.ts

+	firstNumber: number = 0;
+	lastNumber: number = 0;


These two values are not being used in this class... Maybe you forgot to add methods using them? Or maybe they should not be in this class?

FritzOnFire · 2023-09-12T11:37:51Z

myClass.ts

+	firstNumber: number = 0;
+	lastNumber: number = 0;
+	backend: string = "http://localhost:2050";
+	resizeTimeout: number = 0;


This value is not being used in this class... Maybe you forgot to add methods that uses it? or maybe it should not be in this class?

FritzOnFire · 2023-09-12T11:38:09Z

myClass.ts

+	}
+
+	/** fetches records from backend */
+	fetchRecords(from: number, to: number): Promise<any[]> {


Please don't use any

FritzOnFire · 2023-09-12T11:39:00Z

myClass.ts

+	resizeTimeout: number = 0;
+
+	/** fetches the number of records from backend */
+	fetchRecordCount(): Promise<number> {


You saaaaay number... yet you return res.json()... something is not adding up.

anzelIMQS · 2023-09-22T07:41:00Z

app.ts

+		if (this.firstNumber < 0) {
+			this.firstNumber = 0;
+		} else {
+			this.firstNumber = this.firstNumber;


Setting this.firstNumber equal to this.firstNumber? Sjoe 😢 Is the else then even necessary?

anzelIMQS · 2023-09-22T10:56:06Z

app.ts

+		});
+
+		$('#searchInput').on('keydown', (event) => {
+			if (event.key === 'e' || event.key === 'E') {


Please rather make use of regex to stop the user from entering anything besides a digit.

anzelIMQS · 2023-09-22T10:57:12Z

app.ts

+			if (inputValue.includes('.')) {
+				$('#searchInput').val(inputValue.replace('.', ''));
+			}


With regex then these checks won't be necessary since you know the user could only type in digits.

anzelIMQS · 2023-09-22T11:08:07Z

app.ts

+	}
+
+	/** calculates the number of rows that can fit the screen */
+	calculatingRows(): number {


Suggested change

calculatingRows(): number {

getNumberOfRows(): number {

With a function name like that it is more descriptive of what it does. calculatingRows sounds like a boolean check like isCalculatingRows? Rather give it the verb of what it is going to do. I mean, since this function returns something how about get? How it got it is not maybe important. It calculated it, someone else calculated it? We just want the number of rows.

anzelIMQS · 2023-09-22T11:24:07Z

app.ts

+				const lastID = parseFloat(cells[0].textContent || "");
+				// checks if the last value is within range 
+				if (0 <= lastID && lastID <= this.recordCount) {
+					// calculates the first number of the page 
+					this.firstNumber = lastID + 1;
+					const calculatedRows = this.calculatingRows();
+					// calculates the last number of the page 
+					this.lastNumber = this.firstNumber + (calculatedRows - 1);
+				}


The way I understand this function, you only need this part of the logic. You don't have to go and find the last child. You know what it is because you set it in this.lastNumber, right? Replace lastID with this.lastNumber and then you can remove the rest and just call this.updateAndDisplayRecords afterward.

anzelIMQS · 2023-09-22T11:26:52Z

app.ts

+				const firstID = parseFloat(cells[0].textContent || "");
+				if (0 <= firstID && firstID <= this.recordCount) {
+					const calculatedRows = this.calculatingRows();
+					this.lastNumber = firstID - 1;
+					this.firstNumber = this.lastNumber - (calculatedRows - 1);
+				}


Similar comment to the rightArrow function's comment. You don't have to rely on the HTML. You are the point of truth. Also, maybe a more descriptive function name instead of rightArrow and leftArrow? I mean, what does the function do? Navigate back and forth. Verbs instead.

anzelIMQS · 2023-09-22T11:31:16Z

style.css

+.showRecords {
+	top: 0.8%;
+	right: 0;
+	padding: 1rem;


Rather make use of px instead of rem

anzelIMQS · 2023-09-22T11:35:28Z

app.ts

+		if (inputValue !== '') {
+		  errorMessage = 'Failed to search, reload the page and search again';
+		  $('#loader').show();
+		  this.searchRecordsAndResize();


Why not call this outside of updateAndDisplayRecords.
Call both this.searchRecordsAndResize and then this.updateAndDisplayRecords in your search input event?

FritzOnFire

Always try and handle errors as late as possible, otherwise your code will not be very portable and reusable.

FritzOnFire · 2023-09-29T07:49:09Z

app.ts

+		this.firstNumber = 0;
+		this.lastNumber = 0;
+		this.recordCount = 0;
+		this.initialize();


I do not recommend calling functions that can throw errors in a constructor... You could technically still handle the errors, but the rest of your code would think everything went fine, when it very much did not.

Please call this initialize after creating a new instance of RecordManager

FritzOnFire · 2023-09-29T08:09:09Z

app.ts

+		this.createTableHeader();
+		dataManager.fetchRecordCount()
+			.then(count => {
+				this.recordCount = count - 1;
+				this.updateAndDisplayRecords();
+				this.recordEventHandlers();
+				this.handleResize();
+			})
+			.catch(err => {
+				alert('Error fetching and displaying the table records, reload the page');
+				throw err;
+			});


So... two things.

Returning a promise makes catching errors much much easier for the calling function, so i would recommend always doing it. BUT

Here you are kicking off two async function calls that do not depend on each other, which is beautiful :) This is a great way to speed up an app, but the calling function still needs to be able to handle the error of the function that you are not waiting for. And so now I will be breaking my own rule about not commenting exact code on an onboarding project. This is how you can wait on multiple async functions at the same time:

Suggested change

this.createTableHeader();

dataManager.fetchRecordCount()

.then(count => {

this.recordCount = count - 1;

this.updateAndDisplayRecords();

this.recordEventHandlers();

this.handleResize();

})

.catch(err => {

alert('Error fetching and displaying the table records, reload the page');

throw err;

});

let p: Promise<void>[] = [];

p.push(this.createTableHeader());

let frcp = dataManager.fetchRecordCount()

.then(count => {

this.recordCount = count - 1;

this.updateAndDisplayRecords();

this.recordEventHandlers();

this.handleResize();

})

.catch(err => {

alert('Error fetching and displaying the table records, reload the page');

throw err;

});

p.push(frcp);

return Promise.all(p);

FritzOnFire · 2023-09-29T08:11:28Z

app.ts

+				alert('Error fetching and displaying the table records, reload the page');
+				throw err;


???

So you handle the error... and then you throw it anyway meaning that you have to handle it again later?

Please do not handle the error here, as who ever is calling initialize needs to know that we failed to initialize and they need to react accordingly.

FritzOnFire · 2023-09-29T08:12:07Z

app.ts

+				alert('Error creating table heading');
+				throw err;


Similar comment as before about handling and then throwing again.

FritzOnFire · 2023-09-29T08:13:16Z

app.ts

+				alert('Error to fetch and display records, reload the page');
+				throw err;


Similar comment as before about handling and then throwing again.

FritzOnFire · 2023-09-29T08:53:48Z

app.ts

+		$('#btnSearch').on('click', (event) => {
+			event.preventDefault();
+			this.searchRecordsAndResize();
+			this.updateAndDisplayRecords();


You need to handle the error being thrown by updateAndDisplayRecords

FritzOnFire · 2023-09-29T08:55:09Z

dataManager.ts

+				if (!res.ok) {
+					throw 'Failed to fetch record count';
+				}
+				return res.json();


Suggested change

return res.json();

return res.text();

FritzOnFire · 2023-09-29T08:56:09Z

dataManager.ts

+				return res.json();
+			})
+			.then(totalRecords => {
+				if(typeof totalRecords !== 'number'){


Nice touch :) Now I kinda feel bad for asking you to not use json.

FritzOnFire · 2023-09-29T09:20:26Z

dataManager.ts

This is a very creative way to hide the fact that you are using a global variable called backend. While there is nothing wrong with using static, and even making a class like this, as there truly is a time and a place for everything, its just unfortunately not in this on boarding project. Please remove all the static labels in this class.

This is basically as bad as turning off asynchronous requests in javascript for this project (I have only seen it once, and I am unable to find out how to do it, but I know it exists).

FritzOnFire · 2023-09-29T09:21:00Z

index.html

-	<p>Hello</p>
+	<div id="loader">
+		<div class="loader-spinner"></div>
+	  </div>


Your formatting is a bit off here.

ThokozaniNqwili added 6 commits August 11, 2023 14:51

update

ad2404b

update

575b160

update

038f93e

update

30785ac

update

96682be

update

e287191

ThokozaniNqwili requested review from AshtonMar, CelesteNaude, DevinFledermaus, FritzOnFire and anzelIMQS August 25, 2023 11:29

format change

b4e7f57