Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

If the number is present, the output should show the numbers that are not present.

.model small
.Stack 100
.data
            array   db 5, 12, 23, 45, 60, 67, 96, 100
            len     equ $-array
            key     db 100
            msg1    db "found$"
            msg2    db "not found$"
.code
    mov     ax, @data   
            mov     ds, ax  

            mov     si, 00h 
            mov     di, len-1   
            mov     cl, key 

again:      mov     bx, si      
            add     bx, di  
            shr     bx, 1       
            cmp     cl, array[bx]
            je  found   

            ja  down    

            mov     di, bx  
            dec     di      
            jmp     up      

down:       mov     si, bx  
            inc     si      

up:         cmp     si, di      
            jle     again       
            lea     dx, msg2    
            jmp     display 

found:   lea    dx, msg1    
display:  mov   ah, 09h 
            int     21h     
            int     3       
    end     
share|improve this question
add comment

2 Answers 2

  • It's important to provide comments in assembly, especially for register usage. Since registers cannot have unique names like variables, readers are left to deduce their purpose.

    You may, for instance, state the value set to a register in a certain line, and what that register is supposed to represent at that time. That way, you can keep track of the register's usage as it changes throughout the program.

  • Your variable initializations should have equal whitespace on both sides of the variable size:

    array   db   5, 12, 23, 45, 60, 67, 96, 100
    
  • This part is misaligned:

    .code
        mov     ax, @data   
                mov     ds, ax
    

    it should be:

    .code
        mov     ax, @data   
        mov     ds, ax
    

    Same thing here:

    found:   lea    dx, msg1    
    display:  mov   ah, 09h 
                int     21h     
                int     3       
        end
    

    it should be:

    found:    lea    dx, msg1    
    display:  mov    ah, 09h 
              int    21h     
              int    3       
    end
    

    I'd also add a linebreak between those two procedures.

share|improve this answer
add comment
.model small

Hard to imagine somebody writing new code for DOS, but okay...

        msg1    db "found$"
        msg2    db "not found$"

Seems to me these deserve better names:

fnd_msg db "found$"
not_fnd_msg db "not found$"

Or, you can get kind of tricky to save a little space:

not_fnd_msg db "not "
fnd_msg db "found$"

I'm somewhat hesitant to suggest overlapping the storage for the two strings this way. Under almost any other circumstances I'd probably frown on it, but if you're going to write for MS-DOS, using otherwise ugly tricks to save a little space is often nearly a necessity.

        mov     si, 00h 

You typically want to use xor or sub to clear a register:

xor si, si

In this case, however, you can probably skip that entirely -- you don't really need to use si at all. Especially in 16-bit mode, you typically get some real benefit from using the registers as they were designed, such as a count in cx.

        mov     di, len-1   
        mov     cl, key 

As such, I'd probably use:

mov cx, len-1
mov al, key
mov di, array

This improves readability considerably (at least for others who know what they're doing). If you're going to put the value to compare in cl, the count in di, and so on, you just about need to add a comment to point out that you're doing something unusual (and preferably, why). If you use the registers as intended, there's no real need for those comments, because the values are exactly where anybody reading the code will expect them to be. If you explicitly stated that you'd stored the count in cx, there's a pretty fair chance somebody reading it would think you were insulting their intelligence by pointing out the painfully obvious.

Then you can do the search with:

repne scasb

Once that's finished, you can use the z flag to see if the data was found or not. To avoid ugly jumps on both legs of an if/then/else type of construction, you can use a little trick. It depends on the fact that on an x86, a mov does't affect the flags. As such, you can do a mov of a default value, then do a conditional branch, and afterwards load the other possible value:

    mov bx, offset fnd_msg
    jz prnt_msg
    mov bx, offset not_fnd_msg
prnt_msg:
    mov ah, 09h
    int 21h

Although the logic here may not be immediately apparent to "outsiders", almost anybody accustomed to assembly language will recognize this very quickly (and if you don't use it, they'll wonder why, or just assume you don't know what you're doing).

Then you probably want to exit normally, not via a debug interrupt:

mov ax, 4c00h
int 21h
share|improve this answer
    
Good answer concerning code itself, not just formatting. Alternatively to using 4C DOS service, one could just do int 20h for normal termination. –  Ruslan Mar 19 at 6:25
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.