A587929f80f37c163f855f5610f2aa0c

Hi All,

I was given this code on a test. My solution was to pass a pointer to the buffer and adjust the shift values. I actually got the program to work according to spec (without changing the original input buffer), but was told that my solution was rubbish, so I'd appreciate another set of eyes to look at this and suggest their solution(s).

The requirements are as follows:

Fix the following piece of broken code. When executed, it should convert the buffer parameter (using the provided data -- changing the contents of the input buffer doesn't count as fixing a bug) into the string "Art&Logic".

Regards

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
#define kSpace 032

char buf[] = {0x42, 0x73, 0x75, 0x27, 0x13, 0x1C, 0x68, 0x1B, 0x64};

#define mBufferLen(buf) (sizeof(buf) / sizeof(buf[0]))


/*
 * convert the contents of 'buf' in place so that they are decoded. The
 * above buffer should convert to the ASCII string "Art&Logic" (but it doesn't as
 * presented here....)
 */
void Convert(char* buffer)
{
   int i;

   for (i = 0; i <= mBufferLen(buffer); ++i)
   {
      if (buffer[i] > kSpace)
      {
         buffer[i] -= 1;
      }
      else
      {
         buffer[i] = buffer[i] << 2 + 4 - i;
      }
   }
}

int main(void)
{
   int i;
   const char kTarget[] = "Art&Logic";

   Convert(buf);
   for (i = 0; i < kBufferLen; ++i)
   {
      assert(kTarget[i] == buffer[i]);
   }
}

Refactorings

No refactoring yet !

D1588981e0248aaa0174906c99df180e

Andy Lester

October 14, 2007, October 14, 2007 17:00, permalink

1 rating. Login to rate!

I suspect you don't really want to be calling mBufferLen(buffer) every time through the loop. Assign it to a temporary variable first, and then use that variable in the loop condition.

52feb5a1f3175e3706444ca360fb1271

Nathan Sanders

October 15, 2007, October 15, 2007 01:52, permalink

No rating. Login to rate!

Look at the definition of kSpace. I think this is one of those 'change one character' puzzles.

A587929f80f37c163f855f5610f2aa0c

Mike

October 15, 2007, October 15, 2007 11:12, permalink

No rating. Login to rate!

Thanks for the "refactorings" so far. As for Andy's comment, yes, you're correct, I saw that, but I'm looking for more spec related issues than performance issues. Thanks again.

7bfd646dea8e47642bbb573f026bf159

engtech

October 17, 2007, October 17, 2007 01:50, permalink

1 rating. Login to rate!

Step #1: hex vs dec... what is kspace in hex?

Step #2: put a comment beside buffer with the hex values of Art&Logic so you know what your goal is

I can see that your solution is wrong ampersand is greater than kSpace but it's only a -1 conversion.

1
2
3
4
5
6
7
8
#define kSpace 0x20

char buf[] = {0x42, 0x73, 0x75, 0x27, 0x13, 0x1C, 0x68, 0x1B, 0x64};

// http://asciichart.com/ascii_hexadecimal.html
//    A,    r,    t,    &,     L,     o,     g,     i,     c
// 0x41, 0x72, 0x74, 0x26,  0x4c,  0x6f,  0x67,  0x69,  0x63
// -0x1, -0x1, -0x1, -0x1, +0x30, +0x03, -0x01, +0x4E, -0x01
7bfd646dea8e47642bbb573f026bf159

engtech

October 17, 2007, October 17, 2007 01:51, permalink

No rating. Login to rate!

if kSpace was supposed to be 0x32 how would that change your solution?

A587929f80f37c163f855f5610f2aa0c

Mike

October 17, 2007, October 17, 2007 02:38, permalink

No rating. Login to rate!

Thanks, engtech, that's exactly what I did in my solution. Again, I already "solved" it, I was simply told that the solution was "weak" on the verge of wrong; so I was looking for other unadulterated solutions as opposed to posting what I did.

848e7681373328946b4b7ccb3a537627

jaredgrubb

October 29, 2007, October 29, 2007 15:44, permalink

No rating. Login to rate!

A comment on Andy Lester's solution: There is no need to factor the condition check out of the for loop. mBufferLen(buf) is a macro function, so the compiler will text-replace the function code directly into the for loop check -- and the result will be exactly like a check against a constant.

Avatar

Steve

November 5, 2007, November 05, 2007 20:54, permalink

No rating. Login to rate!

#define mBufferLen(buf) (sizeof(buf) / sizeof(buf[0]))

This macro only works properly when used on arrays.
When used inside the function and called on the input parameter,
a pointer, it doesn't do what you want.

sizeof( char* ) / sizeof( char ) -> depends on the architecture, but probably 4.
The function needs to have a buffer size declared with it, and then you can call that function on the actual array from main.
Convert( buf, mBufferLen( buf ) );

You could avoid the buffer size requirement if there was some kind of string terminator to look for, but otherwise you need it.

Avatar

Steve

December 19, 2007, December 19, 2007 15:38, permalink

No rating. Login to rate!

buffer[i] = buffer[i] << 2 + 4 - i;

Additionally, << and >> bind last:
x = 1 << 2 + 3; is 32, not 7, your code should be rewritten as:

buffer[i] = ( buffer[i] << 2 ) + 4 - i; or
buffer[i] = buffer[i] * 4 + 4 - i; ... but you should use parenthesis to help make it more readable

Avatar

chrome

January 11, 2008, January 11, 2008 23:34, permalink

No rating. Login to rate!

What a horribly contrived "bug". No wonder most people hate C with lecturers like that.

Your refactoring





Format Copy from initial code

or Cancel