This discussion has been locked.
You can no longer post new replies to this discussion. If you have a question you can start a new discussion

Writing a better code in better format

Hi guys I am a new learner in this field;
I am working on STR912x;

I am using the following code for sending a string which have saved in memory

          memset(sign_packet_data, 0, 0xFF);
              sign_packet_data = &sign_UART_txBuff[0];//char *sign_packet_data =0;
              strncpy(sign_packet_data, cmd,16);//cmd stores the string
              if(sign_packet_data != 0)
              {
               NbrOfDataToTransfer = strlen(sign_UART_txBuff);
               while(NbrOfDataToTransfer--)
               {
                UART_SendData(UART0, sign_UART_txBuff[TxCounter++]);
                while(UART_GetFlagStatus(UART0, UART_FLAG_TxFIFOFull) != RESET);
               }
               }

may i have a better way to do the same.

Parents
  • You have to spend some own time thinking about what you want to do and how you may do it. Then ask more specific questions. By the way:

    memset(sign_packet_data, 0, 0xFF);
    sign_packet_data = &sign_UART_txBuff[0];//char *sign_packet_data =0;
    


    You do a memset() before you assign a value to the sign_packet_data pointer. What did it point to before, i.e. what memory did you fill?

    Second - you do a memset with size 255. Don't use hard-coded sizes. If you have an array, you can do sizeof(array). If you only have a pointer, you can't do sizeof(p) since that will just give the size of the pointer and not the size of the target of the pointer. But then you should have a constant somewhere, giving the size of the array.

    strncpy(sign_packet_data, cmd,16);//cmd stores the string
    


    One more hard-coded size in your code. How do you know that sign_packet_data - or cmd - are 16 bytes large? Why not use a named constant?

    if (sign_packet_data != 0) { ... }
    


    This test is meaningless. You have already assigned an address to sign_packet_data, so you already know that it isn't NULL. Probably, you intended to check if the string pointed to was non-zero, in which case you should have written:

    if (*sign_packet_data != 0) { ... }
    

    How do you know that there are any terminating zero when doing this call?

    NbrOfDataToTransfer = strlen(sign_UART_txBuff);
    


    sign_UART_txBuff was copied to the memory pointed at by sign_packet_data with a length of 16.

    You have at one time used len 255 when doing a memset() of sign_packet_data - confusing change of length parameter.

    And you did a strncpy() from cmd, in which case the copy can break before any terminating zero was copied - and your zero-fill using sign_packet_data was done before you assigned any address to sign_packet_data...

    UART_SendData(UART0, sign_UART_txBuff[TxCounter++]);
    


    Why do you jump between different representations?
    Why assign a value to the pointer sign_packet_data if you are not using it while sending, but instead goes back to performing indexed access into sign_UART_txBuff?

    Somehow, I think you have to sit down and figure out exactly what you want to do. Then spend time with every single source code line and make sure it is needed. And make sure you understand what it does. And make sure what is does is what you want it to do.

Reply
  • You have to spend some own time thinking about what you want to do and how you may do it. Then ask more specific questions. By the way:

    memset(sign_packet_data, 0, 0xFF);
    sign_packet_data = &sign_UART_txBuff[0];//char *sign_packet_data =0;
    


    You do a memset() before you assign a value to the sign_packet_data pointer. What did it point to before, i.e. what memory did you fill?

    Second - you do a memset with size 255. Don't use hard-coded sizes. If you have an array, you can do sizeof(array). If you only have a pointer, you can't do sizeof(p) since that will just give the size of the pointer and not the size of the target of the pointer. But then you should have a constant somewhere, giving the size of the array.

    strncpy(sign_packet_data, cmd,16);//cmd stores the string
    


    One more hard-coded size in your code. How do you know that sign_packet_data - or cmd - are 16 bytes large? Why not use a named constant?

    if (sign_packet_data != 0) { ... }
    


    This test is meaningless. You have already assigned an address to sign_packet_data, so you already know that it isn't NULL. Probably, you intended to check if the string pointed to was non-zero, in which case you should have written:

    if (*sign_packet_data != 0) { ... }
    

    How do you know that there are any terminating zero when doing this call?

    NbrOfDataToTransfer = strlen(sign_UART_txBuff);
    


    sign_UART_txBuff was copied to the memory pointed at by sign_packet_data with a length of 16.

    You have at one time used len 255 when doing a memset() of sign_packet_data - confusing change of length parameter.

    And you did a strncpy() from cmd, in which case the copy can break before any terminating zero was copied - and your zero-fill using sign_packet_data was done before you assigned any address to sign_packet_data...

    UART_SendData(UART0, sign_UART_txBuff[TxCounter++]);
    


    Why do you jump between different representations?
    Why assign a value to the pointer sign_packet_data if you are not using it while sending, but instead goes back to performing indexed access into sign_UART_txBuff?

    Somehow, I think you have to sit down and figure out exactly what you want to do. Then spend time with every single source code line and make sure it is needed. And make sure you understand what it does. And make sure what is does is what you want it to do.

Children
No data