Noobish C Fail - 'Permission Denied' - recreating Soundex code

peteruk

peteruk

peteruk

Associate
Joined
27 Jan 2005
Posts
1,013
Location
Nr. Norwich, Norfolk
Hey guys,

The lastest episode in the saga of my wrestle to get to grips with C revolves around my inability to handle arrays.

I've been tasked with recreating the Soundex code. This involves several stages - removing vowels from a set of characters, removing double letters, and then translating it into a numeric code. :eek:

However, that all sounds pretty daunting, so to get to grips with how arrays work I've taken one task - removing vowels - and started trying to write a program that does just that and that alone, printing the output. However, I seem unable to get something that doesn't yield a 'Permission Denied' error. I'm assuming I'm grossly misunderstanding something along the way. :o

So far I have:

Code: 
#include <stdio.h>
#include <string.h>
#include <math.h>

void devowel(char *B) {
  int i=1;
  int k=1;
  char a[27];/*This is entirely for reference. 26 letters...including 'ender'.*/
  a[0] ='A';
  a[1] ='B';
  a[2] ='C';
  a[3] ='D';
  a[4] ='E';
  a[5] ='F';
  a[6] ='G';
  a[7] ='H';
  a[8] ='I';
  a[9] ='J';
  a[10] ='K';
  a[11] ='L';
  a[12] ='M';
  a[13] ='N';
  a[14] ='O';
  a[15] ='P';
  a[16] ='Q';
  a[17] ='R';
  a[18] ='S';
  a[19] ='T';
  a[20] ='U';
  a[21] ='V';
  a[22] ='W';
  a[23] ='Y';
  a[24] ='X';
  a[25] ='Z';
  a[26] ='\0';
  //char B[20]; /*This array will hold the input*/
  /* B[20]='\0'; */
  scanf ("%s",B);
  for (i=0;i<=19;i++) {
    if (B[i]==a[0]) {
      for (k=0;k<=19;k++){   
		B[k]=B[k+1];
		}
	if (B[i]==a[4]) {
		for (k=0;k<=19;k++){   
		B[k]=B[k+1];
		}
    } 
	if (B[k]==a[8]) {
      		for (k=0;k<=19;k++){   
		B[k]=B[k+1];
		}
    } 
	if (B[k]==a[14]) {
      		for (k=0;k<=19;k++){   
		B[k]=B[k+1];
		}
    } 
	if (B[k]==a[20]) {
      		for (k=0;k<=19;k++){   
		B[k]=B[k+1];
		}
    }
  }
  printf("%s\n",B);
}

//*int dedouble { }
    
    
int main (void) {
	char B[20];
	devowel(B);
	printf("%s\n",B);
	return 0;
}
 
Last edited:
For a start you can't call

Code: 
a[26] ='\0';

when you intialised the array with

Code: 
char a[26];

As you are accessing the 27th element (it's numbered from 0-25)

So you'll be getting memory corruption issues right there.

Same with

Code: 
for (i=0,i<=20,i++)

You're trying to access the 21st element.

And surely you don't want

Code: 
scanf ("%f",B);

As thats reading in floats? Surely you want

Code: 
scanf ("%s",B);
to read a string?
 
Last edited:
Dave,

Thanks for pointing out my newbish mistakes! And taking the time to explain why they are so. :D

I've modified the length of a [] to 27 characters (I didn't understand before that was how they were numberd) to prevent that fault. I've done a similar thing with the i variable by reducing the continuation parameter to i<=19.

I've also changed the scanf and printf to reflect the correct data to be read.

However, I'm still getting the same error. Any ideas?
 
Firstly, please use the
Code: 
code
tag for code as it preserves the tabbing!

The first problem I can see is you are doing:

if (B=a[0])

You want the comparason operator '==' not '=', the asignment operator. But then you are assigning B=0; This will not remove the vowel, but terminate the string. So "test" will change to "t\0st" which is "t".

You are also using:

printf("%f" ,devowel)

which is going to print out devowel as a float. I'm not sure thats what you wanted.
 
Last edited:
you cant initialise an array like that. do this as an example instead

char a[5] = "ABCDE";

or

char a[5] = {'A','B','C','D','E');


also using single letters as variable names is not recommended and a practice I would advise you to get out of!

Is 'devowel' a function? It may just be the formatting of the text but if it is then the syntax is all wrong
 
Last edited:
You can initialise an array that way, it far from great, but it works. I guess technically you are not initialising the array at all and leaving it up to the whims of the compiler but setting all the values very soon after. However, my next suggestion was going to be to rework how that array work completely and it isn't the ops biggest issue :)

I was curious as to what is going on with the devowel 'function'. I assume thats just a quirk of whatever program this code is running in, or a problem with the cut-n-pasting, but yes, it sure aint the usual C.
 
Last edited:
my point is, there is a declaration (char a[]) some code and then another declaration (char b[]) - this is not allowed in C and likely to be one of the causes of the errors
 
Hey everyone,

I've taken in your comments and spent some more time researching. I've made a few changes and updated my op with the correct coding. I understand however there are a few fundamental flaws with the way I'm coding, but for now that's how I understand it.

You want the comparason operator '==' not '=', the asignment operator. But then you are assigning B=0; This will not remove the vowel, but terminate the string. So "test" will change to "t\0st" which is "t".
Click to expand...


I'm still getting this problem though. I'll stick at it with a loop...

Thanks!
 
BullBoyShoes said:
my point is, there is a declaration (char a[]) some code and then another declaration (char b[]) - this is not allowed in C and likely to be one of the causes of the errors
Click to expand...

Declaration of char b[] is commented out.

UPDATE2: I missed one thing you changed. I see B is declared [20] so its okay, but still:

Why are your loops going from 0 to 19 eg:

for (i=0;i<=19;i++)
and
for (k=0;k<=19;k++)

Have a variable at the start called stringlength and do a:

stringlength = strlen(B);

after you have asked the user for the input.

Then all your loops can be:

for (i=0;i<=stringlength;i++)

Also, single letter variable names are okay for loops, but not important variables and parameters. And to be fussy, your indenting is terrible. It may seem minor but when you have screens of code to look through, its not so silly.

UPDATE:

I would try changing the code to remove asking the user for input. Start real simple and just pass in your function an hard coded string. Its much easier to start that way as you can be sure its all okay. Change your main function to just

devowel("Test string with a few words to make sure things are okay.");
return 0;

That way you can remove the scanf you do and potentially remove another problem.

I hope I'm helping.
 
Last edited:
Hello again everyone,

Once more your comments have been extremely useful! However, I can go on with this dodgy way of coding no longer. Instead, I've spent a while taking in some of Caustic's and others comments to completely rewrite the code.

I've also taken the opportunity to add in the other bits of the code - removal of double letters, and the ignorance of first letters for both filters.

My code now looks something like:

Code: 
#include <stdio.h>
#include <string.h>
#include <math.h>

void shift(int shifter, char* word, int len){
	int i;
	
	for (i = shifter; i != len ; i++)
		word[i] = word[i+1];
		
}

void capitalise(char* word, int len){
	int i;
	
	for (i = 0; i < len ; i++)
		if (islower(word[i]))
			word[i] = toupper(word[i]);
}

int devowel(char* word, int len){
	int i;
	
	for (i = 1; i <= len+1 ; i++)
		if (word[i] == 'A' ||
			word[i] == 'E' ||
			word[i] == 'I' ||
			word[i] == 'O' ||
			word[i] == 'U' ||
			word[i] == 'W' ||
			word[i] == 'Y' ||
			word[i] == 'H'){
				shift(i, word, len);
				len = strlen(word);
		}
	return len;
}

int dedouble(char* word, int len){
	int i;
	
	for (i = 0; i < len ; i++)
		if (word[i] == word[i+1])
		{
			shift(i+1, word, len);
			return 0;
		}
			
	return 1;
}

int main (void){
	char *word;
	int len;
	int flag = 0;
	
	scanf ("%s", word);
	len = strlen(word);
	capitalise(word, len);
	
	while (flag < 1){
		flag = dedouble(word, len);
		len = strlen(word);
	}
	
	len = devowel(word, len);
	len = devowel(word, len);
	printf("%s\n",word);
}

I think my tabulation is MUCH better here, which should help me keep track of what is doing what.

However I am still having slight problems - I can't seem to remove the last vowel off the end of strings when they are realllllly long. ie. eeeeffeeoooiiiee returns EFE. I'm not sure why this is. eeefffeeoooiiie returns the same. eeefffeeooiii returns the correct EF.

Any ideas why?

And a massive thank you to everyone so far, I would never have been able to improve this much without all your suggestions. OcUK never fails to amaze. :D

EDIT: forgot to mention - the code pasted is the one I'm trying to mess around with - hence the repeating of the devoweling.
 
BullBoyShoes said:
devowel asks for a char *, so you need to dereference the pointer when you call the function like this:

devowel(&B);
Click to expand...

Actually you don't. Because it is an array, C treats it as a pointer by default. You can either do devowel(&B[0]); or just devowel(B);

Good work on the variables and tabbing. Miles better. I'm having a few problems working out what the correct output should be:

"eeeeffeeoooiiiee" should become "f" according to your original post?

UPDATE1:

Spotted a nasty bug in devowel. You are doing a:

for (i = 1; i <= len+1 ; i++)

that means you are indexing past the end of the string. It should be

for (i = 1; i < len ; i++)

But why are you starting at 1?

UPDATE2:

There is a similar bug in your shift function. You are doing a:

for (i = shifter; i != len ; i++)

but since you are indexing into the string with a [i+1] you need to stop the loop one earlier. So make it:

for (i = shifter; i < len-1 ; i++)

There is a similar bug in dedouble.

UPDATE3:

As a suggestion, I would make an array of the vowels (or any other letters) that you want to remove. It would help the neatness. Then we can work on making a small change to dedouble which would stop it needing to be placed in a while loop :)

