Here are some things that may help you improve your code.
Don't abuse using namespace std
Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.
Use nullptr rather than NULL
Modern C++ uses nullptr rather than NULL. See this answer for why and how it's useful.
Use more whitespace
Lines like this one:
cout<<"Linked List is"<<endl;
become much easier to read and understand with a little more whitespace:
cout << "Linked List is" << endl;
Use consistent formatting
The code seems to have inconsistent indentation and inconsistent placement of brackets {}. It doesn't matter as much which convention you use as much as it matters that you follow some convention. Doing so makes your code easier to read, understand and maintain.
Don't use std::endl if '\n' will do
Using std::endl emits a \n and flushes the stream. Unless you really need the stream flushed, you can improve the performance of the code by simply emitting '\n' instead of using the potentially more computationally costly std::endl. For instance, the line quoted above could be written like this:
std::cout << "Linked List is\n";
Prefer modern initializers for constructors
The constructor use the more modern initializer style rather than the old style you're currently using. Instead of this:
linklist(){
start=NULL;
}
you could use this:
linklist() : start{nullptr} {}
This uses the more consistent {} form for initializing start (and so requires a C++11 compiler) but this can be done using older C++99 if needed. There is not a significant difference in this code, but it's a good habit to get into using.
Use const where practical
The display member functions of linklist does not alter the underlying object and therefore should be declared const:
void display() const;
Make your class destructor virtual
If there's any chance that your class will be derived from, the class destructor should be virtual to avoid problems with collections of objects. See this question for more details on why. If it can't be derived from, enforce that by declaring the class final.
With that said, as @isanae has correctly pointed out, you shouldn't be left with the impression that those are the only options. For a plain old concrete class such as this one, you can reasonably just leave it as it is right now.
Return something useful from functions
The merge function takes two sorted linked lists and returns a new one; or it should. Right now it's declared like this
void linklist::merge(linklist l1, linklist l2,linklist& l3)
but it would make much more sense like this:
linklist linklist::merge(linklist l1, linklist l2)
Better still would be for the first list to be implicit so that one could write this:
l3 = l1.merge(l2);
To do that, I'd write the function like this:
linklist linklist::merge(const linklist &l2) const {
linklist l3;
node *nodes[2]{start, l2.start};
while (nodes[0] && nodes[1]) {
size_t index = nodes[0]->data < nodes[1]->data ? 0 : 1;
l3.append(nodes[index]->data);
nodes[index] = nodes[index]->link;
}
l3 += nodes[0]; // one of the lists is empty at this point
l3 += nodes[1];
return l3;
}
Note that this assumes the existence of an operator+= which might be implemented like this:
linklist &operator +=(const node *p) {
while (p) {
append(p->data);
p = p->link;
}
return *this;
}
Use operator<< instead of display
It would be convenient to be able to redirect the output to something other than std::cout, so the usual idiom is to use something like this:
friend std::ostream& operator<<(std::ostream& out, const linklist &list) {
for (auto temp=list.start; temp; temp=temp->link) {
out << temp->data << " ";
}
return out << '\n';
}
Now we can use it within main like this:
std::cout << "finished merging\n" << l1 << l2 << l3;
Consider using a template
As the code is currently written, it can only store a single int as data, but with only a very minor change in the code, it could become a template. The first few lines could look like this:
template <class T>
class linklist{
private:
struct node{
T data;
node* link;
Thereafter, anywhere you specifically refer to an int for the data, replace it with the templated type T and you now have a generic container. For your int version, you can write this:
linklist<int> l1;
Consider providing a std::initializer_list constructor
linklist(std::initializer_list<T> list) : start{nullptr} {
for (auto item : list) {
append(item);
}
}
Implementing all of the suggestions above give a main like this:
int main()
{
linklist<int> l1{2, 5, 23, 34, 45};
linklist<int> l2{6, 9, 35, 98};
std::cout << "Before merging\n" << l1 << l2;
auto l3 = l1.merge(l2);
std::cout << "After merging\n" << l1 << l2 << l3;
}
Don't destroy passed objects
Right now the code destroys the passed objects in creating the new list. That's not a good design, especially because the lists are not even passed by reference, so it would be very surprising to someone looking at the code. That can be fixed with the following suggestion.
Create a copy constructor if you need one
The compiler will create a default copy constructor which does a shallow copy, but this won't work for data structures, like yours, which use pointers. Instead, if you really want to create a new linklist from an existing one, you'll need to create your own copy constructor:
linklist(const linklist &other) : start{nullptr} {
for (auto temp = other.start; temp; temp = temp->link) {
append(temp->data);
}
}
Consider using smart pointers
Using something like a std::unique_ptr would free you (pun intended) from having to manage the mechanics of new and delete explicitly.
Omit return 0
When a C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.
