A newbie's code

[main()]

Hi all,

I'm learning this great language called C.
I thought i would try my hand at writing small some code.
I wrote a very small function that takes two char pointers a and b.
It is to remove all letters from a that occurs in b.

#include <stdio.h>

void fun(char *a,const char *b)
{
int i=0,apos=0,j;
if(!a || !b) /* basic check*/
return;
for(;a;i++)
{
for(j=0; b[j] && a != b[j] ;j++); /*loop until end of b or until
a match is found*/
if(!b[j])
a[apos++] = a;
}
a[apos] = '\0';
}

int main(void)
{
char a[] = "hai how are you ?";
char b[] = "aeiou";
fun(a,b);
printf("%s\n",a);
return 0;
}

I have done and complied, it seems ok.
But i need your comment on my code.
I'm a newbie.I welcome any comment (like style, logic, any other neat
way to write the same function or proper usage etc).

Thanks for your time,
Yugi.

 

[pete]

Hi all,

I'm learning this great language called C.
I thought i would try my hand at writing small some code.
I wrote a very small function that takes two char pointers a and b.
It is to remove all letters from a that occurs in b.

#include <stdio.h>

void fun(char *a,const char *b)
{
int i=0,apos=0,j;
if(!a || !b) /* basic check*/
return;
for(;a;i++)
{
for(j=0; b[j] && a != b[j] ;j++); /*loop until end of b or until
a match is found*/
if(!b[j])
a[apos++] = a;
}
a[apos] = '\0';
}

int main(void)
{
char a[] = "hai how are you ?";
char b[] = "aeiou";
fun(a,b);
printf("%s\n",a);
return 0;
}

I have done and complied, it seems ok.
But i need your comment on my code.
I'm a newbie.
I welcome any comment (like style, logic, any other neat
way to write the same function or proper usage etc).

It's not bad.

/* BEGIN new.c */

#include <stdio.h>

#define STRING "\n\n\n\tThere's\n a\r beat in \r\tmy head.\n\n\n"
#define WHITE "\n\r\t"

void fun(char *a,const char *b)
{
size_t i = 0, apos = 0, j;
/*
** size_t is guaranteed to be big enough
** to represent the number of bytes in any object.
*/
if (a == NULL || b == NULL) {
return;
}
/*
** explicit comparisons against NULL for pointers
** are easier to read.
*/
for (; a; i++) {
for (j=0; b[j] && a != b[j]; j++) {
;
}
/*
** Always using the optional {braces}
** and putting the empty statement on it's own line,
** makes the above empty loop more obvious.
*/
if (b[j] == '\0') {
a[apos++] = a;
}
/*
** The same convention of using '\0' below
** in assignement,
** should also be used in the above if()
** in comparison,
** to compare a byte in a string with the null character.
*/
}
a[apos] = '\0';
}

int main(void)
{
char a[] = "hai how are you ?";
char b[] = "aeiou";
char c[] = STRING;

puts(a);
fun(a, b);
puts(a);
puts(c);
fun(c, WHITE);
puts(c);
return 0;
}

/* END new.c */

(fun) is K&R2 Exercise 2-4, Alternate squeeze functions.
I have three overly complicated versions of the same function.
http://groups.google.com/group/comp.lang.c/msg/d9c5bff63bc679f1

 

[Richard Bos]

#include <stdio.h>

void fun(char *a,const char *b)

Wise use of const - you don't see that too often in newbie code.

{
int i=0,apos=0,j;
if(!a || !b) /* basic check*/
return;
for(;a;i++)

I'd initialise both i and apos here, for clarity:

for(i=apos=0; a; i++)

{
for(j=0; b[j] && a != b[j] ;j++); /*loop until end of b or until
a match is found*/
if(!b[j])
a[apos++] = a;
}
a[apos] = '\0';
}

int main(void)
{
char a[] = "hai how are you ?";

And not making the all too common error of passing a pointer to a string
literal to this function - another bonus point.

char b[] = "aeiou";
fun(a,b);
printf("%s\n",a);
return 0;
}

I'm a newbie.I welcome any comment (like style, logic, any other neat
way to write the same function or proper usage etc).

I'd use spaces a little differently, but everybody has a different style
where spacing is concerned. Just be consistent, and don't skip them
altogether. I'd also use more meaningful function and parameter names.

You could write the function completely differently using strcspn() or
strpbrk(). I'm not sure whether that would buy you anything, though.

Apart from that - no remarks.

Richard

 

[spibou]

Hi all,

I'm learning this great language called C.
I thought i would try my hand at writing small some code.
I wrote a very small function that takes two char pointers a and b.
It is to remove all letters from a that occurs in b.

#include <stdio.h>

void fun(char *a,const char *b)
{
int i=0,apos=0,j;
if(!a || !b) /* basic check*/
return;
for(;a;i++)
{
for(j=0; b[j] && a != b[j] ;j++); /*loop until end of b or until
a match is found*/
if(!b[j])
a[apos++] = a;
}
a[apos] = '\0';
}