Am I sounding like a C Nazi? Sorry!
 
Last edited:
Caustic said:
"eeeeffeeoooiiiee" should become "f" according to your original post?

UPDATE1:

Spotted a nasty bug in devowel. You are doing a:

for (i = 1; i <= len+1 ; i++)

that means you are indexing past the end of the string. It should be

for (i = 1; i < len ; i++)

But why are you starting at 1?
Click to expand...

I forgot to mention the first letter of the string should be preserved wether it is a vowel or not. Does that make more sense? :)
 
peteruk said:
Does that mean my shift function still needs changing though?
Click to expand...

Yes, you must make sure that all array indexing stays inside its bounds. C does no checks to detect (or stop) this and array out-of-bounds problems will cause major headaches later.

EDIT:
However, there is always an exception to the rules. Strings in C are null terminated. So the string "test" is stored using 5 bytes, "test\0". So for most things you want to go through just the letters, but when removing a character and moving all along one, you want to move the null as well. So, although I said you want to do "i<len" for most, "i<=len" for the shift function actually makes sense. Some people don't like do it that way and instead do an if and then an explicit set, but I'm not judging!

In summary, I would do this:

Code: 
void shift(int shifter, char* word, int len){
    int i;
	
    for (i = shifter; i < len-1 ; i++)
        word[i] = word[i+1]; // len-1 makes sure i+1 never assumes the string is null terminated.

    word[len] = 0; // Make sure it is null terminated now.
}
 
Last edited:
You could use a While loop which stops when the character is null then you will not need to do the count and will notoverflow the inputted string.

Code: 
int something( char * s )
{
	while( *s++ )
	{
		//do your business
	}
	return //an int
}

But as thats a bit of a pain to read you can always just do the strlen inside the function rather than having to pass it in and make it a bit nicer to use.
 
You must log in or register to reply here.
Back
Top Bottom