0
\$\begingroup\$

Review Wanted

Application Summary

The application is a simple screen scraper which is to notify the user when new items are posted.

The code is run as a CRON job every ten minutes. It will scrape the target page and return an array of ALL items matching the search criteria ( hard-coded ) by the user.

The results are compared to the results from the previous time the code was run. If there are any new items, the user is notified.

My Challenges

First of all, I have never used any of the technologies I had to use in the app ( NodeJS, Puppeteer, and Express ). More significant that is, how to track what constitutes a "new" item, an item that has been "seen", etc.

Currently, I added a column in the database, "seen". When the user hits the front end, I will mark those results as "seen".

Also, the way I am saving the items in general may need refactoring. I will leave that up to your opinion.

WorkFlow

Here is a breakdown of how the app works:

WorkFlow

Source Code

Here is the complete source code of the app. It functions well, as far as I can tell. I am concerned about the Duplicate issue though.

const puppeteer = require('puppeteer');
const _ = require("lodash");
var mysql = require('mysql');

var con = mysql.createConnection({
    host: 'localhost',
    user: 'root',
    password: '',
    database: 'mydatabase'
});

var oldItems; // Items already scraped


getSavedJeeps = function () {
    return new Promise(function (resolve, reject) {
        con.query(
            "SELECT * FROM jeeps",
            function (err, rows) {
                if (err) {
                    reject(new Error("Error rows is undefined"));
                } else {
                    resolve(rows);
                }
            }
        )
    })
}

const saveNewJeeps = async function (entity) {

    let objLen = entity.length;

    // FOR EACH OBJECT IN ARRAY...
    for (var i = 0; i < objLen; i++) {
        var savedJeeps = con.query('INSERT INTO newjeeps SET ?', entity[i], function (err, result) {
            // Neat!
            console.log("Save function complete");
        });
    }

    removeDupes();
    return true;

}

const updateAllItems = async function (entity) {

    let objLen = entity.length;

    // FOR EACH OBJECT IN ARRAY...
    for (var i = 0; i < objLen; i++) {
        var savedJeeps = con.query('INSERT INTO jeeps SET ?', entity[i], function (err, result) {
            // Neat!
            console.log("Save function complete");
        });
    }


}

// Gets current items  Search Results
const getItems = async searchTerm => {

    browser = await puppeteer.launch({
        headless: true,
        timeout: 0,
        args: ["--no-sandbox"]
    });

    page = await browser.newPage();
    await page.goto(`https://facebook.com/marketplace/tampa/search/?query=${encodeURI(searchTerm)}&sort=created_date_descending&exact=false`);
    await autoScroll(page);

    const itemList = await page.waitForSelector('div > div > span > div > a[tabindex="0"]')
        .then(() => page.evaluate(() => {

            const itemArray = [];
            const itemNodeList = document.querySelectorAll('div > div > span > div > a[tabindex="0"]');

            itemNodeList.forEach(item => {

                const itemTitle = item.innerText;
                const itemURL = item.getAttribute('href');
                const itemImg = item.querySelector('div > div > span > div > a > div > div > div > div > div > div > img').getAttribute('src');

                var obj = ['price', 'title', 'location', 'miles',
                        ...itemTitle.split(/\n/)
                    ]
                    .reduce((a, c, i, t) => {
                        if (i < 4) a[c] = t[i + 4]
                        return a
                    }, {});

                obj.imgUrl = itemImg;
                obj.itemURL = itemURL;

                itemArray.push(obj);
            });

            return itemArray;

        }))
        .catch(() => console.log("Selector error."));

    return itemList;

}


// This takes care of the auto scrolling problem
async function autoScroll(page) {
    await page.evaluate(async () => {
        await new Promise(resolve => {
            var totalHeight = 0;
            var distance = 100;
            var timer = setInterval(() => {
                var scrollHeight = document.body.scrollHeight;
                window.scrollBy(0, distance);
                totalHeight += distance;

                if (totalHeight >= scrollHeight || scrollHeight > 9000) {
                    clearInterval(timer);
                    resolve();
                }
            }, 100);
        });
    });
}

