Mixing
![Can micromanaging issue could be brought up as an ethical issue? [closed]](https://blogger.googleusercontent.com/img/b/R29vZ2xl/AVvXsEgjbpfN9tAutmK93bJRC3ZoROZzi2TJDms5n8_qJuhgE0a9b52OOHayv3NGT8igAdFL7byXNst-_1DZK5SjrIJ28_6RQPUpBROqMs5s6jo-ZsjX8kjDwfxJufIitH3TaQRXWaGSQKRQib-f/s72-c/1.jpg)
Add multiples of 3 or 5 below 1000, Can this code be optimised. Project Euler #1
Clash Royale CLAN TAG#URR8PPP
up vote
1
down vote
favorite
I have seen some solution here for the same problem, this and this I have found better ways to solve this problem. I just want to know how good or bad my code and way of approaching this problem is.
using System;
public class Program
{
public static void Main()
int sum = 0;
for (int i = 0; 3 * i < 1000; i++)
sum += 3 * i;
if (5 * i < 1000 && (5 * i) % 3 != 0)
sum += 5 * i;
Console.WriteLine(sum );
c# beginner programming-challenge
asked 3 hours ago
Grv10India
63
New contributor
Grv10India is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
add a comment |Â
up vote
1
down vote
favorite
I have seen some solution here for the same problem, this and this I have found better ways to solve this problem. I just want to know how good or bad my code and way of approaching this problem is.
using System;
public class Program
{
public static void Main()
int sum = 0;
for (int i = 0; 3 * i < 1000; i++)
sum += 3 * i;
if (5 * i < 1000 && (5 * i) % 3 != 0)
sum += 5 * i;
Console.WriteLine(sum );
c# beginner programming-challenge
asked 3 hours ago
Grv10India
63
New contributor
Grv10India is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
1
Console.WriteLine(sum + "");looks weird, why don't you justConsole.WriteLine(sum);?
– nalka
2 hours ago
add a comment |Â
up vote
1
down vote
favorite
up vote
1
down vote
favorite
I have seen some solution here for the same problem, this and this I have found better ways to solve this problem. I just want to know how good or bad my code and way of approaching this problem is.
using System;
public class Program
{
public static void Main()
int sum = 0;
for (int i = 0; 3 * i < 1000; i++)
sum += 3 * i;
if (5 * i < 1000 && (5 * i) % 3 != 0)
sum += 5 * i;
Console.WriteLine(sum );
c# beginner programming-challenge
asked 3 hours ago
Grv10India
63
New contributor
Grv10India is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
I have seen some solution here for the same problem, this and this I have found better ways to solve this problem. I just want to know how good or bad my code and way of approaching this problem is.
using System;
public class Program
{
public static void Main()
int sum = 0;
for (int i = 0; 3 * i < 1000; i++)
sum += 3 * i;
if (5 * i < 1000 && (5 * i) % 3 != 0)
sum += 5 * i;
Console.WriteLine(sum );
c# beginner programming-challenge
c# beginner programming-challenge
asked 3 hours ago
Grv10India
63
New contributor
Grv10India is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
asked 3 hours ago
Grv10India
63
New contributor
Grv10India is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
edited 2 hours ago
asked 3 hours ago
Grv10India
63
New contributor
Grv10India is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
asked 3 hours ago
Grv10India
63
asked 3 hours ago
Grv10India
63
63
New contributor
Grv10India is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
New contributor
Grv10India is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
Grv10India is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
1
Console.WriteLine(sum + "");looks weird, why don't you justConsole.WriteLine(sum);?
– nalka
2 hours ago
add a comment |Â
1
Console.WriteLine(sum + "");looks weird, why don't you justConsole.WriteLine(sum);?
– nalka
2 hours ago
1
1
Console.WriteLine(sum + ""); looks weird, why don't you just Console.WriteLine(sum);?– nalka
2 hours ago
Console.WriteLine(sum + ""); looks weird, why don't you just Console.WriteLine(sum);?– nalka
2 hours ago
add a comment |Â
4 Answers
4
active
oldest
votes
up vote
2
down vote
The main problem
The main issue here is that you define i in function of it being a "factor of 3", and then also try to use this number as a "factor of 5". This doesn't make any sense - as there is no inherent relationship between being a threefold and a fivefold number.
It would've made sense if you were doing it for 3 and 6, for example, because 3 and 6 are related:
//i*3 is obviously always a multiple of 3
sum += 3 * i;
if ((3*i) % 2 == 0)
//Now we know that i*3 is also a multiple of 6
But that is not the case here.
The for loop's readability
I understand your idea, you wanted to only iterate over factors of three which keep the multiple under 1000. While I will suggest to change this approach (later in the answer), your approach could've been written in a much more readable manner:
for( int i = 0 ; i < 1000 ; i+=3 )
This will iterate over i values of 0,3,6,9,12,15,... and will skip all values inbetween.
The benefit here is that you don't need to work with i*3 all the time, you can just use i itself.
You will need to iterate over the multiple of 5 separately. However, should you keep using this approach, I would always suggest splitting these loops anyway.
The algorithm
Your approach works, but it's not the easiest approach. If I put your approach into words:
Add every multiple of 3 to the sum.
Also add the multiple of 5, but only if it's still below 1000, and it's not already divisble by 3.
The issue here is in how you handle the multiples of five. You're working with an i that is defined as the allowed values for threefolds. For any i > 200, you're effectively having to manually exclude this value. You're using a different approach for the 5 than you are using for the 3, even though the logic is exactly the same. That's not a reusable approach.
Secondly, there is a readability problem. Your code should be trivially readable, and I simply wasn't able to understand your intention. I had to google what the question was before I could understand what your code was trying to achieve.
So let me offer a better approach, first putting it into words:
- Check every number from 0 to 1000 (not including 1000)
If it is divisible by 3 or it is divisible by 5, then add it to the sum.
This can be put into code, step by step:
// Check every number from 0 to 1000 (not including 1000)
for(int i = 0; i < 1000; i++)
isDivisibleBy5)
//then add it to the sum
sum += i;
Note how the code exactly mirrors my algorithm description.
You don't need to use the booleans. I simply added them to simplify the example. if(i % 3 == 0 || i % 5 == 0) would be equally okay to use because it's still reasonably readable.
If the calculations become more complex, I suggest always using the booleans so you neatly break your algorithm down to small and manageable steps. It will do wonders for your code readability, and it does not impact performance (the compiler will optimize this in a release build).
A LINQ variation
This can be further shortened using LINQ:
var sum = Enumerable.Range(0,1000)
.Where(i => i % 3 == 0 || i % 5 == 0)
.Sum();
LINQ is just a nicer syntaxt, but it uses a for/foreach iteration in the background, so I suspect it won't be much more performant than the previous example. But I do consider this highly readable.
Maximizing performance
The previous suggestion maximizes readability, but it does so at the cost of performance, as it now has to loop over 1000 values and evaluate them all. You already linked several other answer that clearly dive deeper into the code in order to maximize the performance, I hope you can see that this dramatically impacts the readability.
For example:
public void Solve()
result = SumDivisbleBy(3,999)+SumDivisbleBy(5,999)-SumDivisbleBy(15,999);
private int SumDivisbleBy(int n, int p)
return n*(p/n)*((p/n)+1)/2;
By itself, I would have no idea what this code does. I can sort of understand the intention of Solve(), but it's not quite apparent how SumDivisbleBy() works.
SumDivisbleBy() has effectively become impossible to maintain. If you needed to introduce a change, you would effectively have to reverse engineer it before you can alter it. This means that starting from scratch is the better option, which is clearly not a good thing.
However, when performance is the main focus, this is acceptable. I would, however, strongly urge you to document the algorithm in comments specifically to help future readers in understanding how/why this works.
Note that AlanT's answer contains a more readable variant of the SumDivisbleBy() method, which already helps a lot with understanding the algorithm. The clear naming used clarifies what the algorithm does, which is the main goal of writing readable code (explaining why something works is only a secondary goal and not always required).
answered 1 hour ago
Flater
2,800819
How about using two counters, one withi+=3and the other one withj+=5? Wouldn't it be even more efficient without the%?
– t3chb0t
1 hour ago
@t3chb0t: Sure but then you have to iterate twice. I haven't crunched the numbers on it but it seems to be a "pick your poison" type deal. Either iterate twice, or iterate once and validate the numbers. Besides, if you're going for performance, you're waaaay better off with theSumDivisbleByexample I linked, as this only requires one computation per divisor (3 computations) in total) and no iteration at all.
– Flater
1 hour ago
@t3chb0t: A propos, if you use thei+=3approach, you will always have to have a secondi+=5loop. OP's code relies on the "factors of three" (from 0 to 333) inherently including all "factors of five" (from 0 to 200). Using thei +=3approach,iis no longer a list of "factors of three" but rather "multiples of three", which no longer inherently include all "multiples of five"; thus requiring you to iterate those separately.
– Flater
1 hour ago
Not exactly... I was thinking about a singleforloop with two variables...
– t3chb0t
1 hour ago
@t3chb0t: I don't see how that would work unless you then use a combined condition, i.e.for(int i = 0, j=0; i < 1000 || j < 1000 ; i+=3,j+=5). While technically possible, I don't see the benefit of doing so. It dramatically lowers readability, and the # of operations is pretty much exactly the same regardless of merging theforloops or keeping them separate. If anything, you're going to end up incrementingj333 times (becauseiwill be incremented 333 times) while you only need to increment it 200 times, but since theforis still working withi, it also keeps incrementingj.
– Flater
1 hour ago
 |Â
show 2 more comments
up vote
0
down vote
Thanks @Henrik Hansen it helped.
This is optimised code below according to the WIKI.
But here is the tradeoff, this has to loop 1000 times. where above solution need to run at most 333 times.
using System;
public class Program
public static void Main()
int sum = 0;
for (int i = 0; i < 1000; i++)
Console.WriteLine(sum );
answered 2 hours ago
Grv10India
63
New contributor
Grv10India is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
Yes, but you should read a bit further -:)
– Henrik Hansen
2 hours ago
add a comment |Â
up vote
0
down vote
I'm afraid that the solution given is not better than the referenced C one
(although the C solution could be generalized and made a bit easier to understand)
Why?
The code under review has a outer loop that is executed 333 times (with a multiplication in each condition check). We can remove the multiplication to do the job a bit more cheaply
for (int i = 1; i <= 333; i++)
But, even with the restructured loop we still do
this, sum += 3 * i;, 333 times and then
this, 5 * i < 1000 && (5 * i) % 3 != 0, 333 times
As opposed to
S1= a*(b+c)/2;, once
s2= k*(i+j)/2;, once
s3= n*(m+o)/2;, once
S= S1 + s2 - s3;, once
It is a bit of a cheat in the C solution that the various magic numbers are hardcoded; but even allowing for a function to calculate the values
var sum3 = SumDivisibleBy(999,3);
var sum5 = SumDivisibleBy(999,5);
var sum15 = SumDivisibleBy(999,15);
var sum35 = sum3 + sum5 - sum15;
private static int SumDivisibleBy(int range, int divisor)
var top = range/divisor;
var ret = divisor*top*(top+1)/2;
return ret;
is a lot cheaper than brute-forcing the solution.
In the original solution, for numbers under 1000, we need to loop 333 times, for numbers under 10000, we need to loop 3333 times,... It doesn't scale well. For the other solution, there are the same number of operations each time.
answered 2 hours ago
AlanT
2,8641015
add a comment |Â
up vote
0
down vote
In order to make the algorithm more useful, you could wrap it in a function with 3 arguments:
static int SumFactors(int m, int n, int max)
int sum = 0;
for (int i = 0; m * i < max; i++)
sum += m * i;
if (n * i < max && (n * i) % m != 0)
sum += n * i;
return sum;
Although your algorithm is straight forward and intuitive as a brute force algorithm, it is also the most inefficient way to do the trick.
You can find inspiration to an efficient solution here
answered 2 hours ago
Henrik Hansen
5,0081619
I'm afraid this qualifies as a link-only answer and is off-topic :-( you don't mind if I downvote you this one time? ;-P
– t3chb0t
2 hours ago
@t3chb0t: Well, it should maybe have been a comment rather than an answer. I thought the wiki-link do the job for an answer and that I couldn't do it any better.
– Henrik Hansen
1 hour ago
add a comment |Â
4 Answers
4
active
oldest
votes
4 Answers
4
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
2
down vote
The main problem
The main issue here is that you define i in function of it being a "factor of 3", and then also try to use this number as a "factor of 5". This doesn't make any sense - as there is no inherent relationship between being a threefold and a fivefold number.
It would've made sense if you were doing it for 3 and 6, for example, because 3 and 6 are related:
//i*3 is obviously always a multiple of 3
sum += 3 * i;
if ((3*i) % 2 == 0)
//Now we know that i*3 is also a multiple of 6
But that is not the case here.
The for loop's readability
I understand your idea, you wanted to only iterate over factors of three which keep the multiple under 1000. While I will suggest to change this approach (later in the answer), your approach could've been written in a much more readable manner:
for( int i = 0 ; i < 1000 ; i+=3 )
This will iterate over i values of 0,3,6,9,12,15,... and will skip all values inbetween.
The benefit here is that you don't need to work with i*3 all the time, you can just use i itself.
You will need to iterate over the multiple of 5 separately. However, should you keep using this approach, I would always suggest splitting these loops anyway.
The algorithm
Your approach works, but it's not the easiest approach. If I put your approach into words:
Add every multiple of 3 to the sum.
Also add the multiple of 5, but only if it's still below 1000, and it's not already divisble by 3.
The issue here is in how you handle the multiples of five. You're working with an i that is defined as the allowed values for threefolds. For any i > 200, you're effectively having to manually exclude this value. You're using a different approach for the 5 than you are using for the 3, even though the logic is exactly the same. That's not a reusable approach.
Secondly, there is a readability problem. Your code should be trivially readable, and I simply wasn't able to understand your intention. I had to google what the question was before I could understand what your code was trying to achieve.
So let me offer a better approach, first putting it into words:
- Check every number from 0 to 1000 (not including 1000)
If it is divisible by 3 or it is divisible by 5, then add it to the sum.
This can be put into code, step by step:
// Check every number from 0 to 1000 (not including 1000)
for(int i = 0; i < 1000; i++)
isDivisibleBy5)
//then add it to the sum
sum += i;
Note how the code exactly mirrors my algorithm description.
You don't need to use the booleans. I simply added them to simplify the example. if(i % 3 == 0 || i % 5 == 0) would be equally okay to use because it's still reasonably readable.
If the calculations become more complex, I suggest always using the booleans so you neatly break your algorithm down to small and manageable steps. It will do wonders for your code readability, and it does not impact performance (the compiler will optimize this in a release build).
A LINQ variation
This can be further shortened using LINQ:
var sum = Enumerable.Range(0,1000)
.Where(i => i % 3 == 0 || i % 5 == 0)
.Sum();
LINQ is just a nicer syntaxt, but it uses a for/foreach iteration in the background, so I suspect it won't be much more performant than the previous example. But I do consider this highly readable.
Maximizing performance
The previous suggestion maximizes readability, but it does so at the cost of performance, as it now has to loop over 1000 values and evaluate them all. You already linked several other answer that clearly dive deeper into the code in order to maximize the performance, I hope you can see that this dramatically impacts the readability.
For example:
public void Solve()
result = SumDivisbleBy(3,999)+SumDivisbleBy(5,999)-SumDivisbleBy(15,999);
private int SumDivisbleBy(int n, int p)
return n*(p/n)*((p/n)+1)/2;
By itself, I would have no idea what this code does. I can sort of understand the intention of Solve(), but it's not quite apparent how SumDivisbleBy() works.
SumDivisbleBy() has effectively become impossible to maintain. If you needed to introduce a change, you would effectively have to reverse engineer it before you can alter it. This means that starting from scratch is the better option, which is clearly not a good thing.
However, when performance is the main focus, this is acceptable. I would, however, strongly urge you to document the algorithm in comments specifically to help future readers in understanding how/why this works.
Note that AlanT's answer contains a more readable variant of the SumDivisbleBy() method, which already helps a lot with understanding the algorithm. The clear naming used clarifies what the algorithm does, which is the main goal of writing readable code (explaining why something works is only a secondary goal and not always required).
answered 1 hour ago
Flater
2,800819
How about using two counters, one withi+=3and the other one withj+=5? Wouldn't it be even more efficient without the%?
– t3chb0t
1 hour ago
@t3chb0t: Sure but then you have to iterate twice. I haven't crunched the numbers on it but it seems to be a "pick your poison" type deal. Either iterate twice, or iterate once and validate the numbers. Besides, if you're going for performance, you're waaaay better off with theSumDivisbleByexample I linked, as this only requires one computation per divisor (3 computations) in total) and no iteration at all.
– Flater
1 hour ago
@t3chb0t: A propos, if you use thei+=3approach, you will always have to have a secondi+=5loop. OP's code relies on the "factors of three" (from 0 to 333) inherently including all "factors of five" (from 0 to 200). Using thei +=3approach,iis no longer a list of "factors of three" but rather "multiples of three", which no longer inherently include all "multiples of five"; thus requiring you to iterate those separately.
– Flater
1 hour ago
Not exactly... I was thinking about a singleforloop with two variables...
– t3chb0t
1 hour ago
@t3chb0t: I don't see how that would work unless you then use a combined condition, i.e.for(int i = 0, j=0; i < 1000 || j < 1000 ; i+=3,j+=5). While technically possible, I don't see the benefit of doing so. It dramatically lowers readability, and the # of operations is pretty much exactly the same regardless of merging theforloops or keeping them separate. If anything, you're going to end up incrementingj333 times (becauseiwill be incremented 333 times) while you only need to increment it 200 times, but since theforis still working withi, it also keeps incrementingj.
– Flater
1 hour ago
 |Â
show 2 more comments
up vote
2
down vote
The main problem
The main issue here is that you define i in function of it being a "factor of 3", and then also try to use this number as a "factor of 5". This doesn't make any sense - as there is no inherent relationship between being a threefold and a fivefold number.
It would've made sense if you were doing it for 3 and 6, for example, because 3 and 6 are related:
//i*3 is obviously always a multiple of 3
sum += 3 * i;
if ((3*i) % 2 == 0)
//Now we know that i*3 is also a multiple of 6
But that is not the case here.
The for loop's readability
I understand your idea, you wanted to only iterate over factors of three which keep the multiple under 1000. While I will suggest to change this approach (later in the answer), your approach could've been written in a much more readable manner:
for( int i = 0 ; i < 1000 ; i+=3 )
This will iterate over i values of 0,3,6,9,12,15,... and will skip all values inbetween.
The benefit here is that you don't need to work with i*3 all the time, you can just use i itself.
You will need to iterate over the multiple of 5 separately. However, should you keep using this approach, I would always suggest splitting these loops anyway.
The algorithm
Your approach works, but it's not the easiest approach. If I put your approach into words:
Add every multiple of 3 to the sum.
Also add the multiple of 5, but only if it's still below 1000, and it's not already divisble by 3.
The issue here is in how you handle the multiples of five. You're working with an i that is defined as the allowed values for threefolds. For any i > 200, you're effectively having to manually exclude this value. You're using a different approach for the 5 than you are using for the 3, even though the logic is exactly the same. That's not a reusable approach.
Secondly, there is a readability problem. Your code should be trivially readable, and I simply wasn't able to understand your intention. I had to google what the question was before I could understand what your code was trying to achieve.
So let me offer a better approach, first putting it into words:
- Check every number from 0 to 1000 (not including 1000)
If it is divisible by 3 or it is divisible by 5, then add it to the sum.
This can be put into code, step by step:
// Check every number from 0 to 1000 (not including 1000)
for(int i = 0; i < 1000; i++)
isDivisibleBy5)
//then add it to the sum
sum += i;
Note how the code exactly mirrors my algorithm description.
You don't need to use the booleans. I simply added them to simplify the example. if(i % 3 == 0 || i % 5 == 0) would be equally okay to use because it's still reasonably readable.
If the calculations become more complex, I suggest always using the booleans so you neatly break your algorithm down to small and manageable steps. It will do wonders for your code readability, and it does not impact performance (the compiler will optimize this in a release build).
A LINQ variation
This can be further shortened using LINQ:
var sum = Enumerable.Range(0,1000)
.Where(i => i % 3 == 0 || i % 5 == 0)
.Sum();
LINQ is just a nicer syntaxt, but it uses a for/foreach iteration in the background, so I suspect it won't be much more performant than the previous example. But I do consider this highly readable.
Maximizing performance
The previous suggestion maximizes readability, but it does so at the cost of performance, as it now has to loop over 1000 values and evaluate them all. You already linked several other answer that clearly dive deeper into the code in order to maximize the performance, I hope you can see that this dramatically impacts the readability.
For example:
public void Solve()
result = SumDivisbleBy(3,999)+SumDivisbleBy(5,999)-SumDivisbleBy(15,999);
private int SumDivisbleBy(int n, int p)
return n*(p/n)*((p/n)+1)/2;
By itself, I would have no idea what this code does. I can sort of understand the intention of Solve(), but it's not quite apparent how SumDivisbleBy() works.
SumDivisbleBy() has effectively become impossible to maintain. If you needed to introduce a change, you would effectively have to reverse engineer it before you can alter it. This means that starting from scratch is the better option, which is clearly not a good thing.
However, when performance is the main focus, this is acceptable. I would, however, strongly urge you to document the algorithm in comments specifically to help future readers in understanding how/why this works.
Note that AlanT's answer contains a more readable variant of the SumDivisbleBy() method, which already helps a lot with understanding the algorithm. The clear naming used clarifies what the algorithm does, which is the main goal of writing readable code (explaining why something works is only a secondary goal and not always required).
answered 1 hour ago
Flater
2,800819
How about using two counters, one withi+=3and the other one withj+=5? Wouldn't it be even more efficient without the%?
– t3chb0t
1 hour ago
@t3chb0t: Sure but then you have to iterate twice. I haven't crunched the numbers on it but it seems to be a "pick your poison" type deal. Either iterate twice, or iterate once and validate the numbers. Besides, if you're going for performance, you're waaaay better off with theSumDivisbleByexample I linked, as this only requires one computation per divisor (3 computations) in total) and no iteration at all.
– Flater
1 hour ago
@t3chb0t: A propos, if you use thei+=3approach, you will always have to have a secondi+=5loop. OP's code relies on the "factors of three" (from 0 to 333) inherently including all "factors of five" (from 0 to 200). Using thei +=3approach,iis no longer a list of "factors of three" but rather "multiples of three", which no longer inherently include all "multiples of five"; thus requiring you to iterate those separately.
– Flater
1 hour ago
Not exactly... I was thinking about a singleforloop with two variables...
– t3chb0t
1 hour ago
@t3chb0t: I don't see how that would work unless you then use a combined condition, i.e.for(int i = 0, j=0; i < 1000 || j < 1000 ; i+=3,j+=5). While technically possible, I don't see the benefit of doing so. It dramatically lowers readability, and the # of operations is pretty much exactly the same regardless of merging theforloops or keeping them separate. If anything, you're going to end up incrementingj333 times (becauseiwill be incremented 333 times) while you only need to increment it 200 times, but since theforis still working withi, it also keeps incrementingj.
– Flater
1 hour ago
 |Â
show 2 more comments
up vote
2
down vote
up vote
2
down vote
The main problem
The main issue here is that you define i in function of it being a "factor of 3", and then also try to use this number as a "factor of 5". This doesn't make any sense - as there is no inherent relationship between being a threefold and a fivefold number.
It would've made sense if you were doing it for 3 and 6, for example, because 3 and 6 are related:
//i*3 is obviously always a multiple of 3
sum += 3 * i;
if ((3*i) % 2 == 0)
//Now we know that i*3 is also a multiple of 6
But that is not the case here.
The for loop's readability
I understand your idea, you wanted to only iterate over factors of three which keep the multiple under 1000. While I will suggest to change this approach (later in the answer), your approach could've been written in a much more readable manner:
for( int i = 0 ; i < 1000 ; i+=3 )
This will iterate over i values of 0,3,6,9,12,15,... and will skip all values inbetween.
The benefit here is that you don't need to work with i*3 all the time, you can just use i itself.
You will need to iterate over the multiple of 5 separately. However, should you keep using this approach, I would always suggest splitting these loops anyway.
The algorithm
Your approach works, but it's not the easiest approach. If I put your approach into words:
Add every multiple of 3 to the sum.
Also add the multiple of 5, but only if it's still below 1000, and it's not already divisble by 3.
The issue here is in how you handle the multiples of five. You're working with an i that is defined as the allowed values for threefolds. For any i > 200, you're effectively having to manually exclude this value. You're using a different approach for the 5 than you are using for the 3, even though the logic is exactly the same. That's not a reusable approach.
Secondly, there is a readability problem. Your code should be trivially readable, and I simply wasn't able to understand your intention. I had to google what the question was before I could understand what your code was trying to achieve.
So let me offer a better approach, first putting it into words:
- Check every number from 0 to 1000 (not including 1000)
If it is divisible by 3 or it is divisible by 5, then add it to the sum.
This can be put into code, step by step:
// Check every number from 0 to 1000 (not including 1000)
for(int i = 0; i < 1000; i++)
isDivisibleBy5)
//then add it to the sum
sum += i;
Note how the code exactly mirrors my algorithm description.
You don't need to use the booleans. I simply added them to simplify the example. if(i % 3 == 0 || i % 5 == 0) would be equally okay to use because it's still reasonably readable.
If the calculations become more complex, I suggest always using the booleans so you neatly break your algorithm down to small and manageable steps. It will do wonders for your code readability, and it does not impact performance (the compiler will optimize this in a release build).
A LINQ variation
This can be further shortened using LINQ:
var sum = Enumerable.Range(0,1000)
.Where(i => i % 3 == 0 || i % 5 == 0)
.Sum();
LINQ is just a nicer syntaxt, but it uses a for/foreach iteration in the background, so I suspect it won't be much more performant than the previous example. But I do consider this highly readable.
Maximizing performance
The previous suggestion maximizes readability, but it does so at the cost of performance, as it now has to loop over 1000 values and evaluate them all. You already linked several other answer that clearly dive deeper into the code in order to maximize the performance, I hope you can see that this dramatically impacts the readability.
For example:
public void Solve()
result = SumDivisbleBy(3,999)+SumDivisbleBy(5,999)-SumDivisbleBy(15,999);
private int SumDivisbleBy(int n, int p)
return n*(p/n)*((p/n)+1)/2;
By itself, I would have no idea what this code does. I can sort of understand the intention of Solve(), but it's not quite apparent how SumDivisbleBy() works.
SumDivisbleBy() has effectively become impossible to maintain. If you needed to introduce a change, you would effectively have to reverse engineer it before you can alter it. This means that starting from scratch is the better option, which is clearly not a good thing.
However, when performance is the main focus, this is acceptable. I would, however, strongly urge you to document the algorithm in comments specifically to help future readers in understanding how/why this works.
Note that AlanT's answer contains a more readable variant of the SumDivisbleBy() method, which already helps a lot with understanding the algorithm. The clear naming used clarifies what the algorithm does, which is the main goal of writing readable code (explaining why something works is only a secondary goal and not always required).
answered 1 hour ago
Flater
2,800819
The main problem
The main issue here is that you define i in function of it being a "factor of 3", and then also try to use this number as a "factor of 5". This doesn't make any sense - as there is no inherent relationship between being a threefold and a fivefold number.
It would've made sense if you were doing it for 3 and 6, for example, because 3 and 6 are related:
//i*3 is obviously always a multiple of 3
sum += 3 * i;
if ((3*i) % 2 == 0)
//Now we know that i*3 is also a multiple of 6
But that is not the case here.
The for loop's readability
I understand your idea, you wanted to only iterate over factors of three which keep the multiple under 1000. While I will suggest to change this approach (later in the answer), your approach could've been written in a much more readable manner:
for( int i = 0 ; i < 1000 ; i+=3 )
This will iterate over i values of 0,3,6,9,12,15,... and will skip all values inbetween.
The benefit here is that you don't need to work with i*3 all the time, you can just use i itself.
You will need to iterate over the multiple of 5 separately. However, should you keep using this approach, I would always suggest splitting these loops anyway.
The algorithm
Your approach works, but it's not the easiest approach. If I put your approach into words:
Add every multiple of 3 to the sum.
Also add the multiple of 5, but only if it's still below 1000, and it's not already divisble by 3.
The issue here is in how you handle the multiples of five. You're working with an i that is defined as the allowed values for threefolds. For any i > 200, you're effectively having to manually exclude this value. You're using a different approach for the 5 than you are using for the 3, even though the logic is exactly the same. That's not a reusable approach.
Secondly, there is a readability problem. Your code should be trivially readable, and I simply wasn't able to understand your intention. I had to google what the question was before I could understand what your code was trying to achieve.
So let me offer a better approach, first putting it into words:
- Check every number from 0 to 1000 (not including 1000)
If it is divisible by 3 or it is divisible by 5, then add it to the sum.
This can be put into code, step by step:
// Check every number from 0 to 1000 (not including 1000)
for(int i = 0; i < 1000; i++)
isDivisibleBy5)
//then add it to the sum
sum += i;
Note how the code exactly mirrors my algorithm description.
You don't need to use the booleans. I simply added them to simplify the example. if(i % 3 == 0 || i % 5 == 0) would be equally okay to use because it's still reasonably readable.
If the calculations become more complex, I suggest always using the booleans so you neatly break your algorithm down to small and manageable steps. It will do wonders for your code readability, and it does not impact performance (the compiler will optimize this in a release build).
A LINQ variation
This can be further shortened using LINQ:
var sum = Enumerable.Range(0,1000)
.Where(i => i % 3 == 0 || i % 5 == 0)
.Sum();
LINQ is just a nicer syntaxt, but it uses a for/foreach iteration in the background, so I suspect it won't be much more performant than the previous example. But I do consider this highly readable.
Maximizing performance
The previous suggestion maximizes readability, but it does so at the cost of performance, as it now has to loop over 1000 values and evaluate them all. You already linked several other answer that clearly dive deeper into the code in order to maximize the performance, I hope you can see that this dramatically impacts the readability.
For example:
public void Solve()
result = SumDivisbleBy(3,999)+SumDivisbleBy(5,999)-SumDivisbleBy(15,999);
private int SumDivisbleBy(int n, int p)
return n*(p/n)*((p/n)+1)/2;
By itself, I would have no idea what this code does. I can sort of understand the intention of Solve(), but it's not quite apparent how SumDivisbleBy() works.
SumDivisbleBy() has effectively become impossible to maintain. If you needed to introduce a change, you would effectively have to reverse engineer it before you can alter it. This means that starting from scratch is the better option, which is clearly not a good thing.
However, when performance is the main focus, this is acceptable. I would, however, strongly urge you to document the algorithm in comments specifically to help future readers in understanding how/why this works.
Note that AlanT's answer contains a more readable variant of the SumDivisbleBy() method, which already helps a lot with understanding the algorithm. The clear naming used clarifies what the algorithm does, which is the main goal of writing readable code (explaining why something works is only a secondary goal and not always required).
answered 1 hour ago
Flater
2,800819
edited 1 hour ago
answered 1 hour ago
Flater
2,800819
answered 1 hour ago
Flater
2,800819
answered 1 hour ago
Flater
2,800819
2,800819
How about using two counters, one withi+=3and the other one withj+=5? Wouldn't it be even more efficient without the%?
– t3chb0t
1 hour ago
@t3chb0t: Sure but then you have to iterate twice. I haven't crunched the numbers on it but it seems to be a "pick your poison" type deal. Either iterate twice, or iterate once and validate the numbers. Besides, if you're going for performance, you're waaaay better off with theSumDivisbleByexample I linked, as this only requires one computation per divisor (3 computations) in total) and no iteration at all.
– Flater
1 hour ago
@t3chb0t: A propos, if you use thei+=3approach, you will always have to have a secondi+=5loop. OP's code relies on the "factors of three" (from 0 to 333) inherently including all "factors of five" (from 0 to 200). Using thei +=3approach,iis no longer a list of "factors of three" but rather "multiples of three", which no longer inherently include all "multiples of five"; thus requiring you to iterate those separately.
– Flater
1 hour ago
Not exactly... I was thinking about a singleforloop with two variables...
– t3chb0t
1 hour ago
@t3chb0t: I don't see how that would work unless you then use a combined condition, i.e.for(int i = 0, j=0; i < 1000 || j < 1000 ; i+=3,j+=5). While technically possible, I don't see the benefit of doing so. It dramatically lowers readability, and the # of operations is pretty much exactly the same regardless of merging theforloops or keeping them separate. If anything, you're going to end up incrementingj333 times (becauseiwill be incremented 333 times) while you only need to increment it 200 times, but since theforis still working withi, it also keeps incrementingj.
– Flater
1 hour ago
 |Â
