Downloading images from a forum

Question

I want to improve this program so I want you to criticize the code, and tell me what else can I do, and where can I find information about what I have to do.

TODO:

• Fix wget timeout

• The code should always check the returned value from any of the malloc family of functions to assure to memory allocation was successful

• the code needs to check the returned value from fopen() to assure the operation was successful

• The wget command can fail, so the wget line should route stderrto a file, then after system(cmd) should open/read that file to assure the wget was successful

• The main() function will not compile cleanly because the declaration (as it should) indicates are int returned value, but no return (someint) line exists in the execution paths

• In the download() function, what is the magic number '151' about? Better to #define that number, with a descriptive name and a comment

• Eegarding the makefile:

  1. cc := /usr/lib/gcc
  2. Globbing all the compiles and the link into a single line is not a good nor flexible plan. Suggest breaking out into a compile rule from one .c to one .o and then using the list of .o files on a separate rule that performs the link.
  3. The 'all' and 'clean' do not produce a file with those names. It suggests adding the line: .PHONY: all clean

#include <stdio.h>
#include <stdlib.h> // for using system calls
#include <stdbool.h>
#include <string.h> // for strlen

char  postBegin[] = "forum-post-body-content", postEnd[] = "p-comment-notes", img[] = "img src=";
int length1 = 23, length2 = 15, length3 = 8;
int pos1 = 0, pos2 = 0, pos3 = 0;

void downloadAndOpen (FILE **fp, int i, char *file);
bool search (char needle[], int length, char c, int *pos);
void download (FILE* *fp);

int main ()
{
    bool inPost = false;
    FILE *fp;
    int c;
    char file[20];

    for ( int i = 22; i <= 151; i++ )
    {
        downloadAndOpen (&fp, i, file);
        while ( (c = getc (fp)) != EOF ) {
            if ( search (postBegin, length1, c, &pos1) )
                inPost = true;
            if (inPost) {
                if ( search (postEnd, length2, c, &pos2) )
                    inPost = false;
                if ( search (img, length3, c, &pos3) )
                    download (&fp);
            }
        }
        fclose (fp);
        remove (file);
    }
}

void downloadAndOpen (FILE **fp, int i, char *file)
{
    char cmd[200]={0};
    // download web page
    sprintf (cmd, "wget -q -O page%d.txt 'http://www.mtgsalvation.com/forums/creativity/artwork/340782-official-digital-rendering-thread?page=%d'", i, i);
    system (cmd);

    // open text file
    sprintf (file, "page%d.txt", i);
    *fp = fopen (file, "r");
}

bool search (char needle[], int length, char c, int *pos)
{
    if (needle[*pos] == c)
    {
        if (*pos == length - 1)
        {
            return true;
            *pos = -1;
        }
        (*pos)++;
    }
    else
    {
        if(*pos > 0)
        *pos = 0;
    }
    return false;
}

void download (FILE **fp)
{
    char url[300], cmd[300];
    static int imageNumber = 496; // The image number where I left last time
    int pos = 0, c, j;
    bool found = false;

    while (!found)
    {
        if ( (c = getc (*fp)) == EOF ) {
            printf ("Image not found\n");
            return;
        }
        printf ("%c", (char) c);
        url[pos] = (char) c;
        if ( url[pos-1] == '\"' && url[pos] == '\"' )
            break;
        if ( url[pos-1] == '\"' && url[pos] == '>' )
        {
            printf ("\n");
            found = true;
        }
        ++pos;
    }
    pos -= 2;
    char url2[pos];
    for ( j = 1; j < pos; j++ )
    {
        url2[j - 1] = url[j];
    }
    url2[j - 1] = '\0';
    //http://joequery.me/code/snprintf-c/
    // wget -q for quiet -nc, --no-clobber   skip downloads that would download to existing files (no sobreescribir)
    snprintf(cmd, 300, "wget -q -nc --timeout=10 -O /home/arturo/Dropbox/Digital_Renders/%d \'%s\'", imageNumber++, url2);
    system(cmd);
    pos = 0;
}

Answer 1

Initialize all variables: fp, c, file etc.

your downloadAndOpen function has some issues

You do a system call to download (asynchronous call), but you do no attempt to wait for it to finish - instead you directly try to open it. If for some reason the download takes longer then it will fail. If your function fails you get a crash since you do not handle the return value of fopen.

I think the better approach would be to split it in two functions, one which does the download and returns when the file is there, alt. timeout with error code. Second part to open the file and returning the file pointer.

E.g.

int downloadFile( int pageNumber ); // return 1 - file downloaded 0 - failed
FILE* openFile( int pageNumber ); // NULL - file not found

Then add some checks to make sure they succeeded before you proceed.

if (downloadFile(pageNumber))
{
  FILE* fp = openFile(pageNumber);
  if (fp != NULL)
  {
   ...

You are a bit inconsistent with your if statements, sometimes you use braces sometimes not, sometimes the starting brace is on the same line as the if statement sometimes under the if. It makes the code harder to read. Choose one way and stick with that.

I am also not sure why you pass FILE** fp to download(), since you do not open some other file in there it is enough to pass the file pointer itself download(FILE* fp)

Global variables; bad, especially if they are not really used by several functions e.g. length1 is only used in main().

Comments; good to have. especially when I read such a line

for ( int i = 22; i <= 151; i++ )

It would be interesting to know why you start at 22.

EDIT:

I see you have some of my comments already in your todo list, anyway I will keep them here as well.