11 Replies - 1155 Views - Last Post: 05 December 2012 - 09:52 PM

Poll: Readability Choice (8 member(s) have cast votes)

Is a Recursive Clear to read

  1. Yes, when used correctly (7 votes [87.50%] - View)

    Percentage of vote: 87.50%

  2. No, it is never clear (0 votes [0.00%])

    Percentage of vote: 0.00%

  3. In between - only sometime clear (1 votes [12.50%] - View)

    Percentage of vote: 12.50%

Vote Guests cannot vote

#1 code_m  Icon User is offline

  • D.I.C Head
  • member icon

Reputation: 24
  • View blog
  • Posts: 202
  • Joined: 21-April 09

Readability Choice

Posted 12 July 2011 - 04:38 PM

At work I needed to come up with a clear way (in syntax) to take a given byte array, look for a given byte value and add it with the next value in the array, then remove that value to the right from the array.

I came up with a recursive function that I think is clear:
private static byte[] JoinByteValues(byte[] message, byte valueToJoin, int start)
{
    int joinIndex = Array.IndexOf<byte>(message, valueToJoin, start);

    if (joinIndex > -1 && joinIndex < message.Length)
    {
        // copy to here, minus one byte
        byte[] shorten = new byte[message.Length - 1];

        // lets put the values back together at the index of joinIndex
        message[joinIndex] = (byte)(message[joinIndex] + message[joinIndex + 1]);

        // copy message to shorten, skipping the index [joinIndex + 1]
        Array.Copy(message, 0, shorten, 0, joinIndex + 1);
        Array.Copy(message, joinIndex + 2, shorten, joinIndex + 1, message.Length - joinIndex - 2);

        // recall this method with the shorten copy, to find any more joinIndex values
        return JoinBytesValue(shorten, valueToJoin, joinIndex + 1);
    }
    else
    {
        // no more bytes need joined
        return message;
    }
}


Today I also came up with this function without recursion, but I don't think it's any clearer:
private static void JoinByteValues(ref byte[] message, byte valueToJoin, int start)
{
    int joinCount = 0;

    for (int joinIndex = Array.IndexOf<byte>(message, valueToJoin, start);
             joinIndex > -1 && joinIndex < message.Length;
             joinIndex = Array.IndexOf<byte>(message, valueToJoin, start))
    {
        // add values back to together and store it at joinIndex
        message[joinIndex] = (byte)(message[joinIndex] + message[joinIndex + 1]);

        // start is the value we are 'dropping' out of the array
        start = joinIndex + 1;

        // move everything to the right of start left one index
        Array.Copy(message, start + 1, message, start, message.Length - start - ++joinCount);
    }

    if (joinCount > 0)
    {
        // Drop the empty indexes do the the movement of data
        Array.Resize<byte>(ref message, message.Length - joinCount);
    }
}


So I'm curious as to which way you would rather see it? Both functions pass my tests. (Also, speed is not a concern, I am working with a small data set, never more than 90 bytes. I am only interested to know which you prefer readability wise.)

Is This A Good Question/Topic? 0
  • +

Replies To: Readability Choice

#2 cfoley  Icon User is online

  • Cabbage
  • member icon

Reputation: 2002
  • View blog
  • Posts: 4,167
  • Joined: 11-December 07

Re: Readability Choice

Posted 13 July 2011 - 02:34 AM

Recursive or iterative, that code could be clarified by splitting it up into smaller methods. Please excuse the Java:

	byte[] joinByteValues(byte[] arr, byte value, int start) {
		int index = find(arr, value, start);
		// It's length-1 because if we find the value in the last element
		// there is no following element to add it to.
		if (index >= arr.length - 1) return arr;
		
		byte[] chopped = removeIndex(arr, index);
		chopped[index] += value;
		return joinByteValues(chopped, value, index);
	}

	// Returns the index of the value beginning with start
	// Or returns arr.length (i.e. an out of bounds index)
	int find(byte[] arr, byte value, int start) {
		for(int i = start; i < arr.length; i++) {
			if (arr[i] == value) return i;
		}
		return arr.length;
	}
	
	byte[] removeIndex(byte[] arr, int index) {
		byte[] ret = Arrays.copyOf(arr, arr.length-1);
		for(int i = index;i < ret.length; i++) {
			ret[i] = arr[i+1];
		}
		return ret;
	}


Was This Post Helpful? 0
  • +
  • -

#3 cfoley  Icon User is online

  • Cabbage
  • member icon

Reputation: 2002
  • View blog
  • Posts: 4,167
  • Joined: 11-December 07

Re: Readability Choice

Posted 13 July 2011 - 02:51 AM

Or my attempt at refactoring your code:

private static byte[] JoinByteValues(byte[] arr, byte value, int start) {
    int index = Array.IndexOf<byte>(arr, value, start);
    if (index > -1 && index < arr.Length){
        byte[] chopped = removeIndex(arr, index);
        chopped[index] += value;
        return JoinBytesValue(chopped, value, index + 1);
    } else {
        return arr;
    }
}

private static byte[] removeIndex(byte[] arr, int index) {
	byte[] ret = new byte[arr.Length - 1];
	Array.Copy(arr, 0, ret, 0, index);
	Array.Copy(arr, index + 1, ret, index, ret.Length - index);
	return ret;
}

Was This Post Helpful? 0
  • +
  • -

