??? 10/07/11 15:19 Read: times Msg Score: +2 +2 Good Answer/Helpful |
#184130 - Very unhelpful comments Responding to: ???'s previous message |
DESGN5: CALL CLEAR ;Sub Routine for pattern5 How can anyone think that "DESIGN5" is a good and descriptive name for a function? How would you say that "Sub Routine for pattern5" is descriptive? In what way will any reader be able to look at this comment and learn something new? What is different from the "DESIGN5" label? What, exactly, is "pattern5"? It isn't exactly uncommon that people do document their code with things like: x----- -x---- --x--- ---x-- ----x- -----x ----x- ---x-- --x--- -x---- Suddenly, a reader can visualize the result of the code. Isn't it obvious that comments are intended to help the reader - not just be a duplication of the source code using a different language? The code also contains lots of stupid comments that tells what a specific assembler instruction does. Why comment what an instruction does? Isn't an assembler programmer expected to already know the meaning of an instruction that compares a specific register with a constant number? Wouldn't it be way better to document what problem you are trying to solve, and why you need to perform the task, and why you have selected this specific method? Next thing - between each step of your output, you call a delay. So why do this delay have to perform lots of tests to figure out what delay constants to use? Wouldn't it be way better to have the code that processes the up/down buttons to check if the delay is already at max, or if the delay constant should be increased? Then the delay loop could just pick up the iteration count value from a variable without caring what speed it represents. So: function delay() { buttons = gpio_port & BUTTON_BITS; if (buttons == BUTTON_UP) { // Up button pressed - speed up display frequency (i.e. reduce delay) if (delay > MIN_DELAY) delay -= DELAY_STEP; } else if (buttons == BUTTON_DOWN) { // Down button pressed - slow down display frequency (i.e. increase delay) if (delay < MAX_DELAY) delay += DELAY_STEP; } else { // Zero or multiple buttons pressed - do nothing. } count = delay; while (count > 0) { count--; do_microdelay(); } } And why do you have so huge duplications of code. Why: do_random(); delay(); clear_all(); do_random(); delay(); clear_all(); ... instead of do_random(); delay_and_clear(); do_random() delay_and_clear(); ... And why not then continue with: emit(0x0001); emit(0x0002); emit(0x0004); emit(0x0008); emit(0x0010); emit(0x0020); ... Which could then be rewritten as: for (i = 0; i < NELEM(pattern); i++) { emit(pattern[i]); } |