int main(void)
{
char a[] = "hai how are you ?";
char b[] = "aeiou";
fun(a,b);
printf("%s\n",a);
return 0;
}

I have done and complied, it seems ok.
But i need your comment on my code.
I'm a newbie.I welcome any comment (like style, logic, any other neat
way to write the same function or proper usage etc).

It's fine ; short and elegant. The only thing
that bothers me a bit is that the value of b[j]
is checked both inside the loop
for(j=0; b[j] && a != b[j] ;j++);
and right after you exit the loop. There's a way
to avoid that at the cost of readability.

void fun_version2(char *a,const char *b)
{
int i=0,apos=0,j;
if(!a || !b) /* basic check*/
return;
for(;a;i++)
{
j=0 ;
loop:
if (b[j] == 0) {
a[apos++] = a ;
continue ;
}
if (a != b[j])
continue ;
j++ ;
goto loop ;
}
a[apos] = '\0';
}

I guess it's possible that a clever enough compiler
wouldn't actually check the value of b[j] twice.

It would be nice if C had a "continue n" statement
where n==1 would be like regular continue ,
"continue 2" would mean continue the loop in which
this one is contained etc. But since it doesn't,
one has to rely on goto or on checking the same
condition more than once.

Spiros Bousbouras

 

[Flash Gordon]

You've described the problem and provided your code for implementing it.
This is good.

You should choose a better name than fun. As your projects grow picking
sensible names makes it far easier for you, let alone anyone else, to
read your code.

<snip>

Others have commented on the rest of your code.

My other main comment is you are to be congratulated on coming here for
the right purpose and in the right way. It was a far better first post
than many we see.

It's not bad.

(fun) is K&R2 Exercise 2-4, Alternate squeeze functions.
I have three overly complicated versions of the same function.
http://groups.google.com/group/comp.lang.c/msg/d9c5bff63bc679f1

The OP may also find it interesting to look at Richard Heathfield's
implementation which is available at
http://clc-wiki.net/wiki/K&R2_solutions:Chapter_2:Exercise_4

I would recommend only looking at solutions to other exercises on this
site after you have attempted the exercise yourself.

 

[Kenneth Brody]

Hi all,

I'm learning this great language called C.

[...]

Welcome.

The first thing I noticed is that your pseudonym should probably be
"main(void)" rather than "main()".



--
+-------------------------+--------------------+-----------------------+
| Kenneth J. Brody | www.hvcomputer.com | #include |
| kenbrody/at\spamcop.net | www.fptech.com | <std_disclaimer.h> |
+-------------------------+--------------------+-----------------------+
Don't e-mail me at: <mailto:[email protected]>

 

[Ancient_Hacker]

Just a *few* suggestions:

int i=0,apos=0,j;

You're executing TWO assignment statements unecessarily in the case
where one or both params are NULL. And you neednt set "j" at all
here, as it's always set in the for initializer part (whatever you call
the first for() thingamaggummy).

if(!a || !b) /* basic check*/

You are to be complimented n checking your parameters, wish MicroSoft
did so with any regularity in their examples.

But would it kill you to write clearer code, ala:

if( a == NULL || b == NULL ) ....

Yes, I know !a is a well established C idiom. Even highly regarded
snippets of code are full of that kind of thing. Pls ponder the
relative clarity of each, especially when you get a call at 2:21AM to
fix your program.

return;

"return" is a mighty hand thing, but it's in a sense a amorphous goto.
Consider having just one exit point, at the bottom of the function. It
makes adding debug lines, assertions, and return value settings sooo
much clearer.

for(;a;i++)

This is fine, a really puristt would suggest initializing "i" here,
just in case someone ever adds code between the initialization and this
point and one loses track of wat's in "i".

Also a bubbly-headed coder might suggest "a != EndOfString" or even
"a IsNot EndOfString". Much clearer when the F133 payroll program
for the whole Skunk Works Division of Lockheed isnt running (a friend
of mine was called at 4 AM to fix that).
Yes, it takes a smidgen more typing, but disk space is not exactly
expensive these days.

Otherwise okay.

 

[Frederick Gotham]

main() posted:

#include <stdio.h>

