Inserting an element into a linked list

The name of the pictureThe name of the pictureThe name of the pictureClash Royale CLAN TAG#URR8PPP





.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;







up vote
3
down vote

favorite












This program adds an element to the end of a linked list of integers, which is previously keyed in by the user. This works fine on my computer, but I am wondering:



  1. If there is a simpler way to do this (three procedures seems a lot for this)

  2. If any of the code could potentially become problematic during future usage



program InserElement(input, output);
Has the user type in integers and forms a linked list out of them,
then inserts an element at the end of that linked list and prints the
linked list with the added new element

$mode objfpc$H+

uses
$IFDEF UNIX$IFDEF UseCThreads
cthreads,
$ENDIF$ENDIF
Classes;

type
tRefList = ^tList;
tList = record
info : integer;
next : tRefList
end;
var
RefBeginning: tRefList;
RefEnd : tRefList;
Number : integer;



procedure CreateList(var outRefBeginning: tRefList; var OutRefEnd: tRefList);
Creates a linear list through user input
var
RefNew : tRefList;

begin
writeln('Please key in natural numbers. Key in 0 once you are done. ');
readln(Number);
while Number <> 0 do
begin
new (RefNew);
RefNew^.info := Number;
RefNew^.next := nil;
if outRefBeginning = nil then
begin
outRefBeginning := RefNew;
OutRefEnd := RefNew;
end
else
begin
outRefEnd^.next := RefNew;
outRefEnd := RefNew
end;
readln (Number)
end; while-loop
end; CreateList

procedure InsertElement(inNumber : integer; var outRefBeginning : tRefList; var outRefEnd : tRefList);
Inserts a new element at the end of the list. outRefBeginning points to the first
element of that list, outRefEnd points to the last element of it. The Value of inNumber is
assigned to the record component info of the new element

var
RefNew : tRefList;

begin
Create and initialise new element
new(RefNew);
RefNew^.info := inNumber;
RefNew^.next := nil;
Insert element at the end of the linear list
if outRefBeginning = nil then
begin
outRefBeginning := RefNew;
outRefEnd := RefNew
end
else
begin
outRefEnd^.next := RefNew;
outRefEnd := RefNew;
end;
end; InsertElement

procedure PrintList;
Prints all elements of the linked list

var
RefNew : tRefList;

begin
RefNew := RefBeginning;
while RefNew <> nil do
begin
writeln (RefNew^.info);
RefNew := RefNew^.next
end;
end;

begin
RefBeginning := nil;
RefEnd := nil;
CreateList(RefBeginning, RefEnd);
InsertElement(5,RefBeginning,RefEnd);
PrintList;
readln;
end.






