Skip to content
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 24 additions & 14 deletions app.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,26 @@ const { loadDatabase, searchDatabase } = require('./googleSpreadsheet');

const app = express();

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

function logScanned(uuid, fixture) {
// TRICK: fixture tells if the the uuid was found in database or not
function addRecentlyScanned(uuid, item, nbFound = 0) {
// TRICK: only record uuids
if (!/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i.test(uuid)) {
return;
}
const now = new Date();
if (!fixture) {
allScans.unshift({ time: now, status: 'missing', uuid });
return;

const duplicatedItem = item;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is creating any copy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really useful reply

duplicatedItem.time = new Date();
duplicatedItem.duplicated = nbFound > 1;

if (nbFound === 0) {
duplicatedItem.fixture = '';
duplicatedItem.uuid = uuid;
duplicatedItem.status = 'missing';
}
allScans.map(item => item.status = (item.uuid === uuid) ? 'fixed' : item.status);
allScans.unshift({ time: now, fixture, uuid });

allScans.set(uuid, duplicatedItem);
}

app.set('view engine', 'ejs');
Expand All @@ -32,9 +38,10 @@ app.get(['/favicon.ico', '/robots.txt'], (req, res) => {

app.get('/search', (req, res) => {
loadDatabase((allItems) => {
console.log(allItems);
res.render('search', {
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)),
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is noise, refrain from doing cosmetic changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you may understand why if you try the git blame command

});
});
Expand All @@ -45,7 +52,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 });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

break behaviour , it should be / instead of

Besides, it's off topic with the current branch

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});
});
Expand All @@ -58,14 +65,17 @@ app.get('/:uuid', (req, res) => {
loadDatabase((allItems) => {
const matches = searchDatabase(req.params, allItems);
if (matches.length === 0) {
logScanned(req.params.uuid);
addRecentlyScanned(req.params.uuid, {});
res.status(404).render('notFound', {
item: '',
id: req.params.uuid,
});
return;
}
logScanned(req.params.uuid, matches[0].fixture);
if (matches.length > 1) {
console.log(`Too much matches for uuid ${req.params.uuid} length = ${matches.length}`);
}
addRecentlyScanned(req.params.uuid, matches[0], matches.length);
matches[0].similarItems = searchDatabase({ fixture: matches[0].fixture }, allItems)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

off topic

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.filter(item => item.uuid !== matches[0].uuid)
.splice(0, 3);
Expand Down
18 changes: 11 additions & 7 deletions googleSpreadsheet.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
const google = require('googleapis');
const keys = require('./config/keys');

function rowToObject(val, lab) {
const o = {};
for (let i = 0; i < lab.length; i += 1) {
o[lab[i]] = val[i];
// creates a dictionary mapping column names with the values
// ex: { floor : 402, business : coworking, etc..}
function spreadsheetValuesToObject(values, columns) {
const formatedRow = {};
for (let i = 0; i < columns.length; i += 1) {
formatedRow[columns[i]] = values[i];
}
return o;
return formatedRow;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


function loadDatabase(callback) {
Expand All @@ -20,8 +22,10 @@ function loadDatabase(callback) {
console.log(`The API returned an error: ${err}`);
return;
}
return callback(response.values.map(row =>
rowToObject(row, response.values[0])).splice(1));
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);
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

Expand Down
45 changes: 22 additions & 23 deletions views/recent.ejs
Original file line number Diff line number Diff line change
@@ -1,26 +1,25 @@
<% include ./partials/header %>
<div class="resultContainer col-xs-12">
<h1>Recently scanned QR code</h1>
<article class="col-xs-12">
<ul class='recent-list'>
<% for(var i = 0; i <allScans.length; i++){%>
<% if(allScans[i].fixture) { %>
<li>
<a href="/<%- allScans[i].uuid %>"><%- allScans[i].time.toTimeString() %> - <%- allScans[i].fixture %> </a> :<br/>
<%- allScans[i].uuid %>
</li>
<h1>Recently scanned QR codes</h1>
<article class="col-xs-12">
<ul class='recent-list'>
<% for (let item of allScans.values()) { %>
<li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean do it.

<%- item.time.toTimeString() %> -
<% if (item.fixture) { %>
<%- item.fixture %>
<% } else { %>
<li>
<a href="/<%- allScans[i].uuid %>"><%- allScans[i].time.toTimeString() %>
</a>
<span style="color:<%- allScans[i].status==='fixed'?'orange':'red'%>" >
:<br/>
<%- allScans[i].uuid %>
</span>
</li>
<%}%>
<%}%>
</ul>
</article>
</div>
???
<% } %>
: <br/>
<a href="/<%- item.uuid %>"><%- item.uuid %></a>
<% if (!item.fixture) { %>
<a href="<%- item.link %>" style="color: <%- item.status === 'fixed' ? 'orange' : 'red' %>">Missing Details!</a>
<% } %>
<% if (item.duplicated) { %>
<a href="<%- item.link %>" style="color: red">Duplicate UUID!</a>
<% } %>
</li>
<% } %>
</ul>
</article>
<% include ./partials/footer %>