PPM图像的隐写

Question

我最近完成了一个关于P3图像速记的项目，如果我可以对我的代码做任何改进的话，我只想得到一些建议。我对C很陌生，所以不确定我所做的是否是最好的实践，所以我只是想学习！

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>

/*Using a Link list as the primary Data Structure */

/*Part A Structures*/

//Create the linked list
typedef struct NODE 
{
char *val;
struct NODE *next;
}NODE;

//Hold the Pixel of the image
typedef struct PIXEL 
{
 int R, G, B;
}PIXEL;

//Hold the PPM Image
typedef struct PPM 
{
char format[2];;
NODE *comments;
int w, h;
int max;
PIXEL * arr;
}PPM;

/**
* function used to deepcopy the linkedlist holding the comments.
* it returns a pointer to a deepcopy of the list passed as a parameter;
*/
//Copy the linked list
//Points to the copy
NODE *copy(NODE *first) 
{
NODE *second = NULL, *previous = NULL;

while (first != NULL) 
{
NODE * temp = (NODE *) malloc(sizeof(NODE));
temp->val = first->val;
temp->next = NULL;

if (second == NULL) 
{
    second = temp;
    previous = temp;
} 
else 
{
    previous->next = temp;
    previous = temp;
}
first = first->next;
}
return second;
}


//Copy the PPM File Returning Pointer
struct PPM * createPPM(PPM *old) 
 {
PPM *new = (PPM*) malloc(sizeof(PPM));

strcpy(new->format, old->format); 
new->comments = copy(old->comments); 

new->h = old->h;
new->w = old->w;
new->max = old->max;

new->arr = (PIXEL *) malloc(old->h * old->w * sizeof(PIXEL));
memcpy(new->arr, old->arr, old->h * old->w * sizeof(PIXEL));

return new;
}

/*Part B */


//Function to read a PPM file and store the structure
struct PPM * getPPM(FILE * fd) 
{
PPM *image = (PPM *) malloc(sizeof(PPM));

fscanf(fd, "%s", image->format);
if (strcmp(image->format, "P3") != 0) 
{
printf("Invalid Image Type");
exit(0);
}
char c = getc(fd);
image->comments = (NODE *) malloc(sizeof(NODE));
NODE *temp = image->comments;
 while ((c = getc(fd)) == '#') 
{
fseek(fd, -1, SEEK_CUR); 
char str[50];
fgets(str, 50, fd);
temp->val = (char *) malloc(strnlen(str, 50));
strcpy(temp->val, str);
temp->val[strlen(temp->val) - 1] = 0;
temp->next = (NODE *) malloc(sizeof(NODE));
temp = temp->next;
temp->next = NULL;
}

fseek(fd, -1, SEEK_CUR);
fscanf(fd, "%d", &image->w);
fscanf(fd, "%d", &image->h);
fscanf(fd, "%d", &image->max);

image->arr = (PIXEL *) malloc(image->h * image->w * sizeof(PIXEL));

int t = 0;
int j = 0;

while (j < image->h * image->w)
{
t = fscanf(fd, "%d", &image->arr[j].R);
t = fscanf(fd, "%d", &image->arr[j].G);
t = fscanf(fd, "%d", &image->arr[j].B);
j = j + 1;
}

return image;
}

//Encode The message into the PPM file
struct PPM * encode(char * text, struct PPM * i) 
{
PPM * str = createPPM(i); 
int random;
srand((unsigned) time(NULL));
int randMax = (i->h * i->w) / (strlen(text)+1);

random = rand() % randMax; 

if(random<1)
{
random = 1; 
}

int k =0;
int j = random;

//Red fields swapped with ASCII int
while (k < strlen(text)) 
{
if(str->arr[j].R == text[k])
{
    j = j+1; // if the values are the same we encode in the next  pixel.
}
else
{
str->arr[j].R = text[k]; 
k = k+1;
j = j + random;
}
}

return str;
}

//Function Decodes the messages comparing the new copy to the original
char * decode(struct PPM * i1, struct PPM * i2) 
{
int i = 0;
int j = 0;

char *str = (char *) malloc(255);

while (i < i1->h * i1->w)
{
if(i1->arr[i].R != i2->arr[i].R)
{
str[j] = i2->arr[i].R;
j = j+1;
}
i = i + 1;
}

str = realloc(str,i);

return str;
}

/**
* function to print the PPM structure
*/
void showPPM(struct PPM * i) 
{
printf("%s\n", i->format); //print format

//print comments
NODE *n = i->comments;
while (n->next != NULL)
{
printf("%s\n", n->val);
n = n->next;
}

//print width, height and max
printf("%d %d\n%d\n", i->w, i->h, i->max);

//print the array containing the pixels
int j;
for (j = 0; j < i->h * i->w; ++j) 
{
printf("%d %d %d\n", i->arr[j].R, i->arr[j].G, i->arr[j].B);
}

return;
}

int main (int argc, char ** argv)
{

if(argc != 3 && argc != 4)
{ 
printf("Encode 3 arguments, Decode 4\n");
exit(0);
}

else if(argc ==3)
{   
if(strncmp(argv[1], "e",1) != 0)
{ 
    printf("non valid commands %s for %d arguments\n", argv[1], argc);
    exit(0);
}

FILE * f = fopen(argv[2], "r");
PPM * ppm1 = getPPM(f);
fclose(f);
char * toEnc =(char*) malloc(255 * sizeof(char));
fgets (toEnc, 255, stdin);
PPM * encoded = encode(toEnc, ppm1);
showPPM(encoded);
}

else
{
if(strncmp(argv[1], "d",1) != 0)
{
    printf("non valid commands %s for %d arguments\n", argv[1], argc);
    exit(0);    
}
FILE * file1 = fopen(argv[2], "r");
FILE * file2 = fopen(argv[3], "r");
PPM * ppm1 = getPPM(file1);
PPM * ppm2 = getPPM(file2);
char * dec = decode(ppm1, ppm2);
printf("%s", dec); 
}

}

Answer 1

最好将struct的定义与typedef的定义分开。

所以这个：

typedef struct NODE
{
    char *val;
    struct NODE *next;
}NODE;

最好写成：

struct NODE
{
    char *val;
    struct NODE *next;
};

typedef struct NODE NODE;

类似的考虑也适用于PIXEL和PPM，以提高可读性和易于理解：

1. 通过空行分离代码块(for、if、else、while、do...while、switch、case、default)。
2. 用2或3行空白行分隔函数(保持一致)
3. 始终缩进代码：
   • 在每个开口支撑{ '{‘之后缩进
   • 在每一个闭锁括号'}‘之前缩进
   • 建议每个缩进级别为4个空格，因为宽度足够宽，即使使用可变宽度的字体也可以看到。

为了确保对系统函数的调用成功，请检查返回的值:当调用：malloc()、fscanf()、realloc()m、fopen()、fseek()以避免内存泄漏时，对于每个对堆内存分配函数的调用(对于realloc()来说稍微复杂一些)，需要为相同的内存指针调用free()。

函数srand()只应在整个程序中调用一次。建议尽早在srand()函数中调用main()。

当由于错误退出程序时，不要使用返回值0，因为这是用来表示成功的。建议使用EXIT_FAILURE (在stdlib.h头文件中定义)。当使用==运算符进行比较时，最好将文字放在左边，因此编译器(else if( 3 == argc ))会捕获输入=而不是==之类的击键错误。

在输出错误消息时，应该将其输出到stderr，而不是stdout，因此请使用以下两种方法：

fprintf( stderr, "...");

或者，如果是由于系统功能，则使用：

perror "..." );

perror()还将输出来自操作系统的文本，说明操作系统认为错误发生的原因。

内存分配函数(malloc、realloc、calloc)返回具有void*类型的指针，因此可以分配给任何其他指针。转换返回的值只会使代码混乱，使理解、调试和维护变得更加困难。

PPM图像必须在每行上有4个字节的倍数，所以每行的末尾通常都有一些“虚拟”字节，所发布的代码没有考虑到这种情况。

函数getc()返回一个int，而不是一个char，应该始终检查返回的值是否为EOF。

编译时，始终启用所有警告，然后修复这些警告。(对于gcc，至少要使用：-Wall -Wextra -pedantic。我还使用：-Wconversion -std=gnu11)。

变量和参数名称应该指示content或usage (或者两者都更好)。像i这样的参数名称是没有意义的，即使在当前上下文中也是如此。

关于这一行：

temp->val = (char *) malloc(strnlen(str, 50));

函数strnlen()将不给尾部NUL字节留出足够的空间，因此行应该是：

temp->val = malloc(strnlen(str, 50)+1);

这一行：

temp->val[strlen(temp->val) - 1] = 0;

将截断字符串的最后一个字符，而这一行根本不需要，因为先前对strcpy()的调用将包含一个尾部NUL字节。

有关struct中的此字段：

char format[2];;

设置该字段的方法将包括一个尾部NUL字节，甚至可能包括一个换行符(代码应该用NUL覆盖换行符)，因此行应该是：(包括删除额外的;)。

char format[3];

有更多的问题，但以上应该让你开始。

#endif





#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>

/*Using a Link list as the primary Data Structure */

/*Part A Structures*/

//Create the linked list
struct NODE
{
    char *val;
    struct NODE *next;
};
typedef struct NODE NODE;

//Hold the Pixel of the image
struct PIXEL
{
    int R;
    int G;
    int B;
};
typedef struct PIXEL PIXEL;

//Hold the PPM Image
struct PPM
{
    char format[2];;
    NODE *comments;
    int w, h;
    int max;
    PIXEL * arr;
};
typedef struct PPM PPM;


#if 0
The following function does not seem to actually perform a deep copy,
suggest looking carefully at the logic.
#endif

/**
* function used to deepcopy the linkedlist holding the comments.
* it returns a pointer to a deepcopy of the list passed as a parameter;
*/
//Copy the linked list
//Points to the copy
NODE *copy(NODE *first)
{
    NODE *second = NULL;
    NODE *previous = NULL;

    while (first != NULL)
    {
        NODE * temp = malloc(sizeof(NODE));
        if( !temp )
        {
            perror( "malloc failed" );
            // need logic here to pass all heap pointers to 'free()'
            exit( EXIT_FAILURE );
        }

        // implied else, malloc successful

        temp->val = first->val;
        temp->next = NULL;

        if (second == NULL)
        {
            second = temp;
            previous = temp;
        }

        else
        {
            previous->next = temp;
            previous = temp;
        }

        first = first->next;
    }
    return second;
} // end function: copy