My code is ugly

[Jester]

Often I can get something to work, but I know a real programmer could simplify my solution to a fraction of the size and complexity.

Hardware:  embedded Arduino application running on CortexM0 (MKRZero)

I have a structure and within that structure there is a string and somewhere within that string there is a  'F' followed by either 1, 2 or 3 digits for example F44, I need to extract the 44 as either an int or float.

This works but it sure seems clumsy,  suggestions welcome.

Code: [Select]

     

    struct mData{
      char data;
      int space;
    };

    mData mdiData[40];

// ..... at this point mdiData.data is populated with data including the Fxx portion and is null terminated after the Fxx numeric value.


           fLoc = 0;
           char fs[10];
           for (int j = 0; j < 39; j++) {                                                     
                    if (mdiData[j].data == 'F')
                           fLoc = j;
                                           
                     if (j > fLoc+3) {
                                fs[0] = mdiData[fLoc+1].data;
                                fs[1] = mdiData[fLoc+2].data;
                                fs[2] = mdiData[fLoc+3].data;
                                fs[3] = '\0';
                      }
               }
               feedSpeed = atof(fs);


I tried using strchr() to find the location of 'F' but I don't know how to get that to work when using a structure, I can make it work if the string is in a simple char[] array.

Thanks to anyone that can offer a more elegant solution.

[T3sl4co1l]

Can you not rearrange it to {char[] data[40], int[] space[40]} and then iterate over the string?

strchr will not work because of the alternating chars and ints (which incidentally won't pack tightly either, you lose 3/8ths of the memory there).  A random int value could contain a 'F' and create a false positive.

Tim

[pqass]

If the last four mdiData array elements are F123, then it won't be processed. 
Your code requires a NULL or dummy last element (" if (j > fLoc+3) ...").
Also, are there any characters to skip between F123 and the next F456?
And which (first or last) to keep?

This is my (untested) take:

Code: [Select]

#define NELEMS(x)  (sizeof(x) / sizeof((x)[0]))

float feedSpeed = 0.0;
for (int j = 0; j < NELEMS(mdiData); j++) {
if (mdiData[j].data == 'F') {
            j++;
for (feedSpeed = 0.0; j < NELEMS(mdiData) && isdigit(mdiData[j].data); j++) {
feedSpeed = (feedSpeed * 10.0) + (mdiData[j].data - '0');
            }
            j--;
}
}

feedSpeed is set to the last F456 (0 to n digits; n is more than you need) found.

EDIT: replace "j--;" with "break;" to exit on first occurrence.

The real question is why do you need an array of structures with just a one character (.data) and an int (.space)?

[Jester]

There will only be one Fxxx in the string.

The string gets sent to a small display, in order to display the complete string in the limited space, I cheat by using smaller spaces between terms. The string can be manipulated by the user by selecting the desired character with cursor keys, backspace etc, so I need to keep track of the width used for each character/space in the structure.

Thanks for your elegant solution, I need to read about NELEMS.

[pqass]

My solution involves two floating-point ops per digit read which may be time-costly on a 8-bit MCU.  But at least there are fewer iteration, index, and temp vars to manage (and debug).

sizeof returns bytes allocated to the whole type/var x so dividing by the bytes used by the first element, x[0], returns number of elements.  No need to create a #define for every array; can just drop in a one-time number at the array definition.  It's portable, type independent, and no overhead as the value gets resolved at compile-time.