• 0

C++ code resulting in error


Question

Hi there, I'm fairly new to c++, I've only been doing it for about two months, or so.

Today I was working on writing a program that would calculate the Lowest Common Multiple of two inputted numbers,

but for some reason I get an error message after entering the two numbers.

Here is the code:

#include <iostream>
#include <stdlib.h>
#include <math.h>
using namespace std;
int main() {
	cout << "This program will calculate the LCM of two numbers." << endl;
	//Set variables and arrays.
	int n, m, i, j;
	int mult[n];
	int multo[m];
	//Take input.
	cout << "Input the first number: ";
	cin >> n;
	cout << "Input the second number: ";
	cin >> m;
	//Set values for each element of each array
	//By calculating the multiples of each number.
	for (i = 0; i <= m; i++) {
		mult[i] = n * i;
		}
	for (i = 0; j <= n; i++) {
		multo[i] = m * i;
		}
	//Loop to compare all elements between both arrays.
	//Works by comparin all the elements of mult to the first value of
	//multo. Then by comparing all the elements of mult to the
	//second element of multo and continues to end of multo.
	for (j = 0; j <= n + 1; j++) {
		for (i = 0; i <= n + 1; i++) {
			if (mult[i] == multo[j]) { //compares the elements.
			   cout << mult[i] << " is the LCM." << endl;
			   }
			else { //If they are not equal, continue to compare.
				 continue;
				 }
				 }
				 }
	system ("pause");
	return 0;
}

note: I've tried changing the variables both for the comparison loop, and for both of the calculations loops, using different combinations etc...

It seems that I'm able to get as far as the input, but then I get the error:

LCM.exe has encountered a problem and needs to close. We are sorry for the inconvenience.

Any idea what I wrote wrong, where, and what I can do to fix it?

Thanks in advance.

Edited by Ryuurei
Link to comment
Share on other sites

13 answers to this question

Recommended Posts

  • 0

Hi, welcome to Neowin :)

Your error is that you're writing off the end of the array. There may be others but this is the most obvious error.

int mult[n];
int multo[m]

So say for example you set n = 10, mult is 10 elements wide. This means that you can place integers in array positions 0-9. The 10th, and final, element is actually mult[9].

In your current code you're looping from j = 0 to j <= n + 1, so if we set n = 10, you're looping 12 times for an array that is 10 elements big.

Try something like this

int array[10];
int i = 0;

for( i = 0; i &lt; 10; i++)
{
	// Code
}

Noticed another error. You shouldn't declare your arrays until after you have got the input from the user, as they contain uninitialised data.

Link to comment
Share on other sites

  • 0

I did some of the things you said and changes some things myself and now have:

#include &lt;iostream&gt;
#include &lt;stdlib.h&gt;
#include &lt;math.h&gt;
using namespace std;
int main() {
	cout &lt;&lt; "This program will calculate the LCM of two numbers." &lt;&lt; endl;
	//Set variables and arrays.
	int n, m, j;
	int i = 0;
	//Take input.
	cout &lt;&lt; "Input the first number: ";
	cin &gt;&gt; n;
	cout &lt;&lt; "Input the second number: ";
	cin &gt;&gt; m;
	int mult[n];
	int multo[m];
	//Set values for each element of each array
	//By calculating the multiples of each number.
	for (i = 0; i &lt;= m; i++) {
		mult[i] = n * i;
		}
	for (i = 0; i &lt;= n; i++) {
		multo[i] = m * i;
		}
	//Loop to compare all elements between both arrays.
	//Works by comparin all the elements of mult to the first value of
	//multo. Then by comparing all the elements of mult to the
	//second element of multo and continues to end of multo.
	for (j = 1; j &lt; n; j++) {
		for (i = 0; i &lt; n; i++) {
			if (mult[i] == multo[j]) { //compares the elements.
			   cout &lt;&lt; mult[i] &lt;&lt; " is the LCM." &lt;&lt; endl;
			   break;
				  }
				 }
				 }
	system ("pause");
	return 0;
}

But I'm still getting an error with any input.

Link to comment
Share on other sites

  • 0

Well first off, you're giving your arrays odd sizes. On these two lines...

		  int n, m, i, j;
		  int mult[n];
		  int multo[m];

... you are assigning the two arrays random sizes because you haven't yet assigned n and m values. Ideally, you need to declare the arrays sizes AFTER you have got values of m and n off the user.

In the loops, you're also overflowing.

For example, if you make the size of the array "5", then you're array looks like this...

[int] [int] [int] [int] [int]
  ^	 ^	 ^	 ^	 ^
  0	 1	 2	 3	 4

In your loops however, you have said...

 "for (i = 0; i &lt;= m; i++) { ... }"

...which will loop SIX times ...

