Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I'm pretty new to linked lists - I understand the concept and how they work - but coding them is a bit more...complicated. I have some code below that I'm using for homework - essentially we're to create a struct containing data for a football player, and then create a linked list and add the struct to it. I'm not 100% sure on how my code looks as I haven't compiled it - I just want to make sure that one part looks solid before I continue onto anything else.

So far I have the following:

NFL.h

#include <string>

using namespace std;
#ifndef NFL_H
#define NFL_H
struct NFL {
        string firstName;
        string lastName;
        string currentTeam;
        string Position;
        string school;
};
#endif

sortedNFL.h

#include "NFL.h"
// Header file for Sorted List ADT.  
struct NodeType;
#ifndef SORTEDNFL_H
#define SORTEDNFL_H
class SortedNFL
{
public:
  SortedNFL();     // Class constructor 
  ~SortedNFL();    // Class destructor

  bool IsFull() const;
  int  GetLength() const;
  void MakeEmpty();
  NFL GetItem(NFL& playerRequested, bool& found);
  void PutItem(NFL inputPlayer); 
  void DeleteItem(NFL playerDeleted);
  void ResetList();
  NFL GetNextItem();

private:
  NodeType* nflList;
  int length;
  NodeType* currentPos;
};
#endif

sortedNFL.cpp

#include "sortedNFL.h"

struct NodeType
{
  NFL player;
  NodeType* next;
};

sortedNFL::sortedNFL()  // Class constructor
{
  length = 0;
  nflList = NULL;
}

bool sortedNFL::IsFull() const
{
  NodeType* location;
  try
  {
    location = new NodeType;
    delete location;
    return false;
  }
  catch(std::bad_alloc exception)
  {
    return true;
  }
}

int sortedNFL::GetLength() const
{
  return length;
}

void sortedNFL::MakeEmpty()
{
  NodeType* tempPtr;

  while (nflList != NULL)
  {
    tempPtr = nflList;
    nflList = nflList->next;
    delete tempPtr;
  }
  length = 0;
}

NFL sortedNFL::GetItem(NFL& playerRequested, bool& found)
{
  bool moreToSearch;
  NodeType* location;

  location = nflList;
  found = false;
  moreToSearch = (location != NULL);

  while (moreToSearch && !found)
  {
    switch(playerRequested.lastName.ComparedTo(location->player))
    {
      case GREATER: location = location->next;
                    moreToSearch = (location != NULL);
                    break;
      case EQUAL:   found = true;
                    item = location->player;
                    break;
      case LESS:    moreToSearch = false;
                    break;
    }
  }
  return item;
}

void sortedNFL::PutItem(NFL inputPlayer)
{
  NodeType* newNode;    // pointer to node being inserted
  NodeType* predLoc;    // trailing pointer
  NodeType* location;   // traveling pointer
  bool moreToSearch;

  location = nflList;
  predLoc = NULL;
  moreToSearch = (location != NULL);

  // Find insertion point.
  while (moreToSearch)
  {
    switch(inputPlayer.lastName.ComparedTo(location->player))
    {
      case GREATER: predLoc = location;
                   location = location->next;
                    moreToSearch = (location != NULL);
                    break;
      case LESS:    moreToSearch = false;
                    break;
    }

  }

  // Prepare node for insertion
  newNode = new NodeType;
  newNode->player = inputPlayer;
  // Insert node into list.
  if (predLoc == NULL)         // Insert as first
  {
    newNode->next = nflList;
    nflList = newNode;
  }
  else
  {
    newNode->next = location;
    predLoc->next = newNode;
  }
  length++;
}
void sortedNFL::DeleteItem(NFL playerDeleted)
{
  NodeType* location = nflList;
  NodeType* tempLocation;

  // Locate node to be deleted.
  if (playerDeleted.lastName.ComparedTo(nflList->player) == EQUAL)
  {
    tempLocation = location;
    nflList = nflList->next;    // Delete first node.
  }
  else
  {
    while (playerDeleted.lastName.ComparedTo((location->next)->player) != EQUAL)
      location = location->next;

    // Delete node at location->next
    tempLocation = location->next;
    location->next = (location->next)->next;
  }
  delete tempLocation;
  length--;
}

void sortedNFL::ResetList()
{
  currentPos = NULL;
} 

NFL sortedNFL::GetNextItem()
{
  NFL item;
  if (currentPos == NULL)
    currentPos = nflList;
  item = currentPos->player; 
  currentPos = currentPos->next;
  return item;

} 

sortedNFL::~sortedNFL()
{
  NodeType* tempPtr;

  while (nflList != NULL)
  {
    tempPtr = nflList;
    nflList = nflList->next;
    delete tempPtr;
  }
}

I'm not entirely sure I grasp the concept of inserting structs into linked lists - but I think this would cover it?

share|improve this question
1  
Hi. Welcome to Code Review! "I'm not 100% sure on how my code looks as I haven't compiled it" -- this is problematic for this site. At minimum, you should have code that you have run and think works. Also, as general advice, it is much easier if you maintain your code so that you can always compile it unless you are actually changing the code. Compiling shouldn't be a difficult step. If it is, you probably need to work on your development environment. – Brythan Oct 16 '14 at 0:10
    
@Brythan fair enough - my issue was simply that I wanted to make sure the implementation was proper before I made a client file to test it, but I suppose that is a bit backwards. I'll do that and repost, thanks for the help! – Tai M. Oct 16 '14 at 0:12
    
Oh, and people are also saying that it's hard to tell what feedback you want. Not sure if the site shows you that. You could ask for style criticisms, coding advice, if the code that you think works covers all edge cases, etc. Also, you may want to add the tags beginner and reinventing-the-wheel (because you are writing your own linked list class) to this. – Brythan Oct 16 '14 at 1:22
1  
@Brythan: At this site it doesn't matter what feedback OP wants. The CR charter emphasizes that all facets are fair game, as long as the code qualifies for a review. – vnp Oct 16 '14 at 3:29
1  
@vnp Then why is there an "unclear what you are asking" close reason? Why do people keep asking posters to say what feedback they want? Most reviewers seem to prefer to have direction, so I would encourage posters to provide it. That said, if you want to provide some different feedback, I'm not arguing that you can't. I would point out that a poster is more likely to accept and/or upvote feedback that they want than feedback on some other matter. – Brythan Oct 16 '14 at 5:32

Some quick comments.

using namespace std; is not recommended, see here for why. Obviously this won't matter in a throwaway program, but it may be a bad habit to start.

Why is Position capitalized when everything else isn't?

Why are you naming a player struct NFL? Name it NflPlayer or similar (maybe even just Player).

Similarly, SortedNFL should be named something like SortedLeaguePlayerList or whatever it actually is.

Why not use the standard linked list class? See std::list for more info. If this is the assignment (to produce a linked list class of your own), then you should say so. Given your description, I would normally use the std::list.

If you do have to implement a linked list yourself, at least consider using templates so that you can reuse the code.


NFL sortedNFL::GetItem(NFL& playerRequested, bool& found)

Consider returning a reference as well. In general in C++ you should pass objects by reference.

NFL& sortedNFL::GetItem(NFL& playerRequested, bool& found)

Also, this is an uncommon idiom. The usual rule of thumb is to either pass back via reference arguments or to pass as a return value. You're doing both here, which can be confusing. You'd more commonly do

void sortedNFL::GetItem(NFL& playerRequested, bool& found)

void sortedNFL::PutItem(NFL inputPlayer)

I think that this should be

void sortedNFL::PutItem(const NFL& inputPlayer)

I don't understand the name predLoc. prevLoc would be better, but I'd go all the way and call it previousLocation. Or change location to current and predLoc to previous.

I can see other things that I think are wrong, but I believe that most of them will show up when you try to compile.

share|improve this answer
    
Ah, I should've been more clear - I'll probably end up reposting this tonight after I compile, but you've given a lot to work on, so huge thanks! I also probably should've stated that this is for a homework assignment in my Comp Sci II class, but I suppose that point is moot now. Regardless, thanks again! – Tai M. Oct 16 '14 at 14:58
  • Don't Repeat Yourself

    The code body for MakeEmpty is practically identical to the body of destructor, which means that the destructor should just call MakeEmpty. On the same tune, GetItem and PutItem share a substantial amount of code. Try to identify similarities and factor them out into a specific method (you'd be surprised how useful it would be).

  • Single Responsibility

    Expanding on similarities, PutItem does two distinct jobs: first, it finds the place to insert, and second, does the insertion. Both actions deserve their own methods, namely find and insert. Notice that GetItem also can be expressed in terms of find.

  • Return on Investment

    There is a bug in GetItem (as coded it wouldn't even compile because item is not defined anywhere). Judging by appearance, you are intending to return null if the item is not found. Think of all the work the method have done to determine that the item is not there. Think of the information unknown to caller but acquired in the process. Don't throw it away: return an "insertion point", i.e. a node after which you would insert a search target.

  • Misc

    • The ComparedTo method (not yet defined BTW) looks very strange. It compares the player to the name. Do not compare incomparables. In fact, I strongly recommend to make an NFL::less method, to compare just players. I'd even recommend to overload NFL::operator< etc.; if you didn't cover overloading yet, please disregard.

    • moreToSearch is a variable absolutely uncalled for. Them loops should be

      while (location != NULL)

    • currentPos doesn't belong to a list. It is a property of a particular traversal procedure (think of two parallel independent traversals). A correct way to implement your intention is to define an iterator. I presume that you shouldn't dive that deep now; just get rid of currentPos and GetNextitem().

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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