I have the following HTTP reply saved in a local file called source.txt:
HTTP/1.1 301 Moved Connection: close Content-length: 111 Location: https://11.12.13.14:81/ Content-type: text/html; charset="utf-8" <html><head><META HTTP-EQUIV="refresh" CONTENT="0;URL=https://11.12.13.14:81/"></head><body></body></html>
Source code:
#include <stdio.h>
#include <stdlib.h>
#define MAXBUFLEN 1024
char* getLocation(char* source)
{
const char *p1 = strstr(source, "Location:")+10;
const char *p2 = strstr(p1, "\n");
size_t len = p2-p1;
char *res = (char*)malloc(sizeof(char)*(len+1));
strncpy(res, p1, len);
res[len] = '0円';
return res;
}
char* getData(char* source)
{
const char *p1 = strstr(source, "://")+3;
const char *p2 = strstr(p1, "\n");
size_t len = p2-p1;
char *res = (char*)malloc(sizeof(char)*(len+1));
strncpy(res, p1, len);
res[len] = '0円';
return res;
}
int main()
{
char source[MAXBUFLEN];
char host[100];
int port;
FILE *fp = fopen("source.txt", "r");
if (fp != NULL) {
size_t newLen = fread(source, sizeof(char), MAXBUFLEN, fp);
if (newLen == 0) {
fputs("Error reading file", stderr);
} else {
source[++newLen] = '0円';
//extraction code
char* res = getLocation(source);
printf("getLocation result: %s\n", res);
if (strstr(res, "://"))
{
res = getData(source);
printf("getData result: %s\n", res);
if (strstr(res, ":"))
{
sscanf(res, "%[^:]:%d[^/]", host, &port);
printf("host: %s | port: %d\n", host, port);
}
else
printf("delimiter not found\n");
}
else
printf("no link\n");
//
}
}
fclose(fp);
}
The program is working well, but it's very ugly. Is there any way to improve the code to avoid doing so many operations? I mean somehow merging the two functions, getLocation() and getData().
-
\$\begingroup\$ What about at least a minimal description on what this functions should do? \$\endgroup\$alk– alk2013年09月07日 16:15:44 +00:00Commented Sep 7, 2013 at 16:15
-
\$\begingroup\$ you really need a description for that short code? \$\endgroup\$Sam Reina– Sam Reina2013年09月08日 12:35:42 +00:00Commented Sep 8, 2013 at 12:35
-
\$\begingroup\$ Please excuse, I do not need nothing, not even to read your posting. I just tried to kindly mention that an introduction to an issue, raises the chances of keeping a possible reader attracted. \$\endgroup\$alk– alk2013年09月08日 12:37:27 +00:00Commented Sep 8, 2013 at 12:37
1 Answer 1
Your getLocation can be improved a little:
For a start, the call parameter should be const (although if you were happy
modifying the string you could avoid the memory allocation).
Moving on:
const char *p1 = strstr(source, "Location:")+10;
This is ok, except that p1 is cryptic and it is better to avoid embedded constants such as 10. Since this is the length of "Location:", we can extract that:
#define LOC "Location:"
const char *loc = strstr(source, LOC) - 1 + sizeof LOC;
Note the -1 for the 0円 at the end of LOC. But it is now uglier still. And of course it might fail, returning NULL. So we need to protect the next line:
const char *p2 = strstr(p1, "\n");
in which you are looking for just one character; strchr is more appropriate and eol is a more descriptive name:
if (loc) {
const char *eol = strchr(loc, '\n');
}
So next you get the length
size_t len = p2-p1;
which needs a cast to avoid a sign-conversion warning:
size_t len = (size_t) (eol - loc);
You allocate memory next:
char *res = (char*)malloc(sizeof(char)*(len+1));
but you should omit the cast and note that sizeof(char) is 1 by definition, so:
char *res = malloc(len + 1);
and of course allocation could also fail, so you must check before copying the address into it. You copy the address thus:
strncpy(res, p1, len);
res[len] = '0円';
but as you already know the length of the string, memcpy would be better, as
strncpy must check each char in the input string for 0円
if (res) {
memcpy(res, loc, len);
res[len] = '0円';
}
Putting it all together, we get:
static char* getLocation(const char* source)
{
#define LOC "Location:"
size_t len = 0;
char *eol = NULL;
const char *loc = strstr(source, LOC) - 1 + sizeof LOC;
if (loc) {
eol = strchr(loc, '\n');
}
if (eol && loc) {
len = (size_t) (eol - loc);
}
if (len) {
char *res = malloc(len + 1);
if (res) {
memcpy(res, loc, len);
res[len] = '0円';
}
return res;
}
return NULL;
}
Yes, string handling in C is ugly.
My conclusion from this ugliness is that another approach is needed. Why not
modify the input string by putting a 0円 at eol and returning loc? Then
you could extract the address and port directly from that:
static char* getLocation(char* source)
{
#define LOC "Location:"
char *loc = strstr(source, LOC) - 1 + sizeof LOC;
if (loc) {
char *eol = strchr(loc, '\n');
if (eol) {
*eol = '0円';
}
}
return loc;
}