Skip to content
Open
Show file tree
Hide file tree
Changes from 8 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
33 changes: 21 additions & 12 deletions app.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,26 @@ const app = express();

const allScans = [];

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
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;
}
function logScanned(uuid, matches) {
let allMatches = matches;
Copy link
Member

Choose a reason for hiding this comment

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

redundant

Copy link
Author

@Spazcool Spazcool Dec 20, 2017

Choose a reason for hiding this comment

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

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

const now = new Date();
if (!fixture) {
allScans.unshift({ time: now, status: 'missing', uuid });
// 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;
}
allScans.map(item => item.status = (item.uuid === uuid) ? 'fixed' : item.status);
allScans.unshift({ time: now, fixture, uuid });
if (allMatches.length > 1) {
allMatches = allMatches.splice(0, 1);
allMatches[0].double = true;
}
allScans.unshift({
time: now,
fixture: allMatches[0].fixture ? allMatches[0].fixture : '',
status: allMatches[0].fixture ? '' : 'missing',
uuid,
double: allMatches[0].double,
link: allMatches[0].cellRef,
});
}

app.set('view engine', 'ejs');
Expand All @@ -32,9 +40,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 @@ -58,14 +67,14 @@ app.get('/:uuid', (req, res) => {
loadDatabase((allItems) => {
const matches = searchDatabase(req.params, allItems);
if (matches.length === 0) {
logScanned(req.params.uuid);
logScanned(req.params.uuid, [{ fixture: null }]);
res.status(404).render('notFound', {
item: '',
id: req.params.uuid,
});
return;
}
logScanned(req.params.uuid, matches[0].fixture);
logScanned(req.params.uuid, matches);
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
7 changes: 4 additions & 3 deletions googleSpreadsheet.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
const google = require('googleapis');
const keys = require('./config/keys');

function rowToObject(val, lab) {
function rowToObject(val, lab, index) {
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;
Copy link
Member

Choose a reason for hiding this comment

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

potential security issue in leaking spreadsheet link to front end

hard coded value is pretty bad.

Copy link
Contributor

Choose a reason for hiding this comment

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

agree, why do we need a cellRef here ?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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


Expand All @@ -20,8 +21,8 @@ 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));
return callback(response.values.map((row, index) =>
rowToObject(row, response.values[0], index + 1)).splice(1));
});
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 (var i = 0; i < allScans.length; i++) { %>
<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.

<%- allScans[i].time.toTimeString() %> -
<% if (allScans[i].fixture) { %>
<%- allScans[i].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="/<%- 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>
Copy link
Contributor

Choose a reason for hiding this comment

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

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

<% } %>
<% if (allScans[i].double) { %>
<a href="<%- allScans[i].link %>" style="color: red">Duplicate UUID!</a>
<% } %>
</li>
<% } %>
</ul>
</article>
<% include ./partials/footer %>