[0] [1] [2] [3] [4] [5]

... to fix this, change "<=" to "<"

Does this make any sense?

========================================

EDIT: ViZioN beat me to it. In your corrected code, you're still using "<=" in your for loops.

 //Set values for each element of each array
	//By calculating the multiples of each number.
	for (i = 0; i &lt;= m; i++) {				   //&lt;-- Here
		mult[i] = n * i;
		}
	for (i = 0; i &lt;= n; i++) {					//&lt;-- And here
		multo[i] = m * i;
		}

Edited by Majesticmerc
Link to comment
Share on other sites

  • 0

I rewrote the code for the LCM program using some different tricks and functions, and this time it seems to work.

#include &lt;iostream&gt;
#include &lt;math.h&gt;
#include &lt;stdlib.h&gt;
using namespace std;
int mult[25];
int multo[25];
int main() {
	int n, i, m, c, d;
	cout &lt;&lt; "Enter first number: ";
	cin &gt;&gt; n;
	cout &lt;&lt; "Enter second number: ";
	cin &gt;&gt; m;
	if (n % m == 0) {
		  cout &lt;&lt; "The LCM is: " &lt;&lt; m &lt;&lt; endl;
		  }
	if (m % n == 0) {
		  cout &lt;&lt; "The LCM is: " &lt;&lt; n &lt;&lt; endl;
		  }
	for (i = 0; i &lt; 25; i++) {
		mult[i] = n * i;
		}
	for (i = 0; i &lt; 25; i++) {
		multo[i] = m * i;
		}
	for (c = 0; c &lt; 25; c++) {
		for (i = 0; i &lt; 25; i++) {
			if (mult[i] == multo[c]) {
						cout &lt;&lt; mult[i] &lt;&lt; " is the LCM." &lt;&lt; endl;
						}
				  }
				  }
	system ("pause");
	cout &lt;&lt; "The multiples are:" &lt;&lt; endl;
	for (i = 0; i &lt; 25; i++) {
		cout &lt;&lt; mult[i] &lt;&lt; " ------- " &lt;&lt; multo[i] &lt;&lt; endl;
			 }
	system ("pause");
	return 0;
}

Not a lot of differences, naturally, but it's giving good results.

The only thing I am not sure how fix is the output.

When one LCM is found to be 0 (since that's the default first array data, because of the equation I used), it writes:

0 is the LCM

(ex>) 24 is the LCM

48 is the LCM

and leaves it at that.

How can I change it so it doesn't output the 0 is LCM and everything after the (new) first output?

Link to comment
Share on other sites

  • 0

You could just put an if statement in

if ( mult[i] != 0 )
{
	cout &lt;&lt; mult[i] &lt;&lt; " is the LCM." &lt;&lt; endl;
}

I found this code during a quick search

#include &lt;iostream&gt;
using namespace std;

int main()
{
	int a, b, d, min;

	cout &lt;&lt; "Enter two numbers: ";
	cin &gt;&gt; a &gt;&gt; b;

	if ( a &gt; b )
	{
		min = b;
	}
	else
	{
		min = a;
	}


	for(d = 2; d&lt;min; d++)
	{
		if(((a%d)==0) &amp;&amp; ((b%d)==0))
		{
			break;
		}

		if(d == min)
		{
			cout &lt;&lt; "No common denominators\n";
			return 0;
		}
	}

	cout &lt;&lt; "The lowest common denominator is " &lt;&lt; d &lt;&lt; ".\n";

	return 0;
}

Seems to be quite a compact program for doing what you want, and requires no arrays :)I cleaned it up a bit to make it more readable too. From here

Link to comment
Share on other sites

  • 0
Thats a nicely elegant solution ViZioN. Like it!

Haha, I take no credit :p I merely tidied up the code a bit. I hate FOR loops / IF statements without brackets. Makes the code less readable and harder to understand.

Link to comment
Share on other sites

  • 0
Haha, I take no credit :p I merely tidied up the code a bit. I hate FOR loops / IF statements without brackets. Makes the code less readable and harder to understand.

True dat! :yes:

Link to comment
Share on other sites

  • 0

    //Set variables and arrays.
    int n, m, i, j;
    int mult[n];
    int multo[m];

I don't think that is very portable. You should be using constant sizes for allocating static arrays. As far as I can remember, gcc allows this as an extension or something, but there is no guarantee that others will. Instead you can use dynamic arrays if you don't know the size of the array until runtime but I would suggest using std::vector instead of c-arrays, it makes life a lot easier in the long run. Anyway, the solution ViZioN found is simple enough not to use any arrays :).

I recommend reading the faq from the father himself.

Link to comment
Share on other sites

  • 0
    //Set variables and arrays.
    int n, m, i, j;
    int mult[n];
    int multo[m];

