current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite

This takes a string input like 1,2,3,4,5, splits it using , delimiter, converts each char to int and output it in a single line like this : 12345

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

int* createIntArray(char* numbersInString, int* array_size);

int* createIntArray(char* numbersInString, int* array_size)
{
    char* token = strtok(numbersInString,",");
    int* numbersArray = malloc(sizeof(int)*100);
    int i = 0;

    while(token != NULL)
    {
        numbersArray[i] = (int)*token -'0';
        token = strtok(NULL,",");
        i++;
    }

    *array_size = i;
    return numbersArray;
}

int main(){
    char* stringNumbers = malloc(sizeof(char)*100);
    int* intArray;
    int arrayLength = 0;
    printf("Enter numbers\n");
    scanf("%s",stringNumbers);
    intArray = createIntArray(stringNumbers,&arrayLength);

    int i;
    for(i = 0; i < arrayLength;i++)
    {
        printf("%d",intArray[i]);
    }

    free(stringNumbers);
    free(intArray);
    return 0;
}

I know the code is super useless, but I'm starting to learn C and I wanted to see if my code was alright and if it complies to C coding standards.

share|improve this question
    
I spy an integer overflow exploit! –  syb0rg yesterday
    
This would happen if I was happening to enter : 12379573853625767531596,1. Is that it? –  TopinFrassi yesterday
    
No, you would have to enter enough characters to fill up all of the buffers on the local stack, ~106-116 bytes/characters depending on the computer architecture. –  syb0rg yesterday
    
Oh, not understanding what you mean means I should read your link :P –  TopinFrassi yesterday
1  
This is a more complicated topic for a beginner to understand. Don't worry too much if you don't get the concept. If you were to use this same code for a login application though, then I would have to write a full-fledged answer on the exploitation process and prevention of the overflow. –  syb0rg yesterday
show 2 more comments

2 Answers 2

up vote 3 down vote accepted

A few things to consider, since this is a learning exercise. They don't really matter for your program, but maybe you'll avoid learning a few bad habits...

  • while(token != NULL) would commonly be written while(token) instead.
  • Learn about const. Use it whenever you can. It can make the meaning of the code clearer, and it can help the compiler find mistakes. Your createIntArray function could have parameters char* const numbersInString and int* const array_size, for example.
  • Try to use a consistent style for naming things. numbersInString is one style. array_size is a different one. Either style works ok. Mixing them adds a bit of confusion.
  • strtok is an anti-favorite of mine. It modifies the input string. Sometimes that's fine, but you could do this task without modifying the input. A function that works on a constant string will be useful in more places than a function that alters the input to get the same result.
share|improve this answer
    
What could I use instead of strtok? When I looked on Google it is the one that seemed the most suited for my case –  TopinFrassi yesterday
    
@TopinFrassi Use strtok_r. –  syb0rg yesterday
    
strtok_r also modifies the input string. I don't have a suggested one-size-fits-all replacement to suggest; it depends on the situation. In this case, I might start with sscanf to find one integer at a time from the string. –  GraniteRobert yesterday
    
This was too easy ahah –  TopinFrassi yesterday
    
@GraniteRobert I know, but that is a standard thread-safe option for strtok. Using sscanf to find one integer at a time would be inefficient. –  syb0rg yesterday
add comment
up vote 3 down vote

  • Since you have main() below the other function, you don't need the prototype for that function. It's only needed if main() is defined before any other functions.

  • There's no need to declare a variable and then assign to it:

    int* intArray;
// ...
intArray = createIntArray(stringNumbers,&arrayLength);

    Variables should just be initialized in order to keep them in the lowest scope possible. This will ease maintenance as you won't have to find the declared variable elsewhere.

    int* intArray = createIntArray(stringNumbers,&arrayLength);

  • Instead of incrementing i in createIntArray(), why not just increment array_size?

    Plus, i is more commonly used as a for loop variable, which isn't the case here.

share|improve this answer
    
I'm so focused on figuring pointers out that I forget the basics of programming!! Though I didn't know about your first point, thanks! –  TopinFrassi yesterday
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.