Please help with newbie code

W

whisper

Hi, I am learning how to program in C. Please help me by criticizing
anything
in the code below I wrote that converts int to binary and back. Is
there
something wrong that you see in the coding style, programming errors.
Any pointers would be greatly appreciated. Thanks!


When writing functions with more than one word for name, what is the
standard
practice - to use __ to join them or make them into one word ? For
example,
is binary_to_int more acceptable than BinaryToInt as name. I realize it
makes
no difference in terms of programming, but is there anything like a
recommended
style ?


Do comments have to be more informative ?



#include <stdio.h>
#include <math.h>



int Log2(int n) {
/* return the number of bits needed to store n in binary.
Is there a native C function to do this ? */

return (int) log((double) n) * (int) log(10) +1;
}

int *binary(int m) {
/* convert integer m into binary, return the array of bits
- least significant bit first */

int *binrep;
int numbits, i, indx;

numbits=Log2(m);

binrep=(int *) malloc (numbits*sizeof(int));

if(binrep==NULL) error("Could not allocate memory");

indx=0;

for(i=0;i<numbits;i++)
binrep=0;

while(m>0) {
binrep[indx++]=m%2;
m=m/2;
}

return binrep;
}


int btoint(int *binrep, int maxbits) {
/* convert binary rep to integer, binary rep
has least significant bit first */

int i,num=0;
int pow=1;

for(i=0;i<maxbits;i++) {
num=num+binrep*pow;
pow=2*pow;
}

return num;
}




int main() {

int i,inp, numbits;
int *binvect;

printf("Enter input: ");
scanf("%d", &inp);

numbits=Log2(inp);
binvect=binary(inp);


for(i=0;i<numbits;i++)
printf("%d ", binvect);


printf("\n");

printf("Input Number was %d\n", btoint(binvect, numbits));
}
 
M

Malcolm

whisper said:
When writing functions with more than one word for name, what is the
standard practice - to use __ to join them or make them into one word ? For
example, is binary_to_int more acceptable than BinaryToInt as name. I
realize it makes no difference in terms of programming, but is there anything
like a recommended style ?
For C, no. In Java the recommended style is to place the first word in lower
case, and start the following words in capitals (eg myMethod()) and you
might as well follow that style.
Personally I use all lower case for ANSI C portable functions, and start
platform-dependent functions with capitals. I don't use underscores because
they cannot be pronounced.
Do comments have to be more informative ?
You don't need to write an essay on each line of code. For functions you
need to say what the function does, what parameters it takes, and what it
returns.
#include <stdio.h>
#include <math.h>



int Log2(int n) {
/* return the number of bits needed to store n in binary.
Is there a native C function to do this ? */
No.

return (int) log((double) n) * (int) log(10) +1;
}
Doesn't look right to me. log returns the natural (base e) logarithm of a
number.
Also, do you not divide by the natural logarithm of x to get log to the base
x?

However this is a heavyweight method of doing things. Remember the number is
already stored in binary. sizeof(int) will tell you how many bytes in an
int, and CHAR_BIT how many bits in a byte (char).
So we can get the first bit by saying

if(n & (1 << (CHAR_BIT * sizeof(int) -1)) )

by subtracting more from the shift, we can find the first set bit.
int *binary(int m) {
/* convert integer m into binary, return the array of bits
- least significant bit first */

int *binrep;
int numbits, i, indx;

numbits=Log2(m);

binrep=(int *) malloc (numbits*sizeof(int));

if(binrep==NULL) error("Could not allocate memory");
This is a huge problem. Does error automatically terminate the program? If
not, should you not return zero (and that means you have to test the return
value in caling code)?
indx=0;

for(i=0;i<numbits;i++)
binrep=0;

while(m>0) {
binrep[indx++]=m%2;
m=m/2;
}

return binrep;
}


int btoint(int *binrep, int maxbits) {
/* convert binary rep to integer, binary rep
has least significant bit first */

int i,num=0;
int pow=1;

for(i=0;i<maxbits;i++) {
num=num+binrep*pow;
pow=2*pow;
}

return num;
}




