Skip to main content
Code Review

Return to Answer

your improvement example contained a typo that would compile, but result in and endless loop when the list size was two or larger
Source Link
if(start==NULL)
{
 start=new node{num, nullptr};
}
else
{
 node* temp = start
 while(starttemp->link != NULL)
 {
 temp=temp->link;
 }
 temp->link = new node{num, nullptr};
}
if(start==NULL)
{
 start=new node{num, nullptr};
}
else
{
 node* temp = start
 while(start->link != NULL)
 {
 temp=temp->link;
 }
 temp->link = new node{num, nullptr};
}
if(start==NULL)
{
 start=new node{num, nullptr};
}
else
{
 node* temp = start
 while(temp->link != NULL)
 {
 temp=temp->link;
 }
 temp->link = new node{num, nullptr};
}
added 199 characters in body
Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

###Remove repeated codeWhich with a bit we can use pointers to get tidy.

/* OK now that I have done it.
 * Don't do this.
 *
 * It may be slightly better but the degrade in
 * readability is not worth the tiny performance gain of not using
 * a conditional.
 */
node** find = &start;
while((*find) != NULL)
{
 find = &((*find)->link);
}
(*find)->link = new node{num, nullptr};

while(p!=NULL && q!=NULL) { if(p->data > q->data) { dat=q->data; l3.append(dat); // This is the same for both sides of the else. q=q->link; }###Remove repeated code

while(p!=NULL && q!=NULL)
{
 if(p->data > q->data)
 {
 dat=q->data;
 l3.append(dat); // This is the same for both sides of the else.
 q=q->link;
 }
 else{
 dat=p->data;
 l3.append(dat);
 p=p->link;
 }
}
if(p==NULL) // Don't need this test. Code works fine without it.
{
 while(q!=NULL)
 {
 dat=q->data;
 l3.append(dat);
 q=q->link;
 }
}
else{ // Don't need this else. Code works fine without it.
 while(p!=NULL)
 {
 dat=p->data;
 l3.append(dat);
 p=p->link;
 }
}

###Remove repeated code

while(p!=NULL && q!=NULL) { if(p->data > q->data) { dat=q->data; l3.append(dat); // This is the same for both sides of the else. q=q->link; }

 else{
 dat=p->data;
 l3.append(dat);
 p=p->link;
 }
}
if(p==NULL) // Don't need this test. Code works fine without it.
{
 while(q!=NULL)
 {
 dat=q->data;
 l3.append(dat);
 q=q->link;
 }
}
else{ // Don't need this else. Code works fine without it.
 while(p!=NULL)
 {
 dat=p->data;
 l3.append(dat);
 p=p->link;
 }
}

Which with a bit we can use pointers to get tidy.

/* OK now that I have done it.
 * Don't do this.
 *
 * It may be slightly better but the degrade in
 * readability is not worth the tiny performance gain of not using
 * a conditional.
 */
node** find = &start;
while((*find) != NULL)
{
 find = &((*find)->link);
}
(*find)->link = new node{num, nullptr};

###Remove repeated code

while(p!=NULL && q!=NULL)
{
 if(p->data > q->data)
 {
 dat=q->data;
 l3.append(dat); // This is the same for both sides of the else.
 q=q->link;
 }
 else{
 dat=p->data;
 l3.append(dat);
 p=p->link;
 }
}
if(p==NULL) // Don't need this test. Code works fine without it.
{
 while(q!=NULL)
 {
 dat=q->data;
 l3.append(dat);
 q=q->link;
 }
}
else{ // Don't need this else. Code works fine without it.
 while(p!=NULL)
 {
 dat=p->data;
 l3.append(dat);
 p=p->link;
 }
}
Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

Everything @Edward said.

Dry up your code.

You have a lot of repeated code.

###Declare objects as close to first use as possible.

 while(start!=NULL)
 {
 node* q = start->link; // Move the `node* q` declaration to here.
 delete start;
 start=q;
 }

###For loops are usually more tidy than while loops.

 for(node* next; start != null; start = next)
 {
 next = start->link;
 delete start;
 }

###Use object constructors and don't repeat code

if(start==NULL)
{temp=new node;
temp->data=num;
temp->link=NULL;
start=temp;
}
else{
 while(temp->link!=NULL)
 {
 temp=temp->link;
 }
 r= new node;
 r->data=num;
 r->link=NULL;
 temp->link=r;
}

This becomes:

if(start==NULL)
{
 start=new node{num, nullptr};
}
else
{
 node* temp = start
 while(start->link != NULL)
 {
 temp=temp->link;
 }
 temp->link = new node{num, nullptr};
}

###Remove repeated code

while(p!=NULL && q!=NULL) { if(p->data > q->data) { dat=q->data; l3.append(dat); // This is the same for both sides of the else. q=q->link; }

 else{
 dat=p->data;
 l3.append(dat);
 p=p->link;
 }
}
if(p==NULL) // Don't need this test. Code works fine without it.
{
 while(q!=NULL)
 {
 dat=q->data;
 l3.append(dat);
 q=q->link;
 }
}
else{ // Don't need this else. Code works fine without it.
 while(p!=NULL)
 {
 dat=p->data;
 l3.append(dat);
 p=p->link;
 }
}

We can refactor to this:

while(p!=NULL && q!=NULL)
{
 node** valid = (p->data > q->data)
 ? &p
 : &q;
 l3.append((*valid)->data);
 (*valid) = (*valid)->link;
}
for(;q != NULL; q = q->link)
{
 l3.append(q->data);
}
for(;p != NULL; p = p->link)
{
 l3.append(p->data);
}
lang-cpp

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