const removeDupes = async function () {
    // remove duplicates
    sql = `DELETE
    t1
FROM
    jeeps t1
INNER JOIN jeeps t2 WHERE
    t1.title < t2.title AND t1.price = t2.price `;
    return new Promise(function (resolve, reject) {
        con.query(
            sql,
            function (err, rows) {
                if (err) {
                    reject(new Error("Error rows is undefined"));
                } else {
                    resolve();
                }
            }
        )
    })
}

const getDifferences = async function (objNew, objOld) {
    console.log("Inside Differences")
    return _.difference(objNew, objOld);
}

const init = async function () {
    var oldItems;
    const newItems = await getItems("Jeep Wrangler");

    getSavedJeeps()
        .then(function (results) {
            oldItems = results;
        })
        .catch(function (err) {
            console.log("Promise rejection error: " + err);
        })


    const finalArray = await getDifferences(newItems, oldItems);
    const saveSuccess = await saveNewJeeps(finalArray);
    const saveSuccess2 = await updateAllItems(finalArray);
    const changed = (finalArray.length > 0) ? true : false;
    
    if (changed) {
        // Fire Off Email
        const page2 = await browser.newPage();
        await page2.goto(`http://john.example.com/mail.php`);
    }

}


init();

Thanks in advance for your suggestions or comments. I asked a similar question a while back, but the app wasn't ready for review. It is now, and I am ready for your review - whether good or bad.

Regards, John

\$\endgroup\$
3
  • 2
    \$\begingroup\$ Your init function is not waiting for the getSavedJeeps function to finish before calling getDifferences; oldItems will always be undefined, so getDifferences will always return the newItems unaltered. There are also a whole bunch of improvements described in the prior answer on the code in the question: codereview.stackexchange.com/a/242044 that don't appear to have been implemented. (not that you have to implement them all, but many of them are concrete improvements without disadvantages or database restructuring) \$\endgroup\$ Commented May 20, 2020 at 16:04
  • \$\begingroup\$ I just don't understand why it isn't waiting. What more can I do? It's already async. As far as the other improvements, I appreciated the suggestions, but I wanted to try to get it working before I refactor. I think I bit off more than I could chew for a first project. Ok, I will dig in and see why getSavedJeeps is not waiting. Thank you so much CP. \$\endgroup\$
    – John S.
    Commented May 21, 2020 at 2:04
  • 1
    \$\begingroup\$ The issue is similar to the one described in great detail here. In short, you should await the Promise in order for the current function to stop execution until the Promise resolves. \$\endgroup\$ Commented May 21, 2020 at 12:10

1 Answer 1

2
\$\begingroup\$

I can't comment, but I have a question for you.

Question

Why don't you use the jeep id of Facebook as a unique key, this way duplicates will not be a problem.

Now for the code review

Notes:

  • I saw other responses told you you have bugs in your code, I didn't check your code for bugs. But please follow their advice.
  • This is my add-on above all that was already been told by others.

  1. The first impression the code looks good for its purpose. What I mean - the code is a small CRON script and therefore using a single file might be ok for the use-case. Although code tends to grow or even infrastructure might change.

  2. The code is broken into meaningful functions and is easy to read. and it's ok because its a small CRON job otherwise another structure was required.

    • For example the non-readable parts are some of the boilerplate code that is scattered around which is the misfortune of a single file.
    • Another example of such misfortune is not needing to be object-oriented and have a clear responsibility structure embedded which makes the code less readable.
    • Yet another - There is no sense of layers in your code, so it's just simple progressive programming. If it was part of a bigger project, I would expect it to use existing DB layers or other existing infrastructure and may be part of a single monorepo and layers would have been a must and different restructuring.
  3. Good use of promise & await, but why not stick with one method? promisify the endpoints and keep the consistency of async/await from there on to the top level.

  4. very important, I read another person told you the same thing, you didn't handle catch-all exception in one location.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.