??? 09/10/10 05:12 Read: times |
#178504 - Still a lot of work to do Responding to: ???'s previous message |
if(flag_g==enter_val || flag_g==error_val || flag_g==mismatch_val) It is a common practice to use all capitals for #defined symbols, to make it easy to know that a constant is involved, and not a variable. Most people who read the above line would assume that "enter_val", "error_val" and "mismatch_val" are variables. lcd_disp(special_q); // 0x3f is hex value of ascii character "?" (question mark) The symbols used in a program should have descriptive names. With good names, you don't need to spend a lot of time trying to write comments. Your comment mentions 0x3f is hex value of ascii character "?". But where do you see any 0x3f? And why do you use a #define named special_q that hides your question mark? Wouldn't it have been better to just have: lcd_disp('?'); If rewritten as in my suggestion - do you still see a need for a comment mentioning that you are trying to display a question mark on the display? void change_key(); void close(); void open(); void alarm(); If you are programming in C, then you should write: void change_key(void); void close(void); void open(void); void alarm(void); to inform that the functions do not take any parameters. Leaving empty in C means a function with an unknown parameter list. It's only in C++ that the word "void" can be skipped and still mean zero parameters. if(flag_g==error_val || flag_g==mismatch_val) { flag_g=0; continue; } flag_g=0; There is still no reader who are able to understand what flag_g=0 means, for the simple reason that flag_g is a totally meaningless variable name. You are flagging what character "G"? Flagging it with a checkered flag for being first? Black flag for being thrown out of the race? flag_g seems to be an input result - so why not a variable name saying something like that? Another thing. It's always nice with variables getting their assignments before being used. If you aren't happy with the input you do a continue to the top of the while loop. Wouldn't it be nicer to clear flag_g early in the while loop instead of having it cleared after the input attempt and before you retry the input? With it cleared directly before the for loop, it is way easier for people to see the clear. On the other hand - does it need to be cleared before next attempt to get a password? Is it possible to enter the for loop without getting at least one assign to it? You also clear flag_g if you don't do a continue. Why? Three lines down you assign new value to it anyway. // The menu is shown here j=0; while (j<=100) { j++; You are not telling us what "j" is. Just a random loop variable? If it isn't a random loop variable, why isn't it given a name that tells what it is? And if it is a random loop variable - what is the magic purpose of the value 100? And why use a while requiring three lines instead of a for (j = 0; j <= 100; j++) { ... }? Besides - did you intend 100 or 101 iterations? /*void delay_ms(unsigned char u) { unsigned char i; unsigned int j; for(i=0; i<u; i++) { for(j=0; j<1275; j++); } }*/ Don't keep garbage in your code. You have written an alternative for your software-based delay_ms() - no one (not even you) are interested in seeing the old implementation too. Save the code in a backup file somewhere if you really, really think that the code lines are so magically beautiful that you want to save them for the world to see in 100 years ;) But the source code lines of your program should focus on what your program does - not what it did a week ago. // delay timer 50 ms #define Th50 0xb4 #define Tl50 0x00 How do anyone know that the constants is for 50ms by just looking at the name of the constants? Wouldn't it have helped if you had added "ms" somewhere in the name? And wouldn't it have been nice if your comment had somehow described hw you computed the value, to help recompute a different value in case you change the crystal frequency? void timer_50() Who can figure out that timer_50() is meant to mean that you initialize the timer for 50 milliseconds? Could just as well have meant 50 timer ticks or something else. void timer() if "timer_50()" means 50 ms, does "timer()" then mean zero milliseconds? If I wake you in the middle of the night, and ask you "timer()" - would you have any chance at all to figure out the meaning of the word? Would you have a better chance if I asked "start_1ms_delay()"? By the way - timer() doesn't contain many lines of code. And it is used at a single location. And it isn't meaningful to call on it's own. Maybe the lines inside timer() should have stayed within your implementation of void delay_ms(). //if crystal frequency is 11.0592 MHz the delay dute to timer is about 1 ms Interesting comment inside delay_ms(). Shouldn't the comment instead be close to the source lines that contains the values you need to initialize the timer with? // Four second delay is to be expected if(a==1) { delay_ms(255); delay_ms(255); delay_ms(255); delay_ms(255); delay_ms(255); delay_ms(255); delay_ms(255); delay_ms(255); delay_ms(255); delay_ms(255); delay_ms(255); delay_ms(255); delay_ms(255); delay_ms(255); delay_ms(255); delay_ms(255); delay_ms(255); delay_ms(255); } For this architecture and with this compiler, an int is 16 bit large and can store a value in the range -32768..32767 if signed or 0..65535 if unsigned. But the C standard requires an int to be at least 16 bit large. Don't you think you should write a better delay primitive so you don't need the above code? How about delay_ms(4000) or maybe delay_seconds(4) instead? void close() You closing a file or a back account or a door? If closing a lock - wouldn't it be better if people could clearly see that by you having a function called close_lock()? unsigned char keypad() { unsigned char i,temp=0,temp1=0,temp2=0xff,wait=0;//,wait1=0; bit flag_x=0; while(1) { //write program to prevent indefinite waiting here //wait++; timer_50(); hex = 0x0f; //delay_ms(10); for(i=0; i<4; i++) { if(i==0) hex = temp1 = 0xEF; else if(i==1) hex = temp1 = 0xDF; else if(i==2) hex = temp1 = 0xBF; else if(i==3) hex = temp1 = 0x7F; temp = hex; temp &= 0x0f; temp1&= 0xf0; ... Lots of "temp" variables here. And "i" and "flag_x" aren't exactly descriptive either. Are there no part of your code that involves rows or columns on the keypad? Doesn't seem to be from your symbol names. i can only take the values 0, 1, 2, 3. So why do you have four separate assigns "hex = temp1" directly followed by "temp = hex"? |
Topic | Author | Date |
Please assess the c code written by me | 01/01/70 00:00 | |
Lots of details to improve on | 01/01/70 00:00 | |
REALLY confusing | 01/01/70 00:00 | |
timer delay review | 01/01/70 00:00 | |
what about this timer program | 01/01/70 00:00 | |
Still a lot of work to do | 01/01/70 00:00 | |
I have implemented many of the suggestions | 01/01/70 00:00 | |
Still not thinking about your symbol names | 01/01/70 00:00 | |
variable name dilema | 01/01/70 00:00 |