int main() {

int i,inp, numbits;
int *binvect;

printf("Enter input: ");
scanf("%d", &inp);

numbits=Log2(inp);
binvect=binary(inp);

If binary returns 0 on fail, you can check and exit with EXIT_FAILURE here.
for(i=0;i<numbits;i++)
printf("%d ", binvect);


printf("\n");

printf("Input Number was %d\n", btoint(binvect, numbits));

You need to free binvect() here. Also return 0 to indicate successful
completion.
 
M

Method Man

whisper said:
Hi, I am learning how to program in C. Please help me by criticizing
anything
in the code below I wrote that converts int to binary and back. Is
there
something wrong that you see in the coding style, programming errors.
Any pointers would be greatly appreciated. Thanks!


When writing functions with more than one word for name, what is the
standard
practice - to use __ to join them or make them into one word ? For
example,
is binary_to_int more acceptable than BinaryToInt as name. I realize it
makes
no difference in terms of programming, but is there anything like a
recommended
style ?


Do comments have to be more informative ?



#include <stdio.h>
#include <math.h>



int Log2(int n) {
/* return the number of bits needed to store n in binary.
Is there a native C function to do this ? */

return (int) log((double) n) * (int) log(10) +1;
}

int *binary(int m) {
/* convert integer m into binary, return the array of bits
- least significant bit first */

int *binrep;
int numbits, i, indx;

numbits=Log2(m);

binrep=(int *) malloc (numbits*sizeof(int));

if(binrep==NULL) error("Could not allocate memory");

indx=0;

for(i=0;i<numbits;i++)
binrep=0;

while(m>0) {
binrep[indx++]=m%2;
m=m/2;
}

return binrep;
}


int btoint(int *binrep, int maxbits) {
/* convert binary rep to integer, binary rep
has least significant bit first */

int i,num=0;
int pow=1;

for(i=0;i<maxbits;i++) {
num=num+binrep*pow;
pow=2*pow;
}

return num;
}




int main() {

int i,inp, numbits;
int *binvect;

printf("Enter input: ");
scanf("%d", &inp);

numbits=Log2(inp);
binvect=binary(inp);


for(i=0;i<numbits;i++)
printf("%d ", binvect);


printf("\n");

printf("Input Number was %d\n", btoint(binvect, numbits));
}


Here are a couple optimizations you can make (among others). The compiler
may or may not do this for you:

m=m/2; -----------> m >>= 1;
pow=2*pow; -----> pow <<= 1;
 
E

E. Robert Tisdale

whisper said:
I am learning how to program in C.
Please help me by criticizing anything in the code below
that I wrote that convert int to binary and back.
Can you see anything wrong with the coding style or programming errors.
Any pointers would be greatly appreciated.

When writing functions with more than one word for name,
what is the standard practice -
to use __ to join them or make them into one word?
For example,
is binary_to_int more acceptable than BinaryToInt as name.

binaryToInt requires the fewest character
and minimum use of the shift key.
I realize [that] it makes no difference in terms of programming,
but is there anything like a recommended style?

Whether you choose to adopt an existing style or develop your own,
try to be consistent.
Do comments have to be more informative?
Yes.


> cat main.c
#include <stdlib.h>
#include <stdio.h>
#include <math.h>

int Log2( // return the number of bits
int n // needed to store n in binary.
) { // Is there a native C function to do this?
return (int)ceil(log((double)n)/log(2.0));
}

int *binary( // return an array of bits
int m // which is the binary representation of m
) {

const
int numbits = Log2(m);

int* binrep = (int*)malloc(numbits*sizeof(int));

if (NULL == binrep)
fprintf(stderr, "Could not allocate memory.\n");
else {
for(int i = 0; i < numbits; ++i)
binrep = 0;

int indx = 0;
while (0 < m) {
binrep[indx++] = m&1;
m >>= 1;
}
}
return binrep;
}


int btoint( // return integer value corresponding to
int *binrep, // binary representation in binrep array
int maxbits) { // least significant bit first

int num = 0;
int pow = 1;

for(int i = 0; i < maxbits; ++i) {
num = num + binrep*pow;
pow = 2*pow;
}
return num;
}

int main(int argc, char* argv[]) {

int inp = 0;

printf("Enter input: ");
scanf("%d", &inp);

int numbits = Log2(inp);
int* binvect = binary(inp);


for(int i = 0; i < numbits; ++i)
printf("%d ", binvect);
printf("\n");

printf("Input Number was %d\n", btoint(binvect, numbits));
return EXIT_SUCCESS;
}
> gcc -Wall -std=c99 -pedantic -o main main.c -lm
> ./main
Enter input: 13
1 0 1 1
Input Number was 13
Enter input: 11
1 1 0 1
Input Number was 11

It's hard to real binary numbers right to left.
 
D

Dave Thompson

int Log2(int n) {
/* return the number of bits needed to store n in binary.
Is there a native C function to do this ? */

return (int) log((double) n) * (int) log(10) +1;
}
As others have noted this should be log(n) (no cast needed since log()
is prototyped by math.h) _divided_ by log(10), and the division
should be done in floatingpoint (not cast the operands to int) for
most accurate results. And you need to special-case zero, if you want
to allow that as a value, since log(0) is mathematically erroneous.

Or you can use sizeof(int) * CHAR_BIT +1 for a conservative estimate,
that is sometimes larger than needed but never too small; that also
gives a fixed value that you can allocate at compile-time instead of
malloc'ing (at runtime) if you prefer.

Or if you want the exact figure, you can do divide-and-conquer:
int numbits=0;
/* assume n is at most 32-bit; adjust accordingly if not */
if( n >> 16 ) numbits += 16, n >>= 16;
if( n >> 8 ) numbits += 8, n >>= 8;
if( n >> 4 ) numbits += 4, n >>= 4;
if( n >> 2 ) numbits += 2, n >>= 2;
if( n >> 1 ) numbits += 1 /*, n >>= 1 */;
return numbits +1;
You may want to special-case zero here also. Substitute two statements
in braces for the two subexpressions with comma if you dislike that.

Or on most machines just use the floating-point hardware:
int expt;
/* (void) */ frexp ( /* (double) */ n, &expt);
/* casts not needed, but first may silence some warnings */
return n? expt: /*whatever*/;

Your code apparently isn't designed to handle negative values -- the
log() computation doesn't work at all; my divide-and-conquer and
frexp() versions work in different and probably unexpected ways, and
your loop below with % and / will produce implementation-dependent (in
C89) and probably unexpected results. So you are probably better off
making your 'number' values 'unsigned int' or just 'unsigned' (the int
is implied in that case) which more accurately reflects your intent
and in some places is likely to compile to better object code.
int *binary(int m) {
/* convert integer m into binary, return the array of bits
- least significant bit first */

int *binrep;
int numbits, i, indx;

numbits=Log2(m);

binrep=(int *) malloc (numbits*sizeof(int));
Cast not needed if you #include <stdlib.h> as you must -- and
insufficient to actually be correct if you don't: it often silences
the misleading diagnostic about "convert int to pointer without cast"
but it doesn't correct the actual problem of malloc() being implicitly
declared incorrectly in C89 (and not at all in C99).

Don't really need an int for each extracted bit, a byte (char or
probably preferably unsigned char) is enough. And then you don't need
to multiply *sizeof(int) -- or as c.l.c prefers *sizeof(*binrep) --
since sizeof(char) is guaranteed to always be 1.
if(binrep==NULL) error("Could not allocate memory");

indx=0;

for(i=0;i<numbits;i++)
binrep=0;

while(m>0) {
binrep[indx++]=m%2;
m=m/2;
}

Why two different index variables? In fact, could junk the first loop
and just run the second loop up to numbits:
while( indx < numbits ){ binrep[indx++] = m % 2; m /= 2; }
This may redundantly compute 0 % 2 and 0 / 2 for the last bit or so,
but as these are likely to compile into bit-and and shift instructions
anyway, especially if you declare m unsigned, it probably won't be
detectably slower than just assigning 0. And is simpler and clearer.
return binrep;
}


