2

I have been trying to get more into programming, so I've been trying to make a simple program that takes two numbers as input, and computes the lowest common multiple. I did this in Python because I don't know how to take input in Java. What happens now is the program just hangs after I input my numbers, and nothing happens. Any pointers here would be greatly appreciated. Thank you.

#LCM Calculator
#Author: Ethan Houston
#Language: Python
#Date: 2013-12-27
#Function: Program takes 2 numbers as input, and finds the lowest number
# that goes into each of them

def lcmCalculator(one, two):
    """ takes two numbers as input, computes a number that evenly 
        divides both numbers """
    counter = 2 #this is the number that the program tried to divide each number by.
                #it increases by 1 if it doesn't divide evenly with both numbers.
    while True:
        if one % counter == 0 and two % counter == 0:
            print counter
            break
        else:
            counter += 1

print "\nThis program takes two numbers and computes the LCM of them...\n"

first_number = input("Enter your first number: ")
second_number = input("Enter your second number: ")

print lcmCalculator(first_number, second_number)
ethanzh
  • 133
  • 2
  • 5
  • 19
  • 1
    If you're computing LCM as traditionally understood, both of your code comments have things backwards: for example, "finds the lowest number that goes into each of them" makes it sound like you're looking for factors of `one` and `two`. But you're looking for their multiples, right? – FMc Dec 27 '13 at 01:04
  • In Python 2, you should avoid using the `input` function (it can execute arbitrary Python code from user input!) - instead, when you want an integer, use `int(raw_input("Enter your first number: "))`. If you upgrade to Python 3, then `input` there behaves like the old `raw_input` (the old `input` behavior has gone away precisely because of this problem) and it becomes `int(input(...))`. Also as a matter of style, consider using `for counter in itertools.count():` instead of incrementing the counter manually. – lvc Dec 27 '13 at 01:05
  • Apart from the good solutions posted below a few comments. It's usually not a good idea to have a `while(True)` statement in your code, since potentially it will, well, run 'while true', say infinitely. So I'd recommend using `while(counter<=one*two)` which is the natural upper bound for lcm. Also you should use a return statement in your function, otherwise the print statement in your last line won't have any input to print. `return counter` should do the job if you adhere to my first suggestion as well. – Zakum Dec 27 '13 at 01:11
  • @Zakum Nothing wrong with `while True`. It is even a quite common pattern. – Hyperboreus Dec 27 '13 at 02:21

3 Answers3

6

Your logic is a bit off. This line:

if one % counter == 0 and two % counter == 0:

needs to be rewritten like this:

if counter % one == 0 and counter % two == 0:

Also, your function should return counter instead of print it. This has two advantages:

  1. It will keep the script from printing None at the end (the function's default return value).

  2. It allows you to condense these two lines:

    print counter
break

    into just one:

    return counter

Finally, as @FMc noted in a comment, you can improve the efficiency of the function by doing two things:

  1. Starting counter at the smaller of the function's two arguments.

  2. Incrementing counter by this value.

Below is a version of your script that addresses all this:

#LCM Calculator
#Author: Ethan Houston
#Language: Python
#Date: 2013-12-27
#Function: Program takes 2 numbers as input, and finds the lowest number
# that goes into each of them

def lcmCalculator(one, two):
    """ takes two numbers as input, computes a number that evenly 
        divides both numbers """
    counter = min_inp = min(one, two)
    while True:
        if counter % one == 0 and counter % two == 0:
            return counter
        else:
            counter += min_inp

print "\nThis program takes two numbers and computes the LCM of them...\n"

first_number = input("Enter your first number: ")
second_number = input("Enter your second number: ")

print lcmCalculator(first_number, second_number)

Oh, and one more thing. input in Python 2.x evaluates its input as real Python code. Meaning, it is dangerous to use with uncontrolled input.

A better approach is to use raw_input and then explicitly convert the input into integers with int:

first_number = int(raw_input("Enter your first number: "))
second_number = int(raw_input("Enter your second number: "))
  • Also, the counter can be incremented more rapidly -- by the larger of the two inputs, right? – FMc Dec 27 '13 at 01:01
  • @FMc - No, not actually. To see for yourself, copy the script in my answer and replace the incrementing line with `counter += max(one, two)`. Then, run it and give it two numbers, `2` and `3` for instance. It loops infinitely. –  Dec 27 '13 at 01:04
  • Right, but `min(one,two)` is correct. :) If we're hunting multiples, we ought to be able to stride by one multiple or the other, provided that we start the counter in the right place -- also `min(one,two)`, I guess. – FMc Dec 27 '13 at 01:07
  • 1
    You could easily start with `max(one,two)` in this case, since the minimal multiple has to be bigger or equal than/to the biggest of both numbers. – Zakum Dec 27 '13 at 01:15
  • 1
    @Zakum I thought I agreed with you ... but that's incorrect. :) This question is a BUG TRAP. You have to start at `min(one,two)` -- the same as your stride. Otherwise, you can loop forever because you are on a hopeless cycle: for example, try 13 and 17. – FMc Dec 27 '13 at 01:21
  • Ouch, that's true @FMc, my bad! – Zakum Dec 27 '13 at 01:24
  • Okay thank you. This works perfectly. One question though, what does min_inp = min(one, two) do? I know what min(one, two) does, but what is the purpose of the min_inp? – ethanzh Dec 27 '13 at 10:11
  • @ethanzh - It's just a variable to hold the value of `min(one, two)`. This keeps you from recalculating it with each iteration of the while-loop. –  Dec 27 '13 at 15:07
1

Try this:

#!/usr/local/cpython-2.7/bin/python

def lcmCalculator(one, two):
    """ takes two numbers as input, computes a number that evenly
        divides both numbers """
    counter = 2 #this is the number that the program tried to divide each number by.
                #it increases by 1 if it doesn't divide evenly with both numbers.
    while True:
        if counter % one == 0 and counter % two == 0:
            break
        else:
            counter += 1
    return counter

print "\nThis program takes two numbers and computes the LCM of them...\n"

first_number = int(input("Enter your first number: "))
second_number = int(input("Enter your second number: "))

print lcmCalculator(first_number, second_number)
dstromberg
  • 6,954
  • 1
  • 26
  • 27
1

You need to let the loop end if it finds no factor, instead of while True:

def lcmCalculator(one, two):

    counter = 2    
    while counter <= min(one, two):
        if one % counter == 0 and two % counter == 0:
            return counter
        else:
            counter += 1

    return "No common factor found"

print "\nThis program takes two numbers and computes the LCM of them...\n"

first_number = input("Enter your first number: ")
second_number = input("Enter your second number: ")

print lcmCalculator(first_number, second_number)
Holy Mackerel
  • 3,259
  • 1
  • 25
  • 41
  • He is looking for the leas common multiple (http://en.wikipedia.org/wiki/Least_common_multiple) so counter is necessarily going to be bigger then both the numbers. Although finding an upper bound is generally a good idea. Also your solution should provide a return statement in the function lcmCalculator. :) – Zakum Dec 27 '13 at 01:05
  • 1
    right, perhaps I misread the question then? From the code comments it seemed like the op was looking for the lowest common factor – Holy Mackerel Dec 27 '13 at 01:07
  • added the return statement. – Holy Mackerel Dec 27 '13 at 01:09