Email: Password: Remember Me | Create Account (Free)

Back to Subject List

Old thread has been locked -- no new posts accepted in this thread
???
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]);
}


List of 36 messages in thread
TopicAuthorDate
Speed control of running adancing LEDs            01/01/70 00:00      
   Duplicate Posting            01/01/70 00:00      
      This is just an inadequately understood homework problem            01/01/70 00:00      
   Design your code, make it pretty.            01/01/70 00:00      
   RE: please suggest some idea            01/01/70 00:00      
      I totally agree with ANDY            01/01/70 00:00      
         Very unhelpful comments            01/01/70 00:00      
            Thank you sir            01/01/70 00:00      
               While analysing the problem            01/01/70 00:00      
               Translation into assembler task of programmer.            01/01/70 00:00      
   'subb' instruction            01/01/70 00:00      
      Yes, debounce is definitely good to have.            01/01/70 00:00      
         Got Success !            01/01/70 00:00      
            What is the final code?            01/01/70 00:00      
               homework is done..            01/01/70 00:00      
               Surely not secret, at least for my seniors            01/01/70 00:00      
               Teacher will know            01/01/70 00:00      
                  Don't worry            01/01/70 00:00      
                     What code?            01/01/70 00:00      
                     You must show your effort.            01/01/70 00:00      
                        yes, it's another "gimmee", yet nobody sees it            01/01/70 00:00      
                           what's the problem            01/01/70 00:00      
                              Why ever source current for driving LEDs?            01/01/70 00:00      
                              true enough, but is that what he did?            01/01/70 00:00      
                           I think everybody saw that?            01/01/70 00:00      
                              Cleverness            01/01/70 00:00      
                                 Stange !            01/01/70 00:00      
                                    because it is            01/01/70 00:00      
                                    Yes, often very obvious when copied code turned in            01/01/70 00:00      
                              I agree with Andy            01/01/70 00:00      
                                 the key word is 'help'            01/01/70 00:00      
                                    May not be an achievement for you            01/01/70 00:00      
                                       So ... where's the evidence of your struggle?            01/01/70 00:00      
                                    problem can be outside            01/01/70 00:00      
                                    This is certainly correct!            01/01/70 00:00      
   About sinciarity            01/01/70 00:00      

Back to Subject List