need help with my C++ program

[slayers_ai]

hi there
can anyone help me with my code? it can be compiled successfully in vc++6.0, however it fails to run.

here's the code:

//set.h

#include <iostream>
using namespace std;

struct Node{
char element;
Node* next;
};

class Set {
public:
Set();
Set(char start, char end);
void printSet();

private:
Node* head;
};

//set.cpp
#include <iostream>
#include "set.h"

Set::Set() {
head = NULL;
}

Set::Set(char start, char end) {
char temp = start;
Node* ptemp = new Node;
ptemp->element = temp;
ptemp->next = NULL;
head = ptemp;
temp++;
while (temp != end){
ptemp = ptemp->next;
ptemp->element = temp;
temp++;
}
}

void Set::printSet() {
cout << "[";

if (head != NULL){
Node* pprint = head;
cout << pprint->element;
pprint = pprint->next;
while (pprint != NULL) {
cout << ',' << pprint->element;
pprint = pprint->next;
}
}
cout << "]";
}

//main.cpp
#include "set.h"
using namespace std;

void main() {
Set B('A', 'Z');
B.printSet();
}



//basically i want to store A~Z in a linked list and print it out

[Banfa]

The problem is in the constructor Set::Set(char start, char end), you are only allocating 1 Node structure instead of 1 for each Node that the function is trying to set-up.

You need to add the line ptemp->next = new Node; as the first line of the while loop in the Set::Set(char start, char end) constructor and a line to set the next pointer of the last Node to NULL giving:

Set::Set(char start,
  char end) {
  char temp = start;
  Node* ptemp = new Node;
  ptemp->element = temp;
  ptemp->next = NULL;
  head = ptemp;
  temp++;
  while (temp != end){
    ptemp->next = new Node;
    ptemp = ptemp->next;
    ptemp->element = temp;
    temp++;
  }
  ptemp->next = NULL;
}
 

Some other points, in the constructor using ptemp and temp as variable names is not good style, in my opinion, variable names should have proper meanings, this makes the code more readable and easier to maintain 10 years down the line.

If you made Node a class (or actually even if you didn't) you could give it a constructor (or 2 say Node::Node(void ) and Node::Node(char value) ) this would add more structure to the code and provide better abstraction:

struct Node{
  char element;
  Node* next;
 
  Node::Node(void);
  Node::Node(char value);
};
 
Node:Node(void)
{
  element = '\0';
  next = NULL;
}
 
Node:Node(char value)
{
  element = value;
  next = NULL;
}
 
Set::Set(char start, char end) {
  char nextNodeValue = start;
  Node* ptemp = new Node(nextNodeValue);
  head = ptemp;
  nextNodeValue++;
  while (nextNodeValue != end){
    ptemp->next = new Node(nextNodeValue);
    ptemp = ptemp->next;
    nextNodeValue++;
  }
}
 

Note this code still isn't perfect, I have presevered the logic that you are using however by this logic the line Set B('A', 'Z'); will set up nodes for the letters A - Y, there will be no Z node, is this what you intended? Also have you thought about what will happen if start == end in Set::Set?