share|improve this question



























    up vote
    3
    down vote

    favorite












    This program adds an element to the end of a linked list of integers, which is previously keyed in by the user. This works fine on my computer, but I am wondering:



    1. If there is a simpler way to do this (three procedures seems a lot for this)

    2. If any of the code could potentially become problematic during future usage



    program InserElement(input, output);
    Has the user type in integers and forms a linked list out of them,
    then inserts an element at the end of that linked list and prints the
    linked list with the added new element

    $mode objfpc$H+

    uses
    $IFDEF UNIX$IFDEF UseCThreads
    cthreads,
    $ENDIF$ENDIF
    Classes;

    type
    tRefList = ^tList;
    tList = record
    info : integer;
    next : tRefList
    end;
    var
    RefBeginning: tRefList;
    RefEnd : tRefList;
    Number : integer;



    procedure CreateList(var outRefBeginning: tRefList; var OutRefEnd: tRefList);
    Creates a linear list through user input
    var
    RefNew : tRefList;

    begin
    writeln('Please key in natural numbers. Key in 0 once you are done. ');
    readln(Number);
    while Number <> 0 do
    begin
    new (RefNew);
    RefNew^.info := Number;
    RefNew^.next := nil;
    if outRefBeginning = nil then
    begin
    outRefBeginning := RefNew;
    OutRefEnd := RefNew;
    end
    else
    begin
    outRefEnd^.next := RefNew;
    outRefEnd := RefNew
    end;
    readln (Number)
    end; while-loop
    end; CreateList

    procedure InsertElement(inNumber : integer; var outRefBeginning : tRefList; var outRefEnd : tRefList);
    Inserts a new element at the end of the list. outRefBeginning points to the first
    element of that list, outRefEnd points to the last element of it. The Value of inNumber is
    assigned to the record component info of the new element

    var
    RefNew : tRefList;

    begin
    Create and initialise new element
    new(RefNew);
    RefNew^.info := inNumber;
    RefNew^.next := nil;
    Insert element at the end of the linear list
    if outRefBeginning = nil then
    begin
    outRefBeginning := RefNew;
    outRefEnd := RefNew
    end
    else
    begin
    outRefEnd^.next := RefNew;
    outRefEnd := RefNew;
    end;
    end; InsertElement

    procedure PrintList;
    Prints all elements of the linked list

    var
    RefNew : tRefList;

    begin
    RefNew := RefBeginning;
    while RefNew <> nil do
    begin
    writeln (RefNew^.info);
    RefNew := RefNew^.next
    end;
    end;

    begin
    RefBeginning := nil;
    RefEnd := nil;
    CreateList(RefBeginning, RefEnd);
    InsertElement(5,RefBeginning,RefEnd);
    PrintList;
    readln;
    end.






    share|improve this question























      up vote
      3
      down vote

      favorite









      up vote
      3
      down vote

      favorite











      This program adds an element to the end of a linked list of integers, which is previously keyed in by the user. This works fine on my computer, but I am wondering:



      1. If there is a simpler way to do this (three procedures seems a lot for this)

      2. If any of the code could potentially become problematic during future usage



      program InserElement(input, output);
      Has the user type in integers and forms a linked list out of them,
      then inserts an element at the end of that linked list and prints the
      linked list with the added new element

      $mode objfpc$H+

      uses
      $IFDEF UNIX$IFDEF UseCThreads
      cthreads,
      $ENDIF$ENDIF
      Classes;

      type
      tRefList = ^tList;
      tList = record
      info : integer;
      next : tRefList
      end;
      var
      RefBeginning: tRefList;
      RefEnd : tRefList;
      Number : integer;



      procedure CreateList(var outRefBeginning: tRefList; var OutRefEnd: tRefList);
      Creates a linear list through user input
      var
      RefNew : tRefList;

      begin
      writeln('Please key in natural numbers. Key in 0 once you are done. ');
      readln(Number);
      while Number <> 0 do
      begin
      new (RefNew);
      RefNew^.info := Number;
      RefNew^.next := nil;
      if outRefBeginning = nil then
      begin
      outRefBeginning := RefNew;
      OutRefEnd := RefNew;
      end
      else
      begin
      outRefEnd^.next := RefNew;
      outRefEnd := RefNew
      end;
      readln (Number)
      end; while-loop
      end; CreateList

      procedure InsertElement(inNumber : integer; var outRefBeginning : tRefList; var outRefEnd : tRefList);
      Inserts a new element at the end of the list. outRefBeginning points to the first
      element of that list, outRefEnd points to the last element of it. The Value of inNumber is
      assigned to the record component info of the new element

      var
      RefNew : tRefList;

      begin
      Create and initialise new element
      new(RefNew);
      RefNew^.info := inNumber;
      RefNew^.next := nil;
      Insert element at the end of the linear list
      if outRefBeginning = nil then
      begin
      outRefBeginning := RefNew;
      outRefEnd := RefNew
      end
      else
      begin
      outRefEnd^.next := RefNew;
      outRefEnd := RefNew;
      end;
      end; InsertElement

      procedure PrintList;
      Prints all elements of the linked list

      var
      RefNew : tRefList;

      begin
      RefNew := RefBeginning;
      while RefNew <> nil do
      begin
      writeln (RefNew^.info);
      RefNew := RefNew^.next
      end;
      end;

      begin
      RefBeginning := nil;
      RefEnd := nil;
      CreateList(RefBeginning, RefEnd);
      InsertElement(5,RefBeginning,RefEnd);
      PrintList;
      readln;
      end.






      share|improve this question













      This program adds an element to the end of a linked list of integers, which is previously keyed in by the user. This works fine on my computer, but I am wondering:



      1. If there is a simpler way to do this (three procedures seems a lot for this)

      2. If any of the code could potentially become problematic during future usage



      program InserElement(input, output);
      Has the user type in integers and forms a linked list out of them,
      then inserts an element at the end of that linked list and prints the
      linked list with the added new element

      $mode objfpc$H+

      uses
      $IFDEF UNIX$IFDEF UseCThreads
      cthreads,
      $ENDIF$ENDIF
      Classes;

      type
      tRefList = ^tList;
      tList = record
      info : integer;
      next : tRefList
      end;
      var
      RefBeginning: tRefList;
      RefEnd : tRefList;
      Number : integer;



      procedure CreateList(var outRefBeginning: tRefList; var OutRefEnd: tRefList);
      Creates a linear list through user input
      var
      RefNew : tRefList;

      begin
      writeln('Please key in natural numbers. Key in 0 once you are done. ');
      readln(Number);
      while Number <> 0 do
      begin
      new (RefNew);
      RefNew^.info := Number;
      RefNew^.next := nil;
      if outRefBeginning = nil then
      begin
      outRefBeginning := RefNew;
      OutRefEnd := RefNew;
      end
      else
      begin
      outRefEnd^.next := RefNew;
      outRefEnd := RefNew
      end;
      readln (Number)
      end; while-loop
      end; CreateList

      procedure InsertElement(inNumber : integer; var outRefBeginning : tRefList; var outRefEnd : tRefList);
      Inserts a new element at the end of the list. outRefBeginning points to the first
      element of that list, outRefEnd points to the last element of it. The Value of inNumber is
      assigned to the record component info of the new element

      var
      RefNew : tRefList;

      begin
      Create and initialise new element
      new(RefNew);
      RefNew^.info := inNumber;
      RefNew^.next := nil;
      Insert element at the end of the linear list
      if outRefBeginning = nil then
      begin
      outRefBeginning := RefNew;
      outRefEnd := RefNew
      end
      else
      begin
      outRefEnd^.next := RefNew;
      outRefEnd := RefNew;
      end;
      end; InsertElement

      procedure PrintList;
      Prints all elements of the linked list

      var
      RefNew : tRefList;

      begin
      RefNew := RefBeginning;
      while RefNew <> nil do
      begin
      writeln (RefNew^.info);
      RefNew := RefNew^.next
      end;
      end;

      begin
      RefBeginning := nil;
      RefEnd := nil;
      CreateList(RefBeginning, RefEnd);
      InsertElement(5,RefBeginning,RefEnd);
      PrintList;
      readln;
      end.








      share|improve this question












      share|improve this question




      share|improve this question








      edited Jan 26 at 4:17









      Jamal♦

      30.1k11114225




      30.1k11114225









      asked Jan 26 at 3:51









      Lucky

      403




      403




















          1 Answer
          1






          active

          oldest

          votes

















          up vote
          2
          down vote



          accepted










          Overall, this is a great start. It's easy to read and understand. Personally, I don't think that 3 procedures is a lot. You have 3 things you need to do: create the initial list, add an element to it, and print out the results. If you didn't have at least 3 procedures, I'd say you were doing something wrong. Anyway, here are some things that could be improved.



          Naming



          I found the naming a little confusing. Why is it a "ref" list? To what does it refer? (Or what does ref mean in this context if not "reference"?) I think that I would name the list node type a tNode instead of a tList, and I would name the thing that points to it a tList.



          I notice also hat some of your variable and argument names begin with lowercase letters and some with uppercase. While Pascal is not typically a case-sensitive language, it's generally considered good practice to be consistent. Often variable names start with a lowercase letter, and types start with an uppercase letter, but it's up to you. It just makes it easier to read when it's consistent.



          Types



          Better yet, if you're going to keep track of the head and tail separately, I would make another record type, like this:



          type
          tNodePtr = ^tNode;
          tNode = record
          info : integer;
          next : tNodePtr;
          end;

          tList = record
          head : tNodePtr;
          tail : tNodePtr;
          end;


          Now you can pass a single variable around instead of passing both the head and tail to each procedure. This makes it easier to read the code and less likely that a caller of one of the procedures will accidentally pass the wrong thing for one of the arguments.



          Check Your Allocations



          There are 2 places where you call new(). You don't ever check the result, though, to see if the allocation succeeded. You need to make sure that the memory was actually allocated before you start writing to it. Otherwise, you'll end up writing over some other data causing a hard-to-find bug, or writing to data you don't have access to and causing a crash.



          Delete Stuff When You're Done With It



          The other thing you need to do with memory that you allocate from the heap is delete it when you're no longer using it. In this case, that would be at the end of the program. I would write a procedure to delete the nodes in the list. Something like this (and I'm assuming you're using the types I recommended above):



          procedure DeleteList(var list : tList)
          var
          nextNode : tNodePtr;
          currNode : tNodePtr;
          begin
          nextNode = list.head;
          currNode = list.head;
          while nextNode <> nil do
          begin
          nextNode = currNode^.next;
          delete(currNode);
          currNode = nextNode;
          end;
          list.head = nil;
          list.tail = nil;
          end;


          (Note: It's been a few years since I've written Pascal, so forgive me if I've made any obvious mistakes here!)



          Formatting



          While this won't affect the running of your program, the formatting of the code can affect how easy it is to read and understand. I would suggest adopting a consistent indentation amount. I see that you sometimes use 1 space, sometimes 2. I would recommend always using 2 or more (my preference is 4, personally), as a single space is not as easy to distinguish as 2 or more.



          Also, it looks like in CreateList() the if class has one type of indenting, and the else clause has a different one. I would be consistent about what level of indentation you use for the begin and end keywords.






          share|improve this answer





















          • Thank you for the great advice, this will definitely help me for future tasks! I wrote the code in German and then tried to translate it into English, hence the rather unusual naming ("ref" stands for reference, or pointer).
            – Lucky
            Jan 26 at 6:44










          Your Answer




          StackExchange.ifUsing("editor", function ()
          return StackExchange.using("mathjaxEditing", function ()
          StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
          StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
          );
          );
          , "mathjax-editing");

          StackExchange.ifUsing("editor", function ()
          StackExchange.using("externalEditor", function ()
          StackExchange.using("snippets", function ()
          StackExchange.snippets.init();
          );
          );
          , "code-snippets");

          StackExchange.ready(function()
          var channelOptions =
          tags: "".split(" "),
          id: "196"
          ;
          initTagRenderer("".split(" "), "".split(" "), channelOptions);

          StackExchange.using("externalEditor", function()
          // Have to fire editor after snippets, if snippets enabled
          if (StackExchange.settings.snippets.snippetsEnabled)
          StackExchange.using("snippets", function()
          createEditor();
          );

          else
          createEditor();

          );

          function createEditor()
          StackExchange.prepareEditor(
          heartbeatType: 'answer',
          convertImagesToLinks: false,
          noModals: false,
          showLowRepImageUploadWarning: true,
          reputationToPostImages: null,
          bindNavPrevention: true,
          postfix: "",
          onDemand: true,
          discardSelector: ".discard-answer"
          ,immediatelyShowMarkdownHelp:true
          );



          );








           

          draft saved


          draft discarded


















          StackExchange.ready(
          function ()
          StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f186027%2finserting-an-element-into-a-linked-list%23new-answer', 'question_page');

          );

          Post as a guest






























          1 Answer
          1






          active

          oldest

          votes








          1 Answer
          1






          active

          oldest

          votes









          active

          oldest

          votes






          active

          oldest

          votes








          up vote
          2
          down vote



          accepted










          Overall, this is a great start. It's easy to read and understand. Personally, I don't think that 3 procedures is a lot. You have 3 things you need to do: create the initial list, add an element to it, and print out the results. If you didn't have at least 3 procedures, I'd say you were doing something wrong. Anyway, here are some things that could be improved.



          Naming



          I found the naming a little confusing. Why is it a "ref" list? To what does it refer? (Or what does ref mean in this context if not "reference"?) I think that I would name the list node type a tNode instead of a tList, and I would name the thing that points to it a tList.



          I notice also hat some of your variable and argument names begin with lowercase letters and some with uppercase. While Pascal is not typically a case-sensitive language, it's generally considered good practice to be consistent. Often variable names start with a lowercase letter, and types start with an uppercase letter, but it's up to you. It just makes it easier to read when it's consistent.



          Types



          Better yet, if you're going to keep track of the head and tail separately, I would make another record type, like this:



          type
          tNodePtr = ^tNode;
          tNode = record
          info : integer;
          next : tNodePtr;
          end;

          tList = record
          head : tNodePtr;
          tail : tNodePtr;
          end;


          Now you can pass a single variable around instead of passing both the head and tail to each procedure. This makes it easier to read the code and less likely that a caller of one of the procedures will accidentally pass the wrong thing for one of the arguments.



          Check Your Allocations



          There are 2 places where you call new(). You don't ever check the result, though, to see if the allocation succeeded. You need to make sure that the memory was actually allocated before you start writing to it. Otherwise, you'll end up writing over some other data causing a hard-to-find bug, or writing to data you don't have access to and causing a crash.



          Delete Stuff When You're Done With It



          The other thing you need to do with memory that you allocate from the heap is delete it when you're no longer using it. In this case, that would be at the end of the program. I would write a procedure to delete the nodes in the list. Something like this (and I'm assuming you're using the types I recommended above):



          procedure DeleteList(var list : tList)
          var
          nextNode : tNodePtr;
          currNode : tNodePtr;
          begin
          nextNode = list.head;
          currNode = list.head;
          while nextNode <> nil do
          begin
          nextNode = currNode^.next;
          delete(currNode);
          currNode = nextNode;
          end;
          list.head = nil;
          list.tail = nil;
          end;


          (Note: It's been a few years since I've written Pascal, so forgive me if I've made any obvious mistakes here!)



          Formatting



          While this won't affect the running of your program, the formatting of the code can affect how easy it is to read and understand. I would suggest adopting a consistent indentation amount. I see that you sometimes use 1 space, sometimes 2. I would recommend always using 2 or more (my preference is 4, personally), as a single space is not as easy to distinguish as 2 or more.



          Also, it looks like in CreateList() the if class has one type of indenting, and the else clause has a different one. I would be consistent about what level of indentation you use for the begin and end keywords.






          share|improve this answer





















          • Thank you for the great advice, this will definitely help me for future tasks! I wrote the code in German and then tried to translate it into English, hence the rather unusual naming ("ref" stands for reference, or pointer).
            – Lucky
            Jan 26 at 6:44














          up vote
          2
          down vote



          accepted










          Overall, this is a great start. It's easy to read and understand. Personally, I don't think that 3 procedures is a lot. You have 3 things you need to do: create the initial list, add an element to it, and print out the results. If you didn't have at least 3 procedures, I'd say you were doing something wrong. Anyway, here are some things that could be improved.



          Naming



          I found the naming a little confusing. Why is it a "ref" list? To what does it refer? (Or what does ref mean in this context if not "reference"?) I think that I would name the list node type a tNode instead of a tList, and I would name the thing that points to it a tList.



          I notice also hat some of your variable and argument names begin with lowercase letters and some with uppercase. While Pascal is not typically a case-sensitive language, it's generally considered good practice to be consistent. Often variable names start with a lowercase letter, and types start with an uppercase letter, but it's up to you. It just makes it easier to read when it's consistent.



          Types



          Better yet, if you're going to keep track of the head and tail separately, I would make another record type, like this:



          type
          tNodePtr = ^tNode;
          tNode = record
          info : integer;
          next : tNodePtr;
          end;

          tList = record
          head : tNodePtr;
          tail : tNodePtr;
          end;


          Now you can pass a single variable around instead of passing both the head and tail to each procedure. This makes it easier to read the code and less likely that a caller of one of the procedures will accidentally pass the wrong thing for one of the arguments.



          Check Your Allocations



          There are 2 places where you call new(). You don't ever check the result, though, to see if the allocation succeeded. You need to make sure that the memory was actually allocated before you start writing to it. Otherwise, you'll end up writing over some other data causing a hard-to-find bug, or writing to data you don't have access to and causing a crash.



          Delete Stuff When You're Done With It



          The other thing you need to do with memory that you allocate from the heap is delete it when you're no longer using it. In this case, that would be at the end of the program. I would write a procedure to delete the nodes in the list. Something like this (and I'm assuming you're using the types I recommended above):



          procedure DeleteList(var list : tList)
          var
          nextNode : tNodePtr;
          currNode : tNodePtr;
          begin
          nextNode = list.head;
          currNode = list.head;
          while nextNode <> nil do
          begin
          nextNode = currNode^.next;
          delete(currNode);
          currNode = nextNode;
          end;
          list.head = nil;
          list.tail = nil;
          end;


          (Note: It's been a few years since I've written Pascal, so forgive me if I've made any obvious mistakes here!)



          Formatting



          While this won't affect the running of your program, the formatting of the code can affect how easy it is to read and understand. I would suggest adopting a consistent indentation amount. I see that you sometimes use 1 space, sometimes 2. I would recommend always using 2 or more (my preference is 4, personally), as a single space is not as easy to distinguish as 2 or more.



          Also, it looks like in CreateList() the if class has one type of indenting, and the else clause has a different one. I would be consistent about what level of indentation you use for the begin and end keywords.






          share|improve this answer





















          • Thank you for the great advice, this will definitely help me for future tasks! I wrote the code in German and then tried to translate it into English, hence the rather unusual naming ("ref" stands for reference, or pointer).
            – Lucky
            Jan 26 at 6:44












          up vote
          2
          down vote



          accepted







          up vote
          2
          down vote



          accepted






          Overall, this is a great start. It's easy to read and understand. Personally, I don't think that 3 procedures is a lot. You have 3 things you need to do: create the initial list, add an element to it, and print out the results. If you didn't have at least 3 procedures, I'd say you were doing something wrong. Anyway, here are some things that could be improved.



          Naming



          I found the naming a little confusing. Why is it a "ref" list? To what does it refer? (Or what does ref mean in this context if not "reference"?) I think that I would name the list node type a tNode instead of a tList, and I would name the thing that points to it a tList.



          I notice also hat some of your variable and argument names begin with lowercase letters and some with uppercase. While Pascal is not typically a case-sensitive language, it's generally considered good practice to be consistent. Often variable names start with a lowercase letter, and types start with an uppercase letter, but it's up to you. It just makes it easier to read when it's consistent.



          Types



          Better yet, if you're going to keep track of the head and tail separately, I would make another record type, like this:



          type
          tNodePtr = ^tNode;
          tNode = record
          info : integer;
          next : tNodePtr;
          end;

          tList = record
          head : tNodePtr;
          tail : tNodePtr;
          end;


          Now you can pass a single variable around instead of passing both the head and tail to each procedure. This makes it easier to read the code and less likely that a caller of one of the procedures will accidentally pass the wrong thing for one of the arguments.



          Check Your Allocations



          There are 2 places where you call new(). You don't ever check the result, though, to see if the allocation succeeded. You need to make sure that the memory was actually allocated before you start writing to it. Otherwise, you'll end up writing over some other data causing a hard-to-find bug, or writing to data you don't have access to and causing a crash.



          Delete Stuff When You're Done With It



          The other thing you need to do with memory that you allocate from the heap is delete it when you're no longer using it. In this case, that would be at the end of the program. I would write a procedure to delete the nodes in the list. Something like this (and I'm assuming you're using the types I recommended above):



          procedure DeleteList(var list : tList)
          var
          nextNode : tNodePtr;
          currNode : tNodePtr;
          begin
          nextNode = list.head;
          currNode = list.head;
          while nextNode <> nil do
          begin
          nextNode = currNode^.next;
          delete(currNode);
          currNode = nextNode;
          end;
          list.head = nil;
          list.tail = nil;
          end;


          (Note: It's been a few years since I've written Pascal, so forgive me if I've made any obvious mistakes here!)



          Formatting



          While this won't affect the running of your program, the formatting of the code can affect how easy it is to read and understand. I would suggest adopting a consistent indentation amount. I see that you sometimes use 1 space, sometimes 2. I would recommend always using 2 or more (my preference is 4, personally), as a single space is not as easy to distinguish as 2 or more.



          Also, it looks like in CreateList() the if class has one type of indenting, and the else clause has a different one. I would be consistent about what level of indentation you use for the begin and end keywords.






          share|improve this answer













          Overall, this is a great start. It's easy to read and understand. Personally, I don't think that 3 procedures is a lot. You have 3 things you need to do: create the initial list, add an element to it, and print out the results. If you didn't have at least 3 procedures, I'd say you were doing something wrong. Anyway, here are some things that could be improved.



          Naming



          I found the naming a little confusing. Why is it a "ref" list? To what does it refer? (Or what does ref mean in this context if not "reference"?) I think that I would name the list node type a tNode instead of a tList, and I would name the thing that points to it a tList.



          I notice also hat some of your variable and argument names begin with lowercase letters and some with uppercase. While Pascal is not typically a case-sensitive language, it's generally considered good practice to be consistent. Often variable names start with a lowercase letter, and types start with an uppercase letter, but it's up to you. It just makes it easier to read when it's consistent.



          Types



          Better yet, if you're going to keep track of the head and tail separately, I would make another record type, like this:



          type
          tNodePtr = ^tNode;
          tNode = record
          info : integer;
          next : tNodePtr;
          end;

          tList = record
          head : tNodePtr;
          tail : tNodePtr;
          end;


          Now you can pass a single variable around instead of passing both the head and tail to each procedure. This makes it easier to read the code and less likely that a caller of one of the procedures will accidentally pass the wrong thing for one of the arguments.



          Check Your Allocations



          There are 2 places where you call new(). You don't ever check the result, though, to see if the allocation succeeded. You need to make sure that the memory was actually allocated before you start writing to it. Otherwise, you'll end up writing over some other data causing a hard-to-find bug, or writing to data you don't have access to and causing a crash.



          Delete Stuff When You're Done With It



          The other thing you need to do with memory that you allocate from the heap is delete it when you're no longer using it. In this case, that would be at the end of the program. I would write a procedure to delete the nodes in the list. Something like this (and I'm assuming you're using the types I recommended above):



          procedure DeleteList(var list : tList)
          var
          nextNode : tNodePtr;
          currNode : tNodePtr;
          begin
          nextNode = list.head;
          currNode = list.head;
          while nextNode <> nil do
          begin
          nextNode = currNode^.next;
          delete(currNode);
          currNode = nextNode;
          end;
          list.head = nil;
          list.tail = nil;
          end;


          (Note: It's been a few years since I've written Pascal, so forgive me if I've made any obvious mistakes here!)



          Formatting



          While this won't affect the running of your program, the formatting of the code can affect how easy it is to read and understand. I would suggest adopting a consistent indentation amount. I see that you sometimes use 1 space, sometimes 2. I would recommend always using 2 or more (my preference is 4, personally), as a single space is not as easy to distinguish as 2 or more.



          Also, it looks like in CreateList() the if class has one type of indenting, and the else clause has a different one. I would be consistent about what level of indentation you use for the begin and end keywords.







          share|improve this answer













          share|improve this answer



          share|improve this answer











          answered Jan 26 at 6:09









          user1118321

          10.2k11144




          10.2k11144











          • Thank you for the great advice, this will definitely help me for future tasks! I wrote the code in German and then tried to translate it into English, hence the rather unusual naming ("ref" stands for reference, or pointer).
            – Lucky
            Jan 26 at 6:44
















          • Thank you for the great advice, this will definitely help me for future tasks! I wrote the code in German and then tried to translate it into English, hence the rather unusual naming ("ref" stands for reference, or pointer).
            – Lucky
            Jan 26 at 6:44















          Thank you for the great advice, this will definitely help me for future tasks! I wrote the code in German and then tried to translate it into English, hence the rather unusual naming ("ref" stands for reference, or pointer).
          – Lucky
          Jan 26 at 6:44




          Thank you for the great advice, this will definitely help me for future tasks! I wrote the code in German and then tried to translate it into English, hence the rather unusual naming ("ref" stands for reference, or pointer).
          – Lucky
          Jan 26 at 6:44












           

          draft saved


          draft discarded


























           


          draft saved


          draft discarded














          StackExchange.ready(
          function ()
          StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f186027%2finserting-an-element-into-a-linked-list%23new-answer', 'question_page');

          );

          Post as a guest













































































          Popular posts from this blog

          Chat program with C++ and SFML

          Function to Return a JSON Like Objects Using VBA Collections and Arrays

          Will my employers contract hold up in court?