I don't think that is very portable. You should be using constant sizes for allocating static arrays.

I don't think that particular example is specific to the code not being portable. As values have not yet been assigned to n and m, you're effectively allocating an array of a random size. You could be allocating a million element array or something.

But yes you're right, dynamic arrays are the way to go in instances where you won't know the sizes until runtime.

I recommend reading the faq from the father himself.

Thanks for the link, quite an interesting read :)

Link to comment
Share on other sites

  • 0

Thanks for all the help everyone!

I've finally completed the program now after taking a break, and having just come back to revisit it.

This complete version (which I named LCM_Clean -befitting, hm?-) has a lot of cout << endl; statements, as I'm sure you'll see. This is simply to keep things organized and looking good on the console output. I like it a lot.

#include &lt;iostream&gt;
#include &lt;math.h&gt;
#include &lt;stdlib.h&gt;
using namespace std;
int mult[25];
int multo[25];
int main() {
	again:
	while(1) {
	int n, i, m, c, d, a, o, p;
	cout &lt;&lt; "To use the LCM calculator, enter 1, enter 0 to exit: ";
	cin &gt;&gt; a;
	cout &lt;&lt; endl;
	if (a == 0) {
		  cout &lt;&lt; "Now exiting . . . " &lt;&lt; endl;
		  system ("pause");
		  return 0;
		  }
	cout &lt;&lt; "Enter first number: ";
	cin &gt;&gt; n;
	cout &lt;&lt; "Enter second number: ";
	cin &gt;&gt; m;
	if (n % m == 0) {
		  cout &lt;&lt; "The LCM is: " &lt;&lt; m &lt;&lt; endl;
		  cout &lt;&lt; endl;
		  goto again;
		  }
	if (m % n == 0) {
		  cout &lt;&lt; "The LCM is: " &lt;&lt; n &lt;&lt; endl;
		  cout &lt;&lt; endl;
		  goto again;
		  }
	for (i = 0; i &lt; 25; i++) {
		mult[i] = n * i;
		}
	for (i = 0; i &lt; 25; i++) {
		multo[i] = m * i;
		}
	cout &lt;&lt; endl;
	for (c = 0; c &lt; 25; c++) {
		for (i = 0; i &lt; 25; i++) {
			if (mult[i] == multo[c]) {
						cout &lt;&lt; mult[i] &lt;&lt; " is a common multiple." &lt;&lt; endl;
						break;
						}
				  }
				  }
	cout &lt;&lt; endl;
	system ("pause");
	cout &lt;&lt; endl;
	cout &lt;&lt; "Would you like to see the multiples of the numbers? ";
	cout &lt;&lt; endl;
	cout &lt;&lt; "enter 1 for yes, enter 0 for no: ";
	cin &gt;&gt; d;
	if (d == 1) {
	   cout &lt;&lt; "How many of the multiples of each number would you " &lt;&lt; endl;
	   cout &lt;&lt; "like to see: ";
	   cin &gt;&gt; p;
	   cout &lt;&lt; endl;
	   cout &lt;&lt; "The multiples of " &lt;&lt; n &lt;&lt; " are:" &lt;&lt; endl;
	   for (o = 1; o &lt;= p; o++) {
		   cout &lt;&lt; n * o &lt;&lt; endl;
		   }
	   cout &lt;&lt; endl;
	   system ("pause");
	   cout &lt;&lt; "The multiples of " &lt;&lt; m &lt;&lt; " are:" &lt;&lt; endl; 
	   for (o = 1; o &lt;= p; o++) {
		   cout &lt;&lt; m * o &lt;&lt; endl;
		   }
	   cout &lt;&lt; endl;
	   system ("pause");
	   cout &lt;&lt; endl;
				}
}
	system ("pause");
	return 0;
}

So thanks again everyone for all the advice, and input. I'm really quite pleased with how this turned out, being my first big program (written straight from memory) after my first two months of learning C++ as my first language.

Sincerely,

- Ryuurei

Link to comment
Share on other sites

  • 0
Thanks for all the help everyone!

I've finally completed the program now after taking a break, and having just come back to revisit it.

This complete version (which I named LCM_Clean -befitting, hm?-) has a lot of cout << endl; statements, as I'm sure you'll see. This is simply to keep things organized and looking good on the console output. I like it a lot.

So thanks again everyone for all the advice, and input. I'm really quite pleased with how this turned out, being my first big program (written straight from memory) after my first two months of learning C++ as my first language.

Sincerely,

- Ryuurei

You're very welcome. Glad you're happy with it. If you have any other problems, either post back here, or make a new thread!

Link to comment
Share on other sites

This topic is now closed to further replies.
  • Recently Browsing   0 members

    • No registered users viewing this page.