2
\$\begingroup\$

I wrote this script and got what I wanted from it. However I'm looking for input on ways I can improve the function.

My goal was to scrape a bunch of image links from a website I'd like to rebuild. I wanted to use webscraper but the image links were coded into the CSS instead of the HTML mark up.

Hence I had difficulty getting the web scrapper to find and collect said links. Luckily while looking at them, I was able to discern their structure and pattern into 4 formats and came up with the following script to duplicate and iterate through each format to get my links.

Beside the 1st group (which is comprised of 40 images), each image (n) is part of a group (x) of 12 and there are 170 groups in total. Something I had to consider while writing the script is that each image number starts with a "00" prefix and "000" for the group number. So they had to be removed as you increment through the tens(for images) and hundreds(for groups).

let loop = 0;
for (; loop < 155; loop++) {
let n = 00;
let x = 000;
str1 = "https://site.com/content/2018/02/"; /* groups 1-155*/
str2 = "https://site.com/content/2019/07/"; /* groups 156-167*/
str3 = "https://site.com/content/2019/11/"; /* group 168*/
str4 = "https://site.com/content/2020/03/"; /* groups 169-171*/
ext = ".jpg"
z = "0";
zz ="00";
        if (loop > 0 ) {
        x+=loop;
    console.log('one loop ' + loop );
    if(x >= 9){
        var zz = "00".replace('00','0');
    }
    if(x >= 99){
        var zz = "00".replace('00','');
    }
  }
      while (n <= 11) {
        if(n >= 9){
        var z = "0".replace('0','');
        }
        n++;  
    console.log(str1 + zz + x + "_" + z + n + ext);
  }
}
\$\endgroup\$
  • \$\begingroup\$ You never use str1, str2, or str3. Is that intentional, or did you accidentally leave out some code? \$\endgroup\$ – CertainPerformance May 17 at 18:45
  • \$\begingroup\$ Now you aren't using str2, str3, or str4..? \$\endgroup\$ – CertainPerformance May 17 at 18:58
  • \$\begingroup\$ I wanted to run the function on one group at a time, your question made me realize I had to switch to str1 for my question, thanks. \$\endgroup\$ – a.bizi May 17 at 19:02
2
\$\begingroup\$

You should consider using consistent indentation when writing code - it makes it much easier for people to read (not only for other maintainers, but for you as well). For example

let loop = 0;
for (; loop < 155; loop++) {
let n = 00;
// ...
zz ="00";
        if (loop > 0 ) {
        x+=loop;
    console.log('one loop ' + loop );

should be something like

let loop = 0;
for (; loop < 155; loop++) {
  let n = 00;
  // ...
  zz ="00";
  if (loop > 0 ) {
    x+=loop;
    console.log('one loop ' + loop );

Lots of IDEs provide an automatic tidying function.

In this section, since you aren't using str2, str3, or str4, you may as well remove those assignments from the inside of the loop here, since they aren't used and only add to cognitive overhead.

Best to always declare variables before you use them - you have a number of variables which are not declared, which will either assign to properties of the global object (in sloppy mode), or will throw an error (in strict mode). (Consider always using strict mode so that these sorts of potential bugs can be fixed early.) Specifically, lines like:

str1 = "https://site.com/content/2018/02/"; /* groups 1-155*/

where you use a variable for the first time should be prefixed with const, let, or var:

const str1 = "https://site.com/content/2018/02/"; /* groups 1-155*/

If you're going to use ES2015 syntax to declare some variables like let (which is great, you should), don't use var anywhere; var has numerous potential gotchas which can cause bugs, and doesn't really have anything positive going for it (other than compatibility with ancient browsers - but if you need that sort of compatibility, use Babel to transpile to ES5 automatically - best to keep source code in readable, pretty, modern syntax).

When you have a for loop, if you need to declare a variable at the beginning that's then used inside that loop, you should put the declaration inside the for loop declaration. That is:

let loop = 0;
for (; loop < 155; loop++) {

should be

for (let loop = 0; loop < 155; loop++) {

You have

let n = 00;
let x = 000;

Leading zeros have no effect in numeric literals. Those are equal to

let n = 0;
let x = 0;

You declare the zz variable 3 times, but you only have one binding for it - duplicate variable declarations with var are ignored. If you just want assignment, then only use the assignment operator with =.

The x variable starts at 0 and gets loop assigned to it once, on every iteration:

let x = 0;
// ...
if (loop > 0) {
  x += loop;

So, it's equivalent to loop. Best to just remove that variable completely, since it isn't doing anything useful - use the one loop variable completely. The if (loop > 0) { test is superfluous too, since the tests inside that block if(x >= 9){ if(x >= 99){ are only fulfilled if loop is greater than 0 anyway.

Use meaningful variable names. It is very difficult to tell at a glance what loop, n, x, z, or zz are meant to represent. Maybe call them group (the number that goes from 1 to 155) and subgroup (the number that goes from 1 to 11) instead.

The main block there is meant to ensure that the zz variable for padding the string has the right number of zeros. But there's a much easier way to do that, without creating extra variables: just use String.prototype.padStart. When you need to construct

000_01
154_12

from variables 0 and 1, and from variables 154 and 12, add the leading zeros by calling padStart(length, '0') on them:

`${String(group).padStart(3, '0')}_${String(subgroup).padStart(2, '0')}`

Or, without a template literal:

String(group).padStart(3, '0') + '_' + String(subgroup).padStart(2, '0')

The loop over subgroups (or n in your original code) assigns a variable and increments it every iteration, breaking after a condition. This is a prime spot to be refactored into a for loop instead:

for (let subgroup = 1; subgroup <= 12; subgroup++) {

Since the str1 never changes, and nor does the ext, you may as well declare them outside of the loop, rather than re-declare them every time there's another iteration:

In full:

'use strict';

const str1 = "https://site.com/content/2018/02/"; /* groups 1-155*/
const ext = ".jpg"
for (let group = 0; group < 155; group++) {
  console.log('one loop ' + group);
  for (let subgroup = 1; subgroup <= 12; subgroup++) {
    console.log(`${str1}${String(group).padStart(3, '0')}_${String(subgroup).padStart(2, '0')}${ext}`);
  }
}

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Wow this is beautiful! Apologies I wrote the script in a JS fiddle, will definitely check formatting before posting next time. As per your implementation, the padStart method is really what slimmed down the script. Thank you for this approach it makes so much sense and exposed my oversights. \$\endgroup\$ – a.bizi May 18 at 1:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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