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;

bool search (char needle[], int length, char c, int *pos);

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

for ( int i = 22; i <= 151; i++ )
{
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) )
}
}
fclose (fp);
remove (file);
}
}

{
char cmd[200]={0};
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;
}

{
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 ) {
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/
snprintf(cmd, 300, "wget -q -nc --timeout=10 -O /home/arturo/Dropbox/Digital_Renders/%d \'%s\'", imageNumber++, url2);
system(cmd);
pos = 0;
}

• It is much easier with a shell script: i=1; while [ $i -le 150 ]; do wget -q -O- http://www.mtgsalvation.com/forums/creativity/artwork/340782-official-digital-rendering-thread?page=$i | sed -n '{/forum-post-body-content/,/p-comment-notes/p}' | grep -Po 'src="\K[^"]+' | xargs wget -q -T 6 -t 1; rm page$i; i=$[\$i+1]; done – ajr-dev Dec 26 '14 at 15:20

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


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().

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