Implement basic functionality #2

irokas · 2018-07-17T09:47:30Z

Closes #1 .

This version displays the files given in alphabetical order.

parisk

Alright, great start! I want to see some simplifications in the code before moving on with further code review.

Let's move this on 🚀

parisk · 2018-07-17T11:21:47Z

src/main.js

+  const fileList = document.createElement("ul");
+
+  var i, j;
+  for (i=0; i<input.length; i++) {


Instead, of using a for loop, let's use the forEach method instead, which is available in all arrays 😄.

Also, the map method could make a lot of sense here, in order to map array entries to new li elements. Example:

const entries = [{"name": "blim", "type": "file"}, {"name": "blom", "type": "directory"}]; const elements = entries.map(entry => { const element = document.createElement("li"); element.textContent = entry.name; return element; });

parisk · 2018-07-17T11:22:23Z

src/main.js

+      fileItem.splice(checkAndPush(input[i], fileItem), 0, tempItem);
+  }
+
+  for (i=0; i<dirItem.length; i++) {


Same here; let's use forEach.

parisk · 2018-07-17T11:25:02Z

src/main.js

@@ -0,0 +1,42 @@
+function strComp (str1, str2) {
+  if (str1.localeCompare(str2) === -1 || str1.localeCompare(str2) === 0){


We can simplify this to:

return str1.localeCompare(str2) <= 0

Based on the documentation of localeCompare, its return value is:

A negative number if the reference string occurs before the compare string; positive if the reference string occurs after the compare string; 0 if they are equivalent.

parisk · 2018-07-17T11:26:41Z

src/main.js

+  const dirItem = [];
+  const fileItem = [];
+
+  const dirList = document.createElement("ul");


We should use one list here. We want both directories and files in the same list (as described in #1).

parisk · 2018-07-17T11:29:09Z

src/main.js

+  for (i=0; i<input.length; i++) {
+    const tempItem = document.createElement("li");
+    tempItem.textContent = input[i].name;
+    if (input[i].type === "directory")


You can simplify the code here in two ways:

Use filter to filter directories and files. Example:

const files = allEntries.filter(entry => entry.type == 'file');

You can sort an array using the sort method and providing your own sortFunction argument, as described in the documentation.

parisk

Made a few comments, that I would like to see corrected before we move on with:

Formatting
Making it a library
Setting up tests

😄

parisk · 2018-07-18T09:19:30Z

src/main.js

+    return a.name > b.name ? 1 : b.name > a.name ? -1 : 0;
+  });
+
+  dirList = iterator(dirItem);


2 things here:

We should use a single ul for all items (as described in Implement conversion of single-level payload to unordered list #1)

We should declare the ul using const.

parisk · 2018-07-18T09:20:37Z

src/main.js

+    return a.name > b.name ? 1 : b.name > a.name ? -1 : 0;
+  });
+  fileItem.sort(function(a, b) {
+    return a.name > b.name ? 1 : b.name > a.name ? -1 : 0;


Extract the compare function to a named one (e.g. alphabeticCompare) and pass the named function as parameter.

Here we have implemented the same function twice.

parisk · 2018-07-18T09:23:19Z

src/main.js

@@ -0,0 +1,25 @@
+function iterator(array) {


2 things here:

iterator is not a descriptive function name. What about something like convertItemsToUnorderedList?

array is not a descriptive argument name and also might cause confusion with the Array object. What about something like items?

parisk · 2018-07-18T11:14:49Z

src/main.js

@@ -0,0 +1,23 @@
+function convertItemsToUnorderedList(listItems) {
+  const ulList = document.createElement("ul");


I suggest renaming this to ulElement (or something like that), as ulList essentially means "unordered list list", which is too verbose 😂.

parisk · 2018-07-18T11:20:40Z

src/main.js

@@ -0,0 +1,23 @@
+function convertItemsToUnorderedList(listItems) {
+  const ulList = document.createElement("ul");
+  listItems.map(a => {


We will have to change things here, as there is a bad practice. What happens is that there is a side-effect inside the map function argument; you are appending elements to ulList. When using map we better just return values and have no side effects:

const liElements = listItems.map(item => { const liElement = document.createElement("li"); liElement.textContent = item.name; return liElement; }); liElements.forEach(liElement => ulElement.appendChild(liElement));

Alternatively, you can use forEach which is semantically better for such use cases;

listItems.forEach(item => { const liElement = document.createElement("li"); liElement.textContent = item.name; ulElement.appendChild(liElement) });

Up to you!

parisk · 2018-07-18T11:21:16Z

src/main.js

+}
+
+function renderInput(input, container) {
+  const dirItem = input.filter(inputEl => inputEl.type === "directory"),


Since this constant is an array of values, we better rename it to plural form (e.g. dirItems).

parisk · 2018-07-18T11:21:25Z

src/main.js

+
+function renderInput(input, container) {
+  const dirItem = input.filter(inputEl => inputEl.type === "directory"),
+        fileItem = input.filter(inputEl => inputEl.type === "file");


Same here, since this constant is an array of values, we better rename it to plural form (e.g. fileItems).

parisk

Looks and works great!

irokas added 4 commits July 17, 2018 12:28

First step

b26a09d

This version displays the files given in alphabetical order.

Created Demo Page

5bad487

Clear code

789da60

Even less code

4f3d914

parisk suggested changes Jul 17, 2018

View reviewed changes

irokas added 3 commits July 18, 2018 11:51

Cleared code to maximum

0f3d2ef

Prettier code

3d6efa5

Added empty line

5793625

parisk suggested changes Jul 18, 2018

View reviewed changes

Included both the directories and files in the same list

92f4dd7

parisk suggested changes Jul 18, 2018

View reviewed changes

irokas added 6 commits July 18, 2018 15:33

Minor changes

42afb4f

Added package.json

412fe00

Added jest

e48da7f

Made test

d5ab50c

Added prettier script

80523d9

Added travis.yml

930f04b

parisk changed the title ~~WIP: implement basic functionality~~ Implement basic functionality Jul 19, 2018

parisk approved these changes Jul 19, 2018

View reviewed changes

parisk merged commit 0ce2f10 into sourcelair:master Jul 19, 2018

irokas deleted the poc branch July 20, 2018 09:53

		@@ -0,0 +1,42 @@
		function strComp (str1, str2) {
		if (str1.localeCompare(str2) === -1 \|\| str1.localeCompare(str2) === 0){

		@@ -0,0 +1,23 @@
		function convertItemsToUnorderedList(listItems) {
		const ulList = document.createElement("ul");

Implement basic functionality #2

Implement basic functionality #2

Uh oh!

Conversation

irokas commented Jul 17, 2018

Uh oh!

parisk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parisk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parisk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!