0
\$\begingroup\$

I've wrote this code to calculate the prime numbers between two integers n and m, and I want to optimise it.

#include<iostream>
#include <cmath>
using namespace std;
int main (){
long int T[100][2],n,m,i,j;
int testcase,k;
bool prem;
cin>>testcase;
for(i=0;i<testcase;i++){
   cin>>T[i][0]>>T[i][1];}
for(k=0;k<testcase;k++){
    m=T[k][0];
    n=T[k][1];
for(i=m;i<=n;i++){
    prem=true;
for(j=2;j<i;j++){
    if(i%j==0){prem=false;}}
    if(prem) {cout<<i<<endl;}}}
return 0;}
\$\endgroup\$
  • 7
    \$\begingroup\$ Wow, that formatting and naming is ... a thing. Are you sure you couldn't have removed some more newlines and the rest of the indentation, as well as eliminating all multi-character names the language doesn't actually force on you? \$\endgroup\$ – Deduplicator Nov 16 '15 at 3:25
  • \$\begingroup\$ Yeah. Consider editing your question to use proper indentation, since otherwise you're just scaring away potential answerers. Why would anyone engage with the non-trivial "optimization" part of your question when they have to mentally decompile your code just to see what you're doing? \$\endgroup\$ – Quuxplusone Nov 16 '15 at 5:54
  • 1
    \$\begingroup\$ No point in reviewing that. If you presented that at any company I have worked for you would be looking for a new job very shortly. Even if you (and your code) are brilliant, you still have to work with other people, this code is not something other people want to read! \$\endgroup\$ – Martin York Nov 16 '15 at 8:20
  • \$\begingroup\$ I think this ought to be pinned as an example of how not to present code. I don't meant to be horrible, most of us have written stuff like that in the beginning and in ten years time you will know why we are all going FFS get this off my screen. @Taha - don't take offence from what people are saying, listen to the advice and try again and the response will be better. Either that or go for a program on one line competition :) \$\endgroup\$ – Code Gorilla Nov 16 '15 at 17:25
1
\$\begingroup\$
  1. Use better variable names. Or, alternatively, include comments to show what the variables are for.
  2. Reformat the code with better indentation.
  3. Do not use using namespace std. That is just bad practice.
  4. Instead of looping j from 2 to i, you can just loop upto floor(sqrt(i) + 1).
  5. Do not define i, j and k for such a large scope. Keep their scopes minimal.
  6. Since your inputs would always be positive integers, use unsigned.
  7. This is just a personal convention, and you're not bound to it. I like to define variables in the order bool, char, int, long int, float, double, ... so on.

# include <iostream>
# include <cmath>

# define ULI unsigned long int

int main () {
    bool is_prime;
    int num_cases;
    ULI cases[100][2];
    std::cin>>num_cases;

    for(int i = 0; i < num_cases; i++)
       std::cin >> cases[i][0] >> cases[i][1];
    for(int k = 0; k < num_cases; k++) {
        ULI m = cases[k][0],
            n = cases[k][1];
        for(ULI i = m; i <= n; i++) {
            is_prime = true;
            ULI max = (ULI) sqrt(i) + 1;
            for(ULI j = 2; j < i; j++)
                if(i % j == 0)
                    is_prime = false;
            if(is_prime)
                std::cout<<i<<std::endl;
        }
    }
    return 0;
}
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Personally I would never declare a variable without initialising it. Mainly because when someone else comes along and "improves" your code they will start reading from it before its written to. Not all compilers flag that as a fault. \$\endgroup\$ – Code Gorilla Nov 16 '15 at 17:27
  • \$\begingroup\$ @Matt: If you avoid it be deferring definition until you can usefully initialize, more power to you. If you avoid it by just initializing it to something, I really don't want to work with you, as you just ensured that no compiler can flag the missing proper initialization anymore. Some compilers not being smart enough to warn about an error is no reason to suppress the warning on the rest. \$\endgroup\$ – Deduplicator Nov 16 '15 at 20:50
  • 1
    \$\begingroup\$ @hjpotter: #1 the alternative is a very distant second best, see codereview.stackexchange.com/questions/90111/… #4 that bound should be explicitly saved in a local to avoid recomputing. As an aside, return 0; is implicit for main in C++ and C99+. \$\endgroup\$ – Deduplicator Nov 16 '15 at 20:54
  • \$\begingroup\$ @Deduplicator, delaying definition is preferable, but in some cases, like out parameters you have no choice but to assign neutral values in case the function doesn't set them to a safe value when it fails. Of course the caller shouldn't use the result of the out value if the call failed, but you have to program defensively. \$\endgroup\$ – Code Gorilla Nov 17 '15 at 3:18
  • \$\begingroup\$ @Matt: Against what are you defending by setting an out-variable before calling the possibly-failing setter? And are you sure the contract guarantees to not change it if the function fails, at least for all those you use? Tip: The stream-extractor changed from "no change" to "zero". \$\endgroup\$ – Deduplicator Nov 17 '15 at 4:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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