show 2 more comments
How about using two counters, one withi+=3and the other one withj+=5? Wouldn't it be even more efficient without the%?
– t3chb0t
1 hour ago
@t3chb0t: Sure but then you have to iterate twice. I haven't crunched the numbers on it but it seems to be a "pick your poison" type deal. Either iterate twice, or iterate once and validate the numbers. Besides, if you're going for performance, you're waaaay better off with theSumDivisbleByexample I linked, as this only requires one computation per divisor (3 computations) in total) and no iteration at all.
– Flater
1 hour ago
@t3chb0t: A propos, if you use thei+=3approach, you will always have to have a secondi+=5loop. OP's code relies on the "factors of three" (from 0 to 333) inherently including all "factors of five" (from 0 to 200). Using thei +=3approach,iis no longer a list of "factors of three" but rather "multiples of three", which no longer inherently include all "multiples of five"; thus requiring you to iterate those separately.
– Flater
1 hour ago
Not exactly... I was thinking about a singleforloop with two variables...
– t3chb0t
1 hour ago
@t3chb0t: I don't see how that would work unless you then use a combined condition, i.e.for(int i = 0, j=0; i < 1000 || j < 1000 ; i+=3,j+=5). While technically possible, I don't see the benefit of doing so. It dramatically lowers readability, and the # of operations is pretty much exactly the same regardless of merging theforloops or keeping them separate. If anything, you're going to end up incrementingj333 times (becauseiwill be incremented 333 times) while you only need to increment it 200 times, but since theforis still working withi, it also keeps incrementingj.
– Flater
1 hour ago
How about using two counters, one with
i+=3 and the other one with j+=5? Wouldn't it be even more efficient without the %?– t3chb0t
1 hour ago
How about using two counters, one with
i+=3 and the other one with j+=5? Wouldn't it be even more efficient without the %?– t3chb0t
1 hour ago
@t3chb0t: Sure but then you have to iterate twice. I haven't crunched the numbers on it but it seems to be a "pick your poison" type deal. Either iterate twice, or iterate once and validate the numbers. Besides, if you're going for performance, you're waaaay better off with the
SumDivisbleBy example I linked, as this only requires one computation per divisor (3 computations) in total) and no iteration at all.– Flater
1 hour ago
@t3chb0t: Sure but then you have to iterate twice. I haven't crunched the numbers on it but it seems to be a "pick your poison" type deal. Either iterate twice, or iterate once and validate the numbers. Besides, if you're going for performance, you're waaaay better off with the
SumDivisbleBy example I linked, as this only requires one computation per divisor (3 computations) in total) and no iteration at all.– Flater
1 hour ago
@t3chb0t: A propos, if you use the
i+=3 approach, you will always have to have a second i+=5 loop. OP's code relies on the "factors of three" (from 0 to 333) inherently including all "factors of five" (from 0 to 200). Using the i +=3 approach, i is no longer a list of "factors of three" but rather "multiples of three", which no longer inherently include all "multiples of five"; thus requiring you to iterate those separately.– Flater
1 hour ago
@t3chb0t: A propos, if you use the
i+=3 approach, you will always have to have a second i+=5 loop. OP's code relies on the "factors of three" (from 0 to 333) inherently including all "factors of five" (from 0 to 200). Using the i +=3 approach, i is no longer a list of "factors of three" but rather "multiples of three", which no longer inherently include all "multiples of five"; thus requiring you to iterate those separately.– Flater
1 hour ago
Not exactly... I was thinking about a single
for loop with two variables...– t3chb0t
1 hour ago
Not exactly... I was thinking about a single
for loop with two variables...– t3chb0t
1 hour ago
@t3chb0t: I don't see how that would work unless you then use a combined condition, i.e.
for(int i = 0, j=0; i < 1000 || j < 1000 ; i+=3,j+=5). While technically possible, I don't see the benefit of doing so. It dramatically lowers readability, and the # of operations is pretty much exactly the same regardless of merging the for loops or keeping them separate. If anything, you're going to end up incrementing j 333 times (because i will be incremented 333 times) while you only need to increment it 200 times, but since the for is still working with i, it also keeps incrementing j.– Flater
1 hour ago
@t3chb0t: I don't see how that would work unless you then use a combined condition, i.e.
for(int i = 0, j=0; i < 1000 || j < 1000 ; i+=3,j+=5). While technically possible, I don't see the benefit of doing so. It dramatically lowers readability, and the # of operations is pretty much exactly the same regardless of merging the for loops or keeping them separate. If anything, you're going to end up incrementing j 333 times (because i will be incremented 333 times) while you only need to increment it 200 times, but since the for is still working with i, it also keeps incrementing j.– Flater
1 hour ago
 |Â
show 2 more comments
up vote
0
down vote
Thanks @Henrik Hansen it helped.
This is optimised code below according to the WIKI.
But here is the tradeoff, this has to loop 1000 times. where above solution need to run at most 333 times.
using System;
public class Program
public static void Main()
int sum = 0;
for (int i = 0; i < 1000; i++)
Console.WriteLine(sum );
answered 2 hours ago
Grv10India
63
New contributor
Grv10India is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
Yes, but you should read a bit further -:)
– Henrik Hansen
2 hours ago
add a comment |Â
up vote
0
down vote
Thanks @Henrik Hansen it helped.
This is optimised code below according to the WIKI.
But here is the tradeoff, this has to loop 1000 times. where above solution need to run at most 333 times.
using System;
public class Program
public static void Main()
int sum = 0;
for (int i = 0; i < 1000; i++)
Console.WriteLine(sum );
answered 2 hours ago
Grv10India
63
New contributor
Grv10India is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
Yes, but you should read a bit further -:)
– Henrik Hansen
2 hours ago
add a comment |Â
up vote
0
down vote
up vote
0
down vote
Thanks @Henrik Hansen it helped.
This is optimised code below according to the WIKI.
But here is the tradeoff, this has to loop 1000 times. where above solution need to run at most 333 times.
using System;
public class Program
public static void Main()
int sum = 0;
for (int i = 0; i < 1000; i++)
Console.WriteLine(sum );
answered 2 hours ago
Grv10India
63
New contributor
Grv10India is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
Thanks @Henrik Hansen it helped.
This is optimised code below according to the WIKI.
But here is the tradeoff, this has to loop 1000 times. where above solution need to run at most 333 times.
using System;
public class Program
public static void Main()
int sum = 0;
for (int i = 0; i < 1000; i++)
Console.WriteLine(sum );
answered 2 hours ago
Grv10India
63
New contributor
Grv10India is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
answered 2 hours ago
Grv10India
63
New contributor
Grv10India is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
answered 2 hours ago
Grv10India
63
answered 2 hours ago
Grv10India
63
63
New contributor
Grv10India is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
New contributor
Grv10India is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
Grv10India is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
Yes, but you should read a bit further -:)
– Henrik Hansen
2 hours ago
add a comment |Â
Yes, but you should read a bit further -:)
– Henrik Hansen
2 hours ago
Yes, but you should read a bit further -:)
– Henrik Hansen
2 hours ago
Yes, but you should read a bit further -:)
– Henrik Hansen
2 hours ago
add a comment |Â
up vote
0
down vote
I'm afraid that the solution given is not better than the referenced C one
(although the C solution could be generalized and made a bit easier to understand)
Why?
The code under review has a outer loop that is executed 333 times (with a multiplication in each condition check). We can remove the multiplication to do the job a bit more cheaply
for (int i = 1; i <= 333; i++)
But, even with the restructured loop we still do
this, sum += 3 * i;, 333 times and then
this, 5 * i < 1000 && (5 * i) % 3 != 0, 333 times
As opposed to
S1= a*(b+c)/2;, once
s2= k*(i+j)/2;, once
s3= n*(m+o)/2;, once
S= S1 + s2 - s3;, once
It is a bit of a cheat in the C solution that the various magic numbers are hardcoded; but even allowing for a function to calculate the values
var sum3 = SumDivisibleBy(999,3);
var sum5 = SumDivisibleBy(999,5);
var sum15 = SumDivisibleBy(999,15);
var sum35 = sum3 + sum5 - sum15;
private static int SumDivisibleBy(int range, int divisor)
var top = range/divisor;
var ret = divisor*top*(top+1)/2;
return ret;
is a lot cheaper than brute-forcing the solution.
In the original solution, for numbers under 1000, we need to loop 333 times, for numbers under 10000, we need to loop 3333 times,... It doesn't scale well. For the other solution, there are the same number of operations each time.
answered 2 hours ago
AlanT
2,8641015
add a comment |Â
up vote
0
down vote
I'm afraid that the solution given is not better than the referenced C one
(although the C solution could be generalized and made a bit easier to understand)
Why?
The code under review has a outer loop that is executed 333 times (with a multiplication in each condition check). We can remove the multiplication to do the job a bit more cheaply
for (int i = 1; i <= 333; i++)
But, even with the restructured loop we still do
this, sum += 3 * i;, 333 times and then
this, 5 * i < 1000 && (5 * i) % 3 != 0, 333 times
As opposed to
S1= a*(b+c)/2;, once
s2= k*(i+j)/2;, once
s3= n*(m+o)/2;, once
S= S1 + s2 - s3;, once
It is a bit of a cheat in the C solution that the various magic numbers are hardcoded; but even allowing for a function to calculate the values
var sum3 = SumDivisibleBy(999,3);
var sum5 = SumDivisibleBy(999,5);
var sum15 = SumDivisibleBy(999,15);
var sum35 = sum3 + sum5 - sum15;
private static int SumDivisibleBy(int range, int divisor)
var top = range/divisor;
var ret = divisor*top*(top+1)/2;
return ret;
is a lot cheaper than brute-forcing the solution.
In the original solution, for numbers under 1000, we need to loop 333 times, for numbers under 10000, we need to loop 3333 times,... It doesn't scale well. For the other solution, there are the same number of operations each time.
answered 2 hours ago
AlanT
2,8641015
add a comment |Â
up vote
0
down vote
up vote
0
down vote
I'm afraid that the solution given is not better than the referenced C one
(although the C solution could be generalized and made a bit easier to understand)
Why?
The code under review has a outer loop that is executed 333 times (with a multiplication in each condition check). We can remove the multiplication to do the job a bit more cheaply
for (int i = 1; i <= 333; i++)
But, even with the restructured loop we still do
this, sum += 3 * i;, 333 times and then
this, 5 * i < 1000 && (5 * i) % 3 != 0, 333 times
As opposed to
S1= a*(b+c)/2;, once
s2= k*(i+j)/2;, once
s3= n*(m+o)/2;, once
S= S1 + s2 - s3;, once
It is a bit of a cheat in the C solution that the various magic numbers are hardcoded; but even allowing for a function to calculate the values
var sum3 = SumDivisibleBy(999,3);
var sum5 = SumDivisibleBy(999,5);
var sum15 = SumDivisibleBy(999,15);
var sum35 = sum3 + sum5 - sum15;
private static int SumDivisibleBy(int range, int divisor)
var top = range/divisor;
var ret = divisor*top*(top+1)/2;
return ret;
is a lot cheaper than brute-forcing the solution.
In the original solution, for numbers under 1000, we need to loop 333 times, for numbers under 10000, we need to loop 3333 times,... It doesn't scale well. For the other solution, there are the same number of operations each time.
answered 2 hours ago
AlanT
2,8641015
I'm afraid that the solution given is not better than the referenced C one
(although the C solution could be generalized and made a bit easier to understand)
Why?
The code under review has a outer loop that is executed 333 times (with a multiplication in each condition check). We can remove the multiplication to do the job a bit more cheaply
for (int i = 1; i <= 333; i++)
But, even with the restructured loop we still do
this, sum += 3 * i;, 333 times and then
this, 5 * i < 1000 && (5 * i) % 3 != 0, 333 times
As opposed to
S1= a*(b+c)/2;, once
s2= k*(i+j)/2;, once
s3= n*(m+o)/2;, once
S= S1 + s2 - s3;, once
It is a bit of a cheat in the C solution that the various magic numbers are hardcoded; but even allowing for a function to calculate the values
var sum3 = SumDivisibleBy(999,3);
var sum5 = SumDivisibleBy(999,5);
var sum15 = SumDivisibleBy(999,15);
var sum35 = sum3 + sum5 - sum15;
private static int SumDivisibleBy(int range, int divisor)
var top = range/divisor;
var ret = divisor*top*(top+1)/2;
return ret;
is a lot cheaper than brute-forcing the solution.
In the original solution, for numbers under 1000, we need to loop 333 times, for numbers under 10000, we need to loop 3333 times,... It doesn't scale well. For the other solution, there are the same number of operations each time.
answered 2 hours ago
AlanT
2,8641015
answered 2 hours ago
AlanT
2,8641015
answered 2 hours ago
AlanT
2,8641015
answered 2 hours ago
AlanT
2,8641015
2,8641015
add a comment |Â
add a comment |Â
up vote
0
down vote
In order to make the algorithm more useful, you could wrap it in a function with 3 arguments:
static int SumFactors(int m, int n, int max)
int sum = 0;
for (int i = 0; m * i < max; i++)
sum += m * i;
if (n * i < max && (n * i) % m != 0)
sum += n * i;
return sum;
Although your algorithm is straight forward and intuitive as a brute force algorithm, it is also the most inefficient way to do the trick.
You can find inspiration to an efficient solution here
answered 2 hours ago
Henrik Hansen
5,0081619
I'm afraid this qualifies as a link-only answer and is off-topic :-( you don't mind if I downvote you this one time? ;-P
– t3chb0t
2 hours ago
@t3chb0t: Well, it should maybe have been a comment rather than an answer. I thought the wiki-link do the job for an answer and that I couldn't do it any better.
– Henrik Hansen
1 hour ago
add a comment |Â
up vote
0
down vote
In order to make the algorithm more useful, you could wrap it in a function with 3 arguments:
static int SumFactors(int m, int n, int max)
int sum = 0;
for (int i = 0; m * i < max; i++)
sum += m * i;
if (n * i < max && (n * i) % m != 0)
sum += n * i;
return sum;
Although your algorithm is straight forward and intuitive as a brute force algorithm, it is also the most inefficient way to do the trick.
You can find inspiration to an efficient solution here
answered 2 hours ago
Henrik Hansen
5,0081619
I'm afraid this qualifies as a link-only answer and is off-topic :-( you don't mind if I downvote you this one time? ;-P
– t3chb0t
2 hours ago
@t3chb0t: Well, it should maybe have been a comment rather than an answer. I thought the wiki-link do the job for an answer and that I couldn't do it any better.
– Henrik Hansen
1 hour ago
add a comment |Â
up vote
0
down vote
up vote
0
down vote
In order to make the algorithm more useful, you could wrap it in a function with 3 arguments:
static int SumFactors(int m, int n, int max)
int sum = 0;
for (int i = 0; m * i < max; i++)
sum += m * i;
if (n * i < max && (n * i) % m != 0)
sum += n * i;
return sum;
Although your algorithm is straight forward and intuitive as a brute force algorithm, it is also the most inefficient way to do the trick.
You can find inspiration to an efficient solution here
answered 2 hours ago
Henrik Hansen
5,0081619
In order to make the algorithm more useful, you could wrap it in a function with 3 arguments:
static int SumFactors(int m, int n, int max)
int sum = 0;
for (int i = 0; m * i < max; i++)
sum += m * i;
if (n * i < max && (n * i) % m != 0)
sum += n * i;
return sum;
Although your algorithm is straight forward and intuitive as a brute force algorithm, it is also the most inefficient way to do the trick.
You can find inspiration to an efficient solution here
answered 2 hours ago
Henrik Hansen
5,0081619
edited 1 hour ago
answered 2 hours ago
Henrik Hansen
5,0081619
answered 2 hours ago
Henrik Hansen
5,0081619
answered 2 hours ago
Henrik Hansen
5,0081619
5,0081619
I'm afraid this qualifies as a link-only answer and is off-topic :-( you don't mind if I downvote you this one time? ;-P
– t3chb0t
2 hours ago
@t3chb0t: Well, it should maybe have been a comment rather than an answer. I thought the wiki-link do the job for an answer and that I couldn't do it any better.
– Henrik Hansen
1 hour ago
add a comment |Â
I'm afraid this qualifies as a link-only answer and is off-topic :-( you don't mind if I downvote you this one time? ;-P
– t3chb0t
2 hours ago
@t3chb0t: Well, it should maybe have been a comment rather than an answer. I thought the wiki-link do the job for an answer and that I couldn't do it any better.
– Henrik Hansen
1 hour ago
I'm afraid this qualifies as a link-only answer and is off-topic :-( you don't mind if I downvote you this one time? ;-P
– t3chb0t
2 hours ago
I'm afraid this qualifies as a link-only answer and is off-topic :-( you don't mind if I downvote you this one time? ;-P
– t3chb0t
2 hours ago
@t3chb0t: Well, it should maybe have been a comment rather than an answer. I thought the wiki-link do the job for an answer and that I couldn't do it any better.
– Henrik Hansen
1 hour ago
@t3chb0t: Well, it should maybe have been a comment rather than an answer. I thought the wiki-link do the job for an answer and that I couldn't do it any better.
– Henrik Hansen
1 hour ago
add a comment |Â
Grv10India is a new contributor. Be nice, and check out our Code of Conduct.
Grv10India is a new contributor. Be nice, and check out our Code of Conduct.
Grv10India is a new contributor. Be nice, and check out our Code of Conduct.
Grv10India is a new contributor. Be nice, and check out our Code of Conduct.
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f205578%2fadd-multiples-of-3-or-5-below-1000-can-this-code-be-optimised-project-euler-1%23new-answer', 'question_page');
);
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
1
Console.WriteLine(sum + "");looks weird, why don't you justConsole.WriteLine(sum);?– nalka
2 hours ago