void fun(char *a,const char *b)
{
int i=0,apos=0,j;
if(!a || !b) /* basic check*/
return;

You should decide whether you want the programmer to be able to supply this
function with null pointers. If you forbid this, then use assertions:

assert(a);
assert(b);

I'll give you an example of how I would go about this:

#include <assert.h>

void RemoveCertainChars(char *alter, char const *const arg_certain)
{
char const *certain;

assert(alter);
assert(arg_certain);
assert(*alter);
assert(*arg_certain);

do
{
certain = arg_certain;

do if(*certain++ == *alter) *alter = 'X';
while(*certain);

}while(*++alter);
}

 

[Frederick Gotham]

Flash Gordon posted:

The OP may also find it interesting to look at Richard Heathfield's
implementation which is available at
http://clc-wiki.net/wiki/K&R2_solutions:Chapter_2:Exercise_4

I would frown upon the use of array subscripting rather than pointers.

 

[Frederick Gotham]

Ancient_Hacker posted:

You are to be complimented n checking your parameters, wish MicroSoft
did so with any regularity in their examples.

But would it kill you to write clearer code, ala:

if( a == NULL || b == NULL ) ....

Yes, I know !a is a well established C idiom.

Idiom... ?! Something has to be at least mildy exotic to achieve the status
of "idiom".

Using the inversion operator on a pointer is perfectly clear, and a
fundamental feature of the language.

 

[Frederick Gotham]

Frederick Gotham posted:

I'll give you an example of how I would go about this:

Or, actually removing the characters rather than replacing them with X:

void ShiftStringBackwardsOnce(char *behind)
{
char const *ahead = behind + 1;

assert(behind); assert(*behind);

while(*behind++ = *ahead++);
}

void RemoveCertainChars(char *alter, char const *const arg_certain)
{
char const *certain;

assert(alter); assert(*alter);
assert(arg_certain); assert(*arg_certain);

do
{
certain = arg_certain;

do if(*certain++ == *alter) ShiftStringBackwardsOnce(alter--);
while(*certain);

}while(*++alter);
}

 

[Flash Gordon]

Flash Gordon posted:


I would frown upon the use of array subscripting rather than pointers.

You might, but I and many others would not. Array subscripting is
perfectly clear in my opinion.

 

[Keith Thompson]

Flash Gordon posted:

I would frown upon the use of array subscripting rather than pointers.

Why?

 

[Keith Thompson]

for(;a;i++)

This is fine, a really puristt would suggest initializing "i" here,
just in case someone ever adds code between the initialization and this
point and one loses track of wat's in "i".

Yes, I'd probably drop the initialization in the declaration of i, and
change the for loop to:

for (i = 0; a; i++)

If I could depend on having a C99 compiler, I'd write:

for (int i = 0; a; i++)

Also a bubbly-headed coder might suggest "a != EndOfString" or even
"a IsNot EndOfString". Much clearer when the F133 payroll program
for the whole Skunk Works Division of Lockheed isnt running (a friend
of mine was called at 4 AM to fix that).
Yes, it takes a smidgen more typing, but disk space is not exactly
expensive these days.

Defining the identifiers (macros?) "EndOfString" and "IsNot" would do
nothing but obfuscate the code. I would actually write:

for (i = 0; a != '\0'; i ++)

Anyone capable of understanding C code will know what "!=" and '\0'
mean. Anyone who doesn't know that won't be helped by pseudo-English
aliases. See also question 10.2 in the comp.lang.c FAQ.

 

[Keith Thompson]

Ancient_Hacker posted:



Idiom... ?! Something has to be at least mildy exotic to achieve the status
of "idiom".

Using the inversion operator on a pointer is perfectly clear, and a
fundamental feature of the language.

Yes, it's a fundamental feature of the language, but in my opinion an
explicit comparison against NULL is clearer. For one thing, it makes
it obvious that the variable being tested is a pointer and not some
other kind of scalar. (And I'm well aware that plenty of very smart
people disagree.)

 

[Keith Thompson]

main() posted:



You should decide whether you want the programmer to be able to supply this
function with null pointers. If you forbid this, then use assertions:

assert(a);
assert(b);

This will work in C99, but C90 doesn't guarantee that the argument to
assert() can be anything other than an int (though I think it would be
ok in most implementations). I'd write:

assert(a != NULL);
assert(b != NULL);

IMHO, this is clearer both in the source code and in the error message
that's produced if the assertion fails.

 

[Frederick Gotham]

Keith Thompson posted:

Why?

Efficiency and clarity. And efficiency again.

 

[Frederick Gotham]

Keith Thompson posted:

Yes, it's a fundamental feature of the language, but in my opinion an
explicit comparison against NULL is clearer. For one thing, it makes
it obvious that the variable being tested is a pointer and not some
other kind of scalar. (And I'm well aware that plenty of very smart
people disagree.)

If my own opinion is worth anything, I've never used NULL once in my code.

 

[Walter Roberson]

Keith Thompson posted:

Efficiency and clarity. And efficiency again.

Many compilers these days optimize array subscripting very well,
often better than they can optimize most pointers. pointers are
usually much harder to analyze to prove that aliasing cannot be
happening.

I have also seen implementations in which larger arrays (including
those in automatic variables) were automagically aligned to optimize
cache considerations. I have never -seen- an implementation in which
malloc() did that, especially as malloc() cannot be handed
access pattern information. Compilers can take access patterns
into account when positioning subscripted arrays.

 

[Keith Thompson]

Keith Thompson posted:

Efficiency and clarity. And efficiency again.

Why do you think code using pointers is more efficient than code using
array indexing? It probably would be given a sufficiently naive
compiler (or even a modern compiler invoked without optimization), but
any modern optimizer should be able to generate equally good code
either way.

Clarity, of course, is in the eye of the beholder.