从论坛下载图片

Question

我想改进这个程序，所以我想让你批评代码，告诉我还能做什么，在哪里可以找到关于我必须做的事情的信息。

TODO：

• 修正wget超时
• 代码应该始终检查任何malloc系列函数的返回值，以确保内存分配是成功的。
• 代码需要检查fopen()返回的值，以确保操作成功。
• wget命令可能失败，因此wget行应该将stderr路由到一个文件，然后system(cmd)应该打开/读取该文件，以确保wget成功
• main()函数不会干净地编译，因为声明(正如它应该的那样)表示int返回值，但是在执行路径中不存在返回(someint)行。
• 在download()函数中，魔术数字'151‘是关于什么的？最好用描述性的名称和注释来定义这个数字。
• 执行makefile：
  1. cc := /usr/lib/gcc
  2. 将所有的编译和链接合并成一行并不是一个好的或灵活的计划。建议将编译规则从一个.c分解为一个.o，然后在执行链接的单独规则上使用.o文件列表。
  3. “all”和“clean”不会生成带有这些名称的文件。它建议添加一行：.PHONY: all洁净

#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

初始化所有变量：fp、c、file等。

您的downloadAndOpen函数有一些问题

您可以执行一个system调用来下载(异步调用)，但并不试图等待它完成-而是直接尝试打开它。如果由于某种原因下载时间较长，那么它将失败。如果您的函数失败，则会出现崩溃，因为您不处理fopen的返回值。

我认为更好的方法是将其分成两个函数，一个函数负责下载，在文件存在时返回alt。错误代码超时。第二部分打开文件并返回文件指针。

例如。

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

然后添加一些检查，以确保它们在您继续之前成功。

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

您与您的if语句有些不一致，有时使用大括号有时不使用，有时起始大括号与if语句在同一行，有时在if下面。这会使代码更难读。选择一条路，坚持下去。

我也不知道为什么要传递FILE** fp来下载()，因为您没有在其中打开其他文件，所以只需传递文件指针本身download(FILE* fp)就足够了。

全局变量；糟糕，特别是如果几个函数没有真正使用它们，例如length1只在main()中使用。

评论；好的。尤其是当我读到这样的一句话

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

我很想知道你为什么从22岁开始。

编辑：

我看你的待办事项清单上已经有我的一些评论了，反正我也会把它们保留在这里。