int btoint(int *binrep, int maxbits) {
/* convert binary rep to integer, binary rep
has least significant bit first */

int i,num=0;
int pow=1;

for(i=0;i<maxbits;i++) {
num=num+binrep*pow;
pow=2*pow;
}

return num;
}

Again better to make num, pow, and the return value unsigned.
Can reduce the number of variables with variable shift:
for( i = 0; i < maxbits; i++ ) num += binrep << i;
or by looping downward:
for( i = maxbits; i--; ) num = num * 2/*U*/ + binrep;
/* or just (re)use maxbits as the loop/index variable */
Either of these also eliminates the (apparently) full multiply in the
loop which the compiler is unlikely to be able to optimize well.
(Although performance details are machine and compiler specific, and
hence off-topic.)
int main() {

int i,inp, numbits;
int *binvect;

printf("Enter input: ");
scanf("%d", &inp);
Should check for error. If you make 'inp' unsigned, use "%u".
And similarly for the printf of btoint() below.
numbits=Log2(inp);
binvect=binary(inp);


for(i=0;i<numbits;i++)
printf("%d ", binvect);

This needn't change even if you make the representation elements chars
(of any flavor). char values passed to a variadic function, which
printf is, are promoted to int for you -- except on some very oddball
systems you aren't likely to ever see, to unsigned int, and even there
%d of a unsigned value in the positive range of int, as 0 and 1 must
be, will almost certainly work though not formally guaranteed.
printf("\n");

printf("Input Number was %d\n", btoint(binvect, numbits));
}

Memory is malloc'ed in binary() and (the address) returned to main()
which uses it, but never free's it. For a simple test program like
this it will be cleaned-up when the program exits (not formally
guaranteed but works on any practical 'hosted' system you will
encounter), but if you someday want to use this (or similar) code in a
larger more complex program, if you don't learn to keep track of your
memory use and free() the things you are done with, you will cause
memory leaks that likely reduce performance (perhaps drastically) or
crash your program (at least) at the most inconvenient times.

Should explicitly 'return 0' or some other defined value from main
(unless you exit() first) -- this isn't required in C99 (not yet
common), but omitting causes problems on *some* C89 systems.

- David.Thompson1 at worldnet.att.net
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

No members online now.

Forum statistics

Threads
473,744
Messages
2,569,484
Members
44,904
Latest member
HealthyVisionsCBDPrice

Latest Threads

Top