Log items that share the same UUID #53

Spazcool · 2017-12-14T07:27:13Z

REFACTOR: logScanned to catch doubled UUIDs & render doubles on template

lenzai · 2017-12-17T15:34:23Z

I managed to crash the "page not found" in this branch.

lenzai · 2017-12-17T15:35:22Z

The code both in the logScanned and template introduces duplication.

lenzai

there s a bug

cannot accept code duplication

Spazcool · 2017-12-19T22:55:24Z

@lenzai should be better now. I removed that bug you found as well as the duplicated code. I also added links to item's row in the spreadsheet, should an edit be desired.

lenzai

seems that we are in rabbit hole of moving around more and more information. (i.e. increasing coupling and thus technical debt) in order to just print the perfect logging message.

Logging is supposed to help debugging the application.
Increasing complexity sooner or later will create bugs or at least increase maintenance cost.

lenzai · 2017-12-19T23:47:42Z

app.js

-      matches: searchDatabase(req.query, allItems)
-        .sort((a, b) => (a.floor === b.floor ? 0 : +(a.floor > b.floor) || -1)),
+      matches: searchDatabase(req.query, allItems).sort((a, b) =>
+        (a.floor === b.floor ? 0 : +(a.floor > b.floor) || -1)),
    });


this is noise, refrain from doing cosmetic changes.

you may understand why if you try the git blame command

lenzai · 2017-12-19T23:49:25Z

googleSpreadsheet.js

  const o = {};
  for (let i = 0; i < lab.length; i += 1) {
    o[lab[i]] = val[i];
  }
+  o.cellRef = 'https://docs.google.com/spreadsheets/d/1QHKa3vUpht7zRl_LEzl3BlUbolz3ZiL8yKHzdBL42dY/edit#gid=0&range=A' + index + ':T' + index;
  return o;


potential security issue in leaking spreadsheet link to front end

hard coded value is pretty bad.

agree, why do we need a cellRef here ?

Not a necessary addition but, I thought having a link to any troublesome item's place in the spreadsheet would be useful for whoever has to edit it.

that is a fair concern, and actually you can argue that google spreadsheet retrains access rights to the priviledged users.

on further development we may need a better strategy for that spreadsheet bug fixing.

All that said I still approve that pull request

lenzai · 2017-12-19T23:52:54Z

app.js

-    return;
-  }
+function logScanned(uuid, matches) {
+  let allMatches = matches;


Probably not the right way to deal with it but, this in particular was to avoid linter errors.

mpsido · 2017-12-20T00:30:06Z

views/recent.ejs

+        : <br/>
+        <a href="/<%- allScans[i].uuid %>"><%- allScans[i].uuid %></a>
+        <% if (!allScans[i].fixture) { %>
+          <a href="<%- allScans[i].link %>" style="color: <%- allScans[i].status === 'fixed' ? 'orange' : 'red' %>">Missing Details!</a>


it makes sense to edit style inline here because all we want is to change a color.
but it makes the job of a designer who only knows css more difficult.
consider just giving a style class (class=fixed or class=broken) and edit that style in the css file

mpsido · 2017-12-20T00:31:14Z

googleSpreadsheet.js

  const o = {};
  for (let i = 0; i < lab.length; i += 1) {
    o[lab[i]] = val[i];
  }
+  o.cellRef = 'https://docs.google.com/spreadsheets/d/1QHKa3vUpht7zRl_LEzl3BlUbolz3ZiL8yKHzdBL42dY/edit#gid=0&range=A' + index + ':T' + index;
  return o;


agree, why do we need a cellRef here ?

mpsido · 2017-12-20T00:32:04Z

great job!

Spazcool · 2017-12-20T07:32:46Z

@lenzai I thought the logging here was for whoever is editing the spreadsheets not whoever is debugging the code, even if both groups can use the logs.

lenzai · 2017-12-20T15:32:50Z

@Spazcool, yes it's for the content editors mostly which needs to "debug" data.
Requirements and enhancement you defined are really thoughtful. it's just that the implementation need to be simplified.

Some reference concept:
https://en.wikipedia.org/wiki/Coupling_(computer_programming)
https://en.wikipedia.org/wiki/Encapsulation_(computer_programming)

It's quite difficult to discuss remotely, without either dictating you step by step what to do or just publishing revised code myself. If above links inspire you, feel free to discuss or commit further. Otherwise, we can have a pair programming session in January.

PS: So far, most of the code reviews by @mpsido were published through a mobile phone... I think I fixed that today.

mpsido · 2017-12-20T16:01:29Z

Pushed some simplifications on that code

lenzai

not a good practice to mix refactoring and altering behaviour ( i.e. deleting cellRef)

renaming is thoughtful

introducing redundant local variable is not recommended. It can be a symptom of poor function breakdown

lenzai

some steps towards less complexity ( improved encapsulation and naming)
some off topic changes
introducing multiple bugs

see code comments for details

lenzai · 2017-12-20T16:56:09Z

app.js

@@ -75,6 +75,9 @@ app.get('/:uuid', (req, res) => {
      return;
    }
    logScanned(req.params.uuid, matches);
+    if (matches.length >= 1) {
+      console.log(`Too much matches for uuid ${req.params.uuid} length = ${matches.length}`);
+    }
    matches[0].similarItems = searchDatabase({ fixture: matches[0].fixture }, allItems)


Perhaps amateur questions, but why remove the hard URL and how else do we get a link to work?

lenzai · 2017-12-20T16:57:19Z

app.js

@@ -54,7 +54,7 @@ app.get('/qrlist', (req, res) => {
      .filter(item => item.uuid !== '')
      .filter(item => item.uuid !== undefined)
      .sort((a, b) => (a.floor === b.floor ? 0 : +(a.floor > b.floor) || -1));
-    qrList.forEach(item => item.qr = qr.imageSync('http://url.coderbunker.com/' + item.uuid, { type: 'svg' }));
+    qrList.forEach(item => item.qr = qr.imageSync(item.uuid, { type: 'svg' }));
    res.render('qrList', { matches: qrList });


break behaviour , it should be / instead of

Besides, it's off topic with the current branch

I'll fix that, in which case it won't work ? I need to reproduce it so that I ensure the change will fix that case

if current page is not root folder then it will assume the uuid is a subfolder

lenzai · 2017-12-20T16:59:31Z

googleSpreadsheet.js

  }
-  o.cellRef = 'https://docs.google.com/spreadsheets/d/1QHKa3vUpht7zRl_LEzl3BlUbolz3ZiL8yKHzdBL42dY/edit#gid=0&range=A' + index + ':T' + index;
-  return o;
+  return formatedRow;
 }


not a good practice to mix refactoring and altering behaviour ( i.e. deleting cellRef)

and now the template for http:///recent display irrelevant links to home page.

lenzai · 2017-12-20T16:59:44Z

googleSpreadsheet.js

+    const columns = response.values[0];
+    // map function transforms a list into another list using the given lambda function
+    const formatedRows = response.values.map(row => spreadsheetValuesToObject(row, columns));
+    return callback(formatedRows);
  });


introducing redundant local variable is not recommended. It can be a symptom of poor function breakdown

I actually meant to break into two steps for better readability,
All the things where nested into one line of code that was a headache to read

Extracting local variable is usually bad design.
Extracting subfunction is fine (while inlining others maybe)

I don't understand what "Extracting subfunction" means.
If javascript is smart (and I assume it is), formatedRows is passed by reference.
It does improve readbility don't you think so ?

extracting sub function https://refactoring.com/catalog/extractMethod.html

removing local variables https://refactoring.com/catalog/inlineTemp.html

lenzai · 2017-12-20T17:01:42Z

app.js

-    allMatches[0].double = true;
+
+  const duplicatedItem = item;
+


duplicatedItem is redundant with item we don't need 2 names for 1 thing.

I don't know if item is a reference or a copy.
I wanted to ensure it is a copy because I am writing in it.

I don't think this is creating any copy.

I need to recheck that, but if that is a reference, then I need the local variable

not really useful reply

lenzai · 2017-12-20T17:03:21Z

views/recent.ejs

@@ -2,21 +2,21 @@
 <h1>Recently scanned QR codes</h1>
 <article class="col-xs-12">
  <ul class='recent-list'>
-    <% for (var i = 0; i < allScans.length; i++) { %>
+    <% for (let item of allScans.values()) { %>
      <li>


@mpsido nice! Are there other areas in the code where this rewrite should be applied?

Everywhere there is a for loop ;) if you don't care about the index then you can use for..in or for..of.
See the MSDN documentation to understand the difference ;)

I mean do it.

lenzai · 2017-12-20T17:05:47Z

app.js

@@ -6,28 +6,26 @@ const { loadDatabase, searchDatabase } = require('./googleSpreadsheet');

 const app = express();

-const allScans = [];
+const allScans = new Map();



Because I want to avoid repetitions in the "recent" page.
I want to add one by one unique items on that page

This is not the way to handle changes and features. We create an issue, discuss it and push it.
Hidding secret features in unrelated commits is not appropriate decision workflow and distracting for code review

mpsido · 2017-12-21T03:10:19Z

@lenzai what are the introduced bugs you are talking about ?

mpsido · 2017-12-27T21:41:06Z

Closing this pull request
see that one instead:
#62

…hes list

mpsido · 2018-01-01T15:57:37Z

shall we merge it and close the associated issue ?

mpsido · 2018-01-05T08:41:04Z

@lenzai what do you think we should do with this PR

sonarqubecloud · 2021-01-24T15:00:03Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

REFACTOR: logScanned to catch doubled UUIDs & render doubles on template

9790b58

lenzai requested changes Dec 17, 2017

View reviewed changes

Spazcool added 7 commits December 19, 2017 23:00

FIX: hacked out bug

f19fcc8

REMOVE: cut duplication in logScanned func

4aea460

REFACTOR: logScanned works again

e4bb3c8

REMOVE: double UUID conditional from recent.ejs

b6dc388

ADD: double UUID conditional without duplicate code

93da255

ADD: double UUID conditional without duplicate code

cae791d

ADD: link to item row in spreadsheet to both item obj and recent.ejs

5b37912

lenzai reviewed Dec 19, 2017

View reviewed changes

mpsido approved these changes Dec 20, 2017

View reviewed changes

mpsido added 2 commits December 20, 2017 20:45

Refactoring/simplifying code googleSpreadsheet.js

6f7a97f

Removing hard url in app.js

1c1d78a

Refactoring, addRecentlyScanned function instead of logScanned

d04c2cf

lenzai reviewed Dec 20, 2017

View reviewed changes

lenzai requested changes Dec 20, 2017

View reviewed changes

mpsido closed this Dec 27, 2017

mpsido mentioned this pull request Dec 27, 2017

Refactoring #62

Closed

mpsido added 2 commits December 29, 2017 17:08

Merge master

9d8e1c8

Fixing pre-commit lint warnings, and adding addQrImg function to items

12229de

mpsido reopened this Dec 29, 2017

mpsido added 4 commits December 29, 2017 17:20

moving filterEmptyUuid and sortByFloor functions to attribute of matc…

3cd5020

…hes list

Removing deprecated similar items function

d33e8d9

Fixing qrList regression

9b0b9a6

Reintroducing cellRef

e773185

Log items that share the same UUID #53

Are you sure you want to change the base?

Log items that share the same UUID #53

Uh oh!

Conversation

Spazcool commented Dec 14, 2017

Uh oh!

lenzai commented Dec 17, 2017

Uh oh!

lenzai commented Dec 17, 2017

Uh oh!

lenzai left a comment

Choose a reason for hiding this comment

Uh oh!

Spazcool commented Dec 19, 2017

Uh oh!

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

Spazcool Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mpsido commented Dec 20, 2017

Uh oh!

Spazcool commented Dec 20, 2017

Uh oh!

lenzai commented Dec 20, 2017

Uh oh!

mpsido commented Dec 20, 2017

Uh oh!

lenzai left a comment

Choose a reason for hiding this comment

Uh oh!

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

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

Spazcool Dec 20, 2017 •

edited

Loading