Skip to content
Open
Show file tree
Hide file tree
Changes from 9 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
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 (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 %>