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

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

No description provided.

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

+	}
+}
+
+let resizeTimeout: number;


Another global variable

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

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


remove space

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

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


remove spaces around

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

+}
+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

+		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

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


this and the else does exactly the same

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

@@ -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

+	</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

@@ -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

+		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

+		.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

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


else if and move in the same line as }

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

+	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

+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

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


No need for ; after declaring a function

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

+		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

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


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

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

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


still not done

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

+	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

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


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

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

+	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

+	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

+	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

+	//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

+		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

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


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

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

+  }
+
+.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

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


Something happend with your formatting here.

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

+  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

@@ -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

+	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

+		/** 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

+		}
+		/** 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

@@ -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

@@ -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

+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

+}
+
+/** 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

+					}
+				})
+				.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

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


As mentioned, please rename the class

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

+	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

+	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

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


Please don't use any

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

+	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

+		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

+		});
+
+		$('#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

+			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

+	}
+
+	/** 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

+				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

+				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

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


Rather make use of px instead of rem

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

+		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

+		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

+		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

+				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

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


Similar comment as before about handling and then throwing again.

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

+				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

+		$('#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

+				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

+				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

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

-	<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