Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

You've mentioned that you don't want to utilize the STL, so I'll honor that. Just be aware that neglecting to use it may lessen the quality of your code, especially if you start to experience bugs.

I'll mainly address best-practices as I'm not so familiar with the algorithm myself.

  • Your lack of whitespace just makes this painful to read. If a line appears to be too long, it either needs to be shortened (preferred) or wrapped onto separate lines.

    Either way, you should especially have some whitespace between operators and operands so that it won't take others unneeded time to read and understand it. This will also benefit you whenever you're maintaining the code (fixing bugs, adding things, and changing things around).

    For instance, this line:

    if(arr[x*2]>arr[x*2+1]&&(elm>arr[x*2]||elm>arr[x*2+1])){
    

    should look like this:

     if (arr[x*2] > arr[x*2+1] && (elm > arr[x*2] || elm > arr[x*2+1])) {
    

    Isn't that easier to read? There's no need to cram everything together; just let the code breathe. You could still space out the operators within the [], but it's not quite as common.

  • Your variable names are very generic. Many of them are only single letters, and your array data member is just named arr. Without good naming, you'll just leave others guessing at what each variable does. Plus, if you look at this code years from now, you may have no idea what it all means, and so you may be tempted to throw it away and start all over.

  • Although it's entirely optional, you can add the private keyword anyway, even though classes are private by default. It may add a little more readability, especially if there is a lot of code.

  • The constructor:

    min_heap(){
     P=1;
    }
    

    can be an initializer list:

     min_heap() : P(1) {}
    

    If it ever gets lengthy, you can list the members on separate lines:

     min_heap()
     : P(1)
     , // additional ones
     , // separated by commas
     {}
    

    If pvt and arr need to be initialized, then it should be done here as well. I'm not sure why you just initialize one data member.

  • You don't need std::endl here. In most cases, a flush isn't necessary along with a newline, which is what's being done by that.

    Instead, just output "\n" for a newline:

     std::cout << "\n";
    

    This is also mentioned here here.

Overall, I'd highly recommend making this more readable first, so that it's easier to understand and improve for yourself and others. Implementing all the algorithms yourself will especially require the code to be clean and self-documenting, otherwise you're really just writing C code instead.

You've mentioned that you don't want to utilize the STL, so I'll honor that. Just be aware that neglecting to use it may lessen the quality of your code, especially if you start to experience bugs.

I'll mainly address best-practices as I'm not so familiar with the algorithm myself.

  • Your lack of whitespace just makes this painful to read. If a line appears to be too long, it either needs to be shortened (preferred) or wrapped onto separate lines.

    Either way, you should especially have some whitespace between operators and operands so that it won't take others unneeded time to read and understand it. This will also benefit you whenever you're maintaining the code (fixing bugs, adding things, and changing things around).

    For instance, this line:

    if(arr[x*2]>arr[x*2+1]&&(elm>arr[x*2]||elm>arr[x*2+1])){
    

    should look like this:

     if (arr[x*2] > arr[x*2+1] && (elm > arr[x*2] || elm > arr[x*2+1])) {
    

    Isn't that easier to read? There's no need to cram everything together; just let the code breathe. You could still space out the operators within the [], but it's not quite as common.

  • Your variable names are very generic. Many of them are only single letters, and your array data member is just named arr. Without good naming, you'll just leave others guessing at what each variable does. Plus, if you look at this code years from now, you may have no idea what it all means, and so you may be tempted to throw it away and start all over.

  • Although it's entirely optional, you can add the private keyword anyway, even though classes are private by default. It may add a little more readability, especially if there is a lot of code.

  • The constructor:

    min_heap(){
     P=1;
    }
    

    can be an initializer list:

     min_heap() : P(1) {}
    

    If it ever gets lengthy, you can list the members on separate lines:

     min_heap()
     : P(1)
     , // additional ones
     , // separated by commas
     {}
    

    If pvt and arr need to be initialized, then it should be done here as well. I'm not sure why you just initialize one data member.

  • You don't need std::endl here. In most cases, a flush isn't necessary along with a newline, which is what's being done by that.

    Instead, just output "\n" for a newline:

     std::cout << "\n";
    

    This is also mentioned here.

Overall, I'd highly recommend making this more readable first, so that it's easier to understand and improve for yourself and others. Implementing all the algorithms yourself will especially require the code to be clean and self-documenting, otherwise you're really just writing C code instead.

You've mentioned that you don't want to utilize the STL, so I'll honor that. Just be aware that neglecting to use it may lessen the quality of your code, especially if you start to experience bugs.

I'll mainly address best-practices as I'm not so familiar with the algorithm myself.

  • Your lack of whitespace just makes this painful to read. If a line appears to be too long, it either needs to be shortened (preferred) or wrapped onto separate lines.

    Either way, you should especially have some whitespace between operators and operands so that it won't take others unneeded time to read and understand it. This will also benefit you whenever you're maintaining the code (fixing bugs, adding things, and changing things around).

    For instance, this line:

    if(arr[x*2]>arr[x*2+1]&&(elm>arr[x*2]||elm>arr[x*2+1])){
    

    should look like this:

     if (arr[x*2] > arr[x*2+1] && (elm > arr[x*2] || elm > arr[x*2+1])) {
    

    Isn't that easier to read? There's no need to cram everything together; just let the code breathe. You could still space out the operators within the [], but it's not quite as common.

  • Your variable names are very generic. Many of them are only single letters, and your array data member is just named arr. Without good naming, you'll just leave others guessing at what each variable does. Plus, if you look at this code years from now, you may have no idea what it all means, and so you may be tempted to throw it away and start all over.

  • Although it's entirely optional, you can add the private keyword anyway, even though classes are private by default. It may add a little more readability, especially if there is a lot of code.

  • The constructor:

    min_heap(){
     P=1;
    }
    

    can be an initializer list:

     min_heap() : P(1) {}
    

    If it ever gets lengthy, you can list the members on separate lines:

     min_heap()
     : P(1)
     , // additional ones
     , // separated by commas
     {}
    

    If pvt and arr need to be initialized, then it should be done here as well. I'm not sure why you just initialize one data member.

  • You don't need std::endl here. In most cases, a flush isn't necessary along with a newline, which is what's being done by that.

    Instead, just output "\n" for a newline:

     std::cout << "\n";
    

    This is also mentioned here.

Overall, I'd highly recommend making this more readable first, so that it's easier to understand and improve for yourself and others. Implementing all the algorithms yourself will especially require the code to be clean and self-documenting, otherwise you're really just writing C code instead.

added 131 characters in body
Source Link
Jamal
  • 35.2k
  • 13
  • 134
  • 238

You've mentioned that you don't want to utilize the STL, so I'll honor that. Just be aware that neglecting to use it, especially storage containers such as std::vector, may lessen the quality of your code, especially if you start to experience bugs.

I'll mainly address best-practices as I'm not so familiar with the algorithm myself.

  • Your lack of whitespace just makes this painful to read. If a line appears to be too long, it either needs to be shortened (preferred) or wrapped onto separate lines.

    Either way, you should especially have some whitespace between operators and operands so that it won't take others unneeded time to read and understand it. This will also benefit you whenever you're maintaining the code (fixing bugs, adding things, and changing things around).

    For instance, this line:

    if(arr[x*2]>arr[x*2+1]&&(elm>arr[x*2]||elm>arr[x*2+1])){
    

    should look like this:

     if (arr[x*2] > arr[x*2+1] && (elm > arr[x*2] || elm > arr[x*2+1])) {
    

    Isn't that easier to read? There's no need to cram everything together; just let the code breathe. You could still space out the operators within the [], but it's not quite as common.

  • Your variable names are very generic. Many of them are only single letters, and your array data member is just named arr. Without good naming, you'll just leave others guessing at what each variable does. Plus, if you look at this code years from now, you may have no idea what it all means, and so you may be tempted to throw it away and start all over.

  • Although it's entirely optional, you can add the private keyword anyway, even though classes are private by default. It may just add a little more readability, especially if there is a lot of code.

  • The constructor:

    min_heap(){
     P=1;
    }
    

    can be an initializer list:

     min_heap() : P(1) {}
    

    If it ever gets lengthy, you can list the members on separate lines:

     min_heap()
     : P(1)
     , // additional ones
     , // separated by commas
     {}
    

    If pvt and arr need to be initialized, then it should be done here as well. I'm not sure why you just initialize one data member.

  • You don't need std::endl here:

    std::cout<<std::endl;
    

    In. In most cases, you don't need to do a flush isn't necessary along with a newline, which is what's being done by that. Instead

    Instead, just output "\n" for a newline.:

     std::cout << "\n";
    

    This is also mentioned here .

Overall, I'd highly recommend making this more readable first, so that it's easier to understand and improve for yourself and others. Implementing all the algorithms yourself will especially require the code to be clean and self-documenting, otherwise you're really just writing C code instead.

You've mentioned that you don't want to utilize the STL, so I'll honor that. Just be aware that neglecting to use it, especially storage containers such as std::vector, may lessen the quality of your code, especially if you start to experience bugs.

I'll mainly address best-practices as I'm not so familiar with the algorithm myself.

  • Your lack of whitespace just makes this painful to read. If a line appears to be too long, it either needs to be shortened (preferred) or wrapped onto separate lines.

    Either way, you should especially have some whitespace between operators and operands so that it won't take others unneeded time to read and understand it. This will also benefit you whenever you're maintaining the code (fixing bugs, adding things, and changing things around).

    For instance, this line:

    if(arr[x*2]>arr[x*2+1]&&(elm>arr[x*2]||elm>arr[x*2+1])){
    

    should look like this:

     if (arr[x*2] > arr[x*2+1] && (elm > arr[x*2] || elm > arr[x*2+1])) {
    

    Isn't that easier to read? There's no need to cram everything together; just let the code breathe.

  • Your variable names are very generic. Many of them are only single letters, and your array data member is just named arr. Without good naming, you'll just leave others guessing at what each variable does. Plus, if you look at this code years from now, you may have no idea what it all means, and so you may be tempted to throw it away and start all over.

  • Although it's entirely optional, you can add the private keyword anyway, even though classes are private by default. It may just add a little more readability.

  • The constructor:

    min_heap(){
     P=1;
    }
    

    can be an initializer list:

     min_heap() : P(1) {}
    

    If it ever gets lengthy, you can list the members on separate lines:

     min_heap()
     : P(1)
     , // additional ones
     , // separated by commas
     {}
    

    If pvt and arr need to be initialized, then it should be done here as well. I'm not sure why you just initialize one data member.

  • You don't need std::endl here:

    std::cout<<std::endl;
    

    In most cases, you don't need to do a flush along with a newline, which is what's being done by that. Instead, just output "\n" for a newline.

     std::cout << "\n";
    

Overall, I'd highly recommend making this more readable first, so that it's easier to understand and improve for yourself and others. Implementing all the algorithms yourself will especially require the code to be clean and self-documenting, otherwise you're really just writing C code instead.

You've mentioned that you don't want to utilize the STL, so I'll honor that. Just be aware that neglecting to use it may lessen the quality of your code, especially if you start to experience bugs.

I'll mainly address best-practices as I'm not so familiar with the algorithm myself.

  • Your lack of whitespace just makes this painful to read. If a line appears to be too long, it either needs to be shortened (preferred) or wrapped onto separate lines.

    Either way, you should especially have some whitespace between operators and operands so that it won't take others unneeded time to read and understand it. This will also benefit you whenever you're maintaining the code (fixing bugs, adding things, and changing things around).

    For instance, this line:

    if(arr[x*2]>arr[x*2+1]&&(elm>arr[x*2]||elm>arr[x*2+1])){
    

    should look like this:

     if (arr[x*2] > arr[x*2+1] && (elm > arr[x*2] || elm > arr[x*2+1])) {
    

    Isn't that easier to read? There's no need to cram everything together; just let the code breathe. You could still space out the operators within the [], but it's not quite as common.

  • Your variable names are very generic. Many of them are only single letters, and your array data member is just named arr. Without good naming, you'll just leave others guessing at what each variable does. Plus, if you look at this code years from now, you may have no idea what it all means, and so you may be tempted to throw it away and start all over.

  • Although it's entirely optional, you can add the private keyword anyway, even though classes are private by default. It may add a little more readability, especially if there is a lot of code.

  • The constructor:

    min_heap(){
     P=1;
    }
    

    can be an initializer list:

     min_heap() : P(1) {}
    

    If it ever gets lengthy, you can list the members on separate lines:

     min_heap()
     : P(1)
     , // additional ones
     , // separated by commas
     {}
    

    If pvt and arr need to be initialized, then it should be done here as well. I'm not sure why you just initialize one data member.

  • You don't need std::endl here. In most cases, a flush isn't necessary along with a newline, which is what's being done by that.

    Instead, just output "\n" for a newline:

     std::cout << "\n";
    

    This is also mentioned here .

Overall, I'd highly recommend making this more readable first, so that it's easier to understand and improve for yourself and others. Implementing all the algorithms yourself will especially require the code to be clean and self-documenting, otherwise you're really just writing C code instead.

added 10 characters in body
Source Link
Jamal
  • 35.2k
  • 13
  • 134
  • 238

You've mentioned that you don't want to utilize the STL, so I'll abide byhonor that. Just be sureaware that neglecting to use it, especially storage containers such as std::vector, may lessen the quality of your code, especially if you start to experience bugs.

I'll mainly address best-practices as I'm not so familiar with the algorithm myself.

  • Your lack of whitespace just makes this painful to read. If a line appears to be too long, it either needs to be shortened (preferred) or wrapped onto separate lines.

    Either way, you should especiallyespecially have some whitespace between operators and operands so that it won't take others unneeded time to read and understand it. This will also benefit you whenever you're maintaining the code (fixing bugs, adding things, and changing things around).

    For instance, this line:

    if(arr[x*2]>arr[x*2+1]&&(elm>arr[x*2]||elm>arr[x*2+1])){
    

    should look like this:

     if (arr[x*2] > arr[x*2+1] && (elm > arr[x*2] || elm > arr[x*2+1])) {
    

    Isn't that easier to read? There's no need to cram everything together; just let the code breathe.

  • Your variable names are very generic. Many of them are only single letters, and your array data member is just named arr. Without good naming, you'll just leave others guessing at what each variable does. Plus, if you look at this code years from now, you may have no idea what it all means, and so you may be tempted to throw it away and start all over.

  • Although it's entirely optional, you can add the private keyword anyway, even though classes are private by default. It may just add a little more readability.

  • The constructor:

    min_heap(){
     P=1;
    }
    

    can be an initializer list:

     min_heap() : P(1) {}
    

    If it ever gets lengthy, you can list themthe members on separate lines:

     min_heap()
     : P(1)
     , // additional ones
     , // separated by commas
     {}
    

    If pvt and arr need to be initialized, then it should be done here as well. I'm not sure why you just initialize one data member.

  • You don't need std::endl here:

     std::cout<<std::endl;
    
    std::cout<<std::endl;
    

    In most cases, you don't need to do a flush along with a newline, which is what's being done by that. Instead, just output "\n" for a newline.

     std::cout << "\n";
    

Overall, I'd highly recommend making this more readable first, so that it's easier to understand and improve for yourself and others. Implementing all the algorithms yourself will especially require the code to be clean and self-documenting, otherwise you're really just writing C code instead.

You've mentioned that you don't want to utilize the STL, so I'll abide by that. Just be sure that neglecting to use it, especially storage containers such as std::vector, may lessen the quality of your code, especially if you start to experience bugs.

I'll mainly address best-practices as I'm familiar with the algorithm myself.

  • Your lack of whitespace just makes this painful to read. If a line appears to be too long, it either needs to be shortened (preferred) or wrapped onto separate lines.

    Either way, you should especially have some whitespace between operators and operands so that it won't take others unneeded time to read and understand it. This will also benefit you whenever you're maintaining the code (fixing bugs, adding things, and changing things around).

    For instance, this line:

    if(arr[x*2]>arr[x*2+1]&&(elm>arr[x*2]||elm>arr[x*2+1])){
    

    should look like this:

     if (arr[x*2] > arr[x*2+1] && (elm > arr[x*2] || elm > arr[x*2+1])) {
    

    Isn't that easier to read? There's no need to cram everything together; just let the code breathe.

  • Your variable names are very generic. Many of them are only single letters, and your array data member is just named arr. Without good naming, you'll just leave others guessing at what each variable does. Plus, if you look at this code years from now, you may have no idea what it all means, and so you may be tempted to throw it away and start all over.

  • Although it's entirely optional, you can add the private keyword anyway, even though classes are private by default. It may just add a little more readability.

  • The constructor:

    min_heap(){
     P=1;
    }
    

    can be an initializer list:

     min_heap() : P(1) {}
    

    If it ever gets lengthy, you can list them on separate lines:

     min_heap()
     : P(1)
     , // additional ones
     , // separated by commas
     {}
    

    If pvt and arr need to be initialized, then it should be done here as well. I'm not sure why you just initialize one data member.

  • You don't need std::endl here:

     std::cout<<std::endl;
    

    In most cases, you don't need to do a flush along with a newline, which is what's being done by that. Instead, just output "\n" for a newline.

     std::cout << "\n";
    

Overall, I'd highly recommend making this more readable first, so that it's easier to understand and improve for yourself and others. Implementing all the algorithms yourself will especially require the code to be clean and self-documenting, otherwise you're really just writing C code instead.

You've mentioned that you don't want to utilize the STL, so I'll honor that. Just be aware that neglecting to use it, especially storage containers such as std::vector, may lessen the quality of your code, especially if you start to experience bugs.

I'll mainly address best-practices as I'm not so familiar with the algorithm myself.

  • Your lack of whitespace just makes this painful to read. If a line appears to be too long, it either needs to be shortened (preferred) or wrapped onto separate lines.

    Either way, you should especially have some whitespace between operators and operands so that it won't take others unneeded time to read and understand it. This will also benefit you whenever you're maintaining the code (fixing bugs, adding things, and changing things around).

    For instance, this line:

    if(arr[x*2]>arr[x*2+1]&&(elm>arr[x*2]||elm>arr[x*2+1])){
    

    should look like this:

     if (arr[x*2] > arr[x*2+1] && (elm > arr[x*2] || elm > arr[x*2+1])) {
    

    Isn't that easier to read? There's no need to cram everything together; just let the code breathe.

  • Your variable names are very generic. Many of them are only single letters, and your array data member is just named arr. Without good naming, you'll just leave others guessing at what each variable does. Plus, if you look at this code years from now, you may have no idea what it all means, and so you may be tempted to throw it away and start all over.

  • Although it's entirely optional, you can add the private keyword anyway, even though classes are private by default. It may just add a little more readability.

  • The constructor:

    min_heap(){
     P=1;
    }
    

    can be an initializer list:

     min_heap() : P(1) {}
    

    If it ever gets lengthy, you can list the members on separate lines:

     min_heap()
     : P(1)
     , // additional ones
     , // separated by commas
     {}
    

    If pvt and arr need to be initialized, then it should be done here as well. I'm not sure why you just initialize one data member.

  • You don't need std::endl here:

    std::cout<<std::endl;
    

    In most cases, you don't need to do a flush along with a newline, which is what's being done by that. Instead, just output "\n" for a newline.

     std::cout << "\n";
    

Overall, I'd highly recommend making this more readable first, so that it's easier to understand and improve for yourself and others. Implementing all the algorithms yourself will especially require the code to be clean and self-documenting, otherwise you're really just writing C code instead.

added 561 characters in body
Source Link
Jamal
  • 35.2k
  • 13
  • 134
  • 238
Loading
De-Jamalized? ;-)
Source Link
Jerry Coffin
  • 34.1k
  • 4
  • 77
  • 145
Loading
Source Link
Jamal
  • 35.2k
  • 13
  • 134
  • 238
Loading
lang-cpp

AltStyle によって変換されたページ (->オリジナル) /