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.

Since this problem involves small numbers (particularly with a small loop count of 100), it's possible to ease the modulo operation setup by simply working with 16-bit and 8-bit registers:

[AX] (16-bit register)
---------------------- = [AH] (remainder)
[other 8-bit register]

My main concern is with the layout. Every "basic" high-level implementation I've seen has a check and print together for each case. I've found it easier to do the same thing here, but I'm not sure if that would also be too readable in assembly.

I'm also aware that it's good to minimize register-moving. Unfortunately, I still do that with every case since I'm incrementing with one register (CX) and using another (AX) for the dividend. I can stick to AX for both, but that may involve keeping a copy of the current counter value, which may just make the code a bit more complicated. I suppose it's not much of a problem here anyway.

Macros used:

  • nwln - prints a newline
  • PutStr - prints a defined string
  • PutInt - prints a 16-bit integer value

It's not necessary to address the macros; they do work properly.

%include "macros.s"

.DATA

fizz_lbl:       DB    "Fizz", 0
buzz_lbl:       DB    "Buzz", 0
fizzbuzz_lbl:   DB    "FizzBuzz", 0

.CODE
.STARTUP

xor   CX, CX ; counter

main_loop:
   inc   CX
   cmp   CX, 100

   jg    done

   fizzbuzz_check:
      mov   AX, CX ; dividend = counter
      mov   BH, 15 ; divisor
      div   BH     ; (counter / 15)

      cmp   AH, 0            ; counter divisible by 15?
      je    print_fizzbuzz   ; if so, proceed with printing

      jmp   fizz_check       ; if not, try checking for fizz

      print_fizzbuzz:
          PutStr   fizzbuzz_lbl
          nwln
          jmp      main_loop

   fizz_check:
      mov   AX, CX ; dividend = counter
      mov   BH, 3  ; divisor
      div   BH     ; (counter / 3)

      cmp   AH, 0        ; counter divisible by 3?
      je    print_fizz   ; if so, proceed with printing

      jmp   buzz_check   ; if not, try checking for buzz

      print_fizz:
          PutStr   fizz_lbl
          nwln
          jmp      main_loop

   buzz_check:
      mov   AX, CX ; dividend = counter
      mov   BH, 5  ; divisor
      div   BH     ; (counter / 5)

      cmp   AH, 0        ; counter divisible by 5?
      je    print_buzz   ; if so, proceed with printing

      jmp   print_other  ; if not, then can only display number

      print_buzz:
          PutStr   buzz_lbl
          nwln
          jmp      main_loop

   print_other:
      PutInt   CX
      nwln
      jmp   main_loop

done:
   .EXIT
share|improve this question
1  
Nice! +1 for the effort writing this is a low level language. May I suggest posting the result set? –  Phrancis Jul 12 '14 at 22:59
1  
@Phrancis: For verification of successful execution, you mean? –  Jamal Jul 12 '14 at 23:02
    
Yeah basically. Though I don't doubt it did exec. –  Phrancis Jul 12 '14 at 23:18
1  
@Phrancis: I only hesitate because it'll take up a lot (possibly unnecessary) room. Would a link to a screenshot suffice? –  Jamal Jul 12 '14 at 23:21
1  
Where can we obtain macros.s? –  200_success Jul 13 '14 at 2:24

2 Answers 2

up vote 22 down vote accepted

Since we're doing this in assembly language, it makes sense to do it much more efficiently than is typically done in high level languages. Otherwise, why bother with assembly language? So with that said, there are ways that this can be made much, much more efficient.

Avoid division

The div instruction in x86 is one of the slower instructions possible. Since we already know that we're looking for numbers divisible by 3, 5 or both, what would make far more sense is to simple keep countdown counters for both. Your initialization currently says:

    xor cx, cx

It could be easily expanded to say:

    xor cx, cx
    mov bx,0503h  ; set bh = 5 counter, bl = 3 counter

Then instead of dividing, simply decrement:

    inc cx
    cmp cx, 100
    jg done
    dec bh
    dec bl
    cmp bx, 0
    je print_fizzbuzz
    cmp bl, 0
    je print_fizz
    cmp bh, 0
    je print_buzz
print_other:

Naturally the various print_... routines would have to reset bh, bl or both as well as printing.

Improve formatting

Generally speaking, assembly language code is not indented in the way you have your code indented. It's much more linear, with the only indentation for assembly language statements or directives.

Consider better I/O

Your output routines are not shown, but it's likely that it would be more efficient to keep the numeric output in string form, incrementing each ASCII digit and emitting the string, rather than repeatedly converting from binary register contents to a string value.

share|improve this answer

Use local labels

All of your labels are global labels.

Since all these labels are trying to complete the same task and they all work together, they should all be grouped under a single global variable, and have the rest of the labels be local.

For example, you would change this label:

fizzbuzz_check:

to:

.fizzbuzz_check:

Also, it's just better practice.


Different conditional jump

At the end of each check for either Fizz, Buzz, or FizzBuzz, you do something like this:

  je    print_fizzbuzz   ; if so, proceed with printing

  jmp   fizz_check       ; if not, try checking for fizz

  print_fizzbuzz:

This could be shortened to:

  jne    main_loop

  print_fizzbuzz:

If the jne doesn't pass, execution will fall through to print_fizzbuzz


Versatility

Right now, your code only supports Fizz, Buzz, and Fizzbuzz.

But what if you wanted to change things up a bit? Say you wanted to say "Fizz" every fourth number?

To do this, you'd be adding quite a chunk of code.

Although, there is an easier way to do this; use strucs.

Say you created this struc:

struc message

    .say: resb 10
    .num: resb 1

endstruc

You could then do something create a bunch of messages easily like this:

messages:
    db "FizzBuzz", 0, 0
    db 15

    db "Buzz",0,0,0,0,0,0
    db 5

    db "Fizz",0,0,0,0,0,0
    db 3

    db 0,0,0,0,0,0,0,0,0,0; so, when iterating, can know if the end has been reached
    db 0

(The extra 0's are for filling up the 10 bytes given for the name) (Note the order: you want greatest to least)

And, you can easily

Now, in your main code, you can easily iterate through messages and, if the counter is evenly divisible by the value in the num field, then you log the say field.

Now, the code could be written like this:

xor cx, cx

main_loop:
    inc cx
    cmp cx, 100

    jg .done

    call search

    jmp main_loop

.done:
    .EXIT

search:
    mov si, messages

.next:
    mov ax, cx
    mov bh, [si + message.num]; divisor
    div bh

    cmp ah, 0; was evenly divisible
    je .print_message

    add si, message_size
    cmp byte [si], 0; the next item in `messages` is the terminator
    jne .next

    jmp .print_num

.print_message:
    PutStr [si + message.say]
    nwln
    ret

.print_num:
    PutInt cx
    nwln
    ret

Note: This was troublesome to test out without macros.s so if there are any issues, notify me

share|improve this answer

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.