Problem is to calculate median of numbers. My idea is :
- Fill array of integer with numbers using
Arraymake()
function - copy the address into a pointer created in
main()
function - pass that array to
Median()
function and calculate that median
Code:
#include <stdio.h>
#include <stdlib.h>
int* Arraymake(size_t);//Function for creating array of numbers.
void Median(size_t ,int* );//Function for calculate the median.
int main(int argc, char **argv)
{
size_t count;//Size of the list of numbers.
puts("Size of the list:");
scanf("%Iu",&count);
int *ArResult=Arraymake(count);//Result of the first function
Median(count,ArResult);
free(ArResult);
return 0;
}
int* Arraymake(size_t ln) //ln~lenght
{
int* Array=(int*) malloc(ln*sizeof(int));
int number ,i; //numbers filling the array
puts("Please input numbers.\n");
for(i=0;i<(ln);i++)
{
scanf("%i", &number);
Array[i]=number;
}
return Array;
}
void Median(size_t ln,int* Array)
{
float median;
if((ln)%2==0)
{
median= (Array[(ln)/2]+Array[(ln/2 )-1] )/2;
printf("%f",median);
}
else
{
median= Array[((ln-1)/2)];
printf("%f",median);
}
}
I want to find a way to optimize my variables and make my code more self-documented? How can I improve my code?
1 Answer 1
1. Arraymake
:
- Change the name to
MakeArray
as it makes more sense or name itMakeArrayFromInput
to make it more descriptive to what it actually does - Name the parameter
length
, as in :int* MakeArray(size_t length)
- this is descriptive and you don't have to put the comment explaining what it does - Name local variables in
camelCase
, soarray
instead ofArray
- Matter of preference but consider declaring
array
asint *array
instead ofint* array
- you won't forget that the variable itself is a pointer and you have to declare additional pointers in the same statement with*
s -int *array, *anotherPointer;
int number ,i; //numbers filling the array
- comment is redundant and you don't have to declarei
here, you can do it in thefor
- speaking of which:for(i=0;i<(ln);i++)
can becomefor(int i = 0; i < length; i++)
- parentheses overln
are redundant, you should use better spacing to make it more readable.
2. Median
:
- The same story with the parameter and naming, also pass the array first, then the length :
void Median(int *array, size_t length)
- You don't have to declare the
median
variable before theif
- Parentheses in the
if
are redundant :if(length % 2 == 0)
- The calculation of the median in the case of an even number of elements is flawed - you do an integer division as both operands of
/
areint
s - you'd have to make at least one of them afloat
e.g. like thismedian= (Array[(ln)/2]+Array[(ln/2 )-1] )/2.0;
- see the2.0
- this is adouble
literal, a floating-point number, not anint
- You can shorten the calculation a bit by changing
array[(length - 1) / 2];
toarray[length / 2];
in the case of an odd number - the integer division will truncate the result into what you need - The function should do one thing - you're calculating the median and printing it in the same function. Consider making the function
Median
return the value of the median and print it somewhere else
The function then should look more or less like this :
float Median(int *array, size_t length)
{
if(length % 2 == 0)
{
float median = (array[length / 2] + array[length / 2 - 1]) / 2.0;
return median;
}
else
{
float median = array[length / 2];
return median;
//or just simply
return array[length / 2];
}
}
Something more complex, the function can be greatly simplified by using the ternary operator ?:
:
float Median(int *array, size_t length)
{
return length % 2 == 0 ?
(array[length / 2] + array[length / 2 - 1]) / 2.0 :
array[length / 2];
}
However, this is just trivia and you should aim to make your code as readable as possible
3. main
- I think that all of the comments are redundant here, everything is self-documenting
- Naming :
ArResult
should be something descriptive and simple - the most simple way would be to name it justarray
- Spacing in function parameters e.g.
scanf("%Iu", &count);
instead ofscanf("%Iu",&count);
4. General advice
- Work on the spacing - try to put spaces between operands and operators as in
if(length % 2 == 0)
, in places likefor
loops :for(int i = 0; i < length; i++)
and in function parameters :scanf("%Iu", &count);
- Put comments before the thing they're describing, not at the end of the line - this way if you want to make it a multi-line comment you won't have problems and it's more readable that way e.g.
like this:
//Function for creating array of numbers.
int* MakeArray(size_t);
//Function for calculating the median.
int Median(int*, size_t);
This is just an example - I think the function names are descriptive enough not to put comments there, they will clutter the view :
int* MakeArray(size_t);
int Median(int*, size_t);
EDIT
What I didn't initially remember is that to calculate the median, you have to use a sorted array. You should probably do that separately
int *array = MakeArray(count);
qsort(array, count, sizeof(int), compare)
float result = Median(count, array);
The qsort is a function from stdlib.h
. You have to also declare a function compare
to use in it e.g.
int compare(const void* a, const void* b)
{
int int_a = *((int*) a);
int int_b = *((int*) b);
if(int_a == int_b) return 0;
else if (int_a < int_b) return -1;
else return 1;
}
This is taken from https://stackoverflow.com/a/3893967/7931009