GoHttp icon indicating copy to clipboard operation
GoHttp copied to clipboard

Scan should return output length

Open ztbrown opened this issue 8 years ago • 7 comments

this change will make scan return the length of the output string. Closes #13

ztbrown avatar Jul 25 '17 02:07 ztbrown

Does this change the overall behaviour?

fekberg avatar Jul 25 '17 06:07 fekberg

I can see two places where I think it changes the behavior for the better: getHttpVersion and handleHttpGET.

On line 286 and 287 of getHttpVersion, we do this:

    int start = scan(input, filename, 4, 100);
    if ( start > 0 ) {
        ...
    }

Unless the start index is greater than strlen(input), the conditional will always be true, even if filename is empty. I would think that, if filename, which in this case is holding the HTTP version, is empty, getHttpVersion should immediately return a fail code.

On line 377 of handleHttpGET, we do this:

    fileNameLenght = scan(input, filename, 5, 200);
        if ( fileNameLenght > 0 )
        {
            ...
        }

This suffers from the same problem as handleHttpGET and is misleading to anyone who might be reading it for educational purposes. The variable says that it is holding the fileNameLength, when it is actually holding the length of the beginning of the input to the end of the output.

My solution isn't great, now that I'm looking at it again. In the spirit of being readable and exemplary, I should have returned strlen(output). I will make that update now.

ztbrown avatar Jul 25 '17 13:07 ztbrown

Hmm. Then, the following would not be needed, right?

Look at the lines above it:

	i += 1;

	for (; i < strlen(input); i ++ )
	{
		if ( *(input + i ) != '\t' && *(input + i) != ' ' && *(input + i) != '\n' && *(input + i) != '\r')
			break;
	}

If it finds a tab, whitespace or a newline, it returns the amount of characters until that new segment.

Which is pretty much the same as doing strlen(output), right?

Just make sure it's working the same way!

fekberg avatar Jul 25 '17 13:07 fekberg

That is correct. I removed that code in my project and my test is still passing:

https://github.com/ztbrown/Porter/blob/scan-tests/src/http_utils.c#L10

https://github.com/ztbrown/Porter/blob/scan-tests/tests/http_utils_test/http_utils_test.c#L169

ztbrown avatar Jul 25 '17 13:07 ztbrown

Excellent, have you tried using a browser as well?

Could you remove the dead code from this PR as well please?

fekberg avatar Jul 25 '17 13:07 fekberg

Well, this is currently breaking stuff. I'll take a look later and find out why that's happening.

ztbrown avatar Jul 25 '17 15:07 ztbrown

Ah, this is breaking on checkMime. Looking into it.

ztbrown avatar Jul 25 '17 18:07 ztbrown