#4 cfoley  Icon User is online

  • Cabbage
  • member icon

Reputation: 2002
  • View blog
  • Posts: 4,167
  • Joined: 11-December 07

Re: Readability Choice

Posted 13 July 2011 - 02:59 AM

Incidentally, shouldn't this
return JoinBytesValue(chopped, value, index + 1);

be index, not index+1? What if the value you find is followed by a zero? Don't you want to remove that again and add it to the next one? For readability, it could now become:

private static byte[] JoinByteValues(byte[] arr, byte value) {
    int index = Array.IndexOf<byte>(arr, value);
    bool isFound = index > -1 && index < arr.Length;
    if (isFound){
        byte[] chopped = removeIndex(arr, index);
        chopped[index] += value;
        return JoinBytesValue(chopped, value);
    } else {
        return arr;
    }
}

private static byte[] removeIndex(byte[] arr, int i) {
	byte[] ret = new byte[arr.Length - 1];
	Array.Copy(arr,   0, ret, 0, i);
	Array.Copy(arr, i+1, ret, i, ret.Length - i);
	return ret;
}

Was This Post Helpful? 0
  • +
  • -

#5 code_m  Icon User is offline

  • D.I.C Head
  • member icon

Reputation: 24
  • View blog
  • Posts: 202
  • Joined: 21-April 09

Re: Readability Choice

Posted 05 December 2012 - 07:23 AM

View Postcfoley, on 13 July 2011 - 04:59 AM, said:

be index, not index+1? What if the value you find is followed by a zero?


The function is to un-escape a received byte message. That's the why the protocol works.

Sorry for digging up an old post :) Anyway, I went with the recursive definition. In the end it didn't matter much, only one other developer even ever saw the code.
Was This Post Helpful? 0
  • +
  • -

#6 lordofduct  Icon User is offline

  • I'm a cheeseburger
  • member icon


Reputation: 2533
  • View blog
  • Posts: 4,633
  • Joined: 24-September 10

Re: Readability Choice

Posted 05 December 2012 - 07:46 AM

Wouldn't choice 1 and 3 of your poll be nearly the same?

I could say that it's only sometimes clear. When you're doing it clearly, it's correct, and when it's not clear, you're doing it incorrectly.
Was This Post Helpful? 0
  • +
  • -

#7 code_m  Icon User is offline

  • D.I.C Head
  • member icon

Reputation: 24
  • View blog
  • Posts: 202
  • Joined: 21-April 09

Re: Readability Choice

Posted 05 December 2012 - 09:29 AM

Perhaps. Important thing is that no one chose "2". :) :^:
Was This Post Helpful? 0
  • +
  • -

#8 jon.kiparsky  Icon User is online

  • Pancakes!
  • member icon


Reputation: 7763
  • View blog
  • Posts: 13,127
  • Joined: 19-March 11

Re: Readability Choice

Posted 05 December 2012 - 09:33 AM

Quote

Perhaps. Important thing is that no one chose "2".


Well, of course. That's a straw man.

I like the auto-necro by the way. Usually it's someone else who raises a topic from the dead a year later.
Was This Post Helpful? 0
  • +
  • -

#9 cfoley  Icon User is online

  • Cabbage
  • member icon

Reputation: 2002
  • View blog
  • Posts: 4,167
  • Joined: 11-December 07

Re: Readability Choice

Posted 05 December 2012 - 11:19 AM

View Postcode_m, on 05 December 2012 - 02:23 PM, said:

View Postcfoley, on 13 July 2011 - 04:59 AM, said:

be index, not index+1? What if the value you find is followed by a zero?


The function is to un-escape a received byte message. That's the why the protocol works.

Sorry for digging up an old post :)/> Anyway, I went with the recursive definition. In the end it didn't matter much, only one other developer even ever saw the code.


Then I guess your code can't have been clear enough for me. :P
Was This Post Helpful? 0
  • +
  • -

#10 code_m  Icon User is offline

  • D.I.C Head
  • member icon

Reputation: 24
  • View blog
  • Posts: 202
  • Joined: 21-April 09

Re: Readability Choice

Posted 05 December 2012 - 12:21 PM

Don't yank my chain too hard, I need that neck! :cry:
Was This Post Helpful? 0
  • +
  • -

#11 BobRodes  Icon User is offline

  • Your Friendly Local Curmudgeon
  • member icon

Reputation: 574
  • View blog
  • Posts: 2,989
  • Joined: 19-May 09

Re: Readability Choice

Posted 05 December 2012 - 09:39 PM

View Postjon.kiparsky, on 05 December 2012 - 10:33 AM, said:

Quote

Perhaps. Important thing is that no one chose "2".


Well, of course. That's a straw man.

I like the auto-necro by the way. Usually it's someone else who raises a topic from the dead a year later.

A subtle form of recursion perhaps...
Was This Post Helpful? 0
  • +
  • -

#12 jon.kiparsky  Icon User is online

  • Pancakes!
  • member icon


Reputation: 7763
  • View blog
  • Posts: 13,127
  • Joined: 19-March 11

Re: Readability Choice

Posted 05 December 2012 - 09:52 PM

I always favored the definition of recursion that I got from my first computer teacher, in high school. "What's recursion? Well, you know what it means to curse, right? So clearly, to recurse you simply do that over and over."
Was This Post Helpful? 0
  • +
  • -

Page 1 of 1