StuXYZ 731 Practically a Master Poster

This method seems strange to me .

The first thing that is obviously wrong is these lines:

 while((*bIter)<(*aIter)){
            bIter++;
        }

What happens if bIter is at the last position in the array.

So in short I would have written something like this

  std::vector<int>::const_iterator aIter=aList.begin();
  std::vector<int>::const_iterator bIter=bList.begin();
  if (bIter==bList.end()) return 0;

  for(;aIter!=aList.end();aIter++)
    {
      while(*bIter < *aIter)
        {
          if (++bIter==bList.end()) break;
        }
     if (*bIter == *aIter)
        cList.push_back(*bIter);
    }

Finally there is always the standard algorithms and you can do this:

#include <algorithm> 

//.....

std::set_intersection(aList.begin(), aList.end(),
            bList.begin(), bList.end(),                       
            std::back_inserter(cList));

I am not 100% certain of its speed but is it quick and easy to write and normally the stl is quick enough relative to simple hand coded stuff.

Notes:

I am very disinclinded to use auto when the algorithm only works for int and not doubles or other number types.

Second many may not like the break in the middle of the loop -- you can put it outside if you like.

StuXYZ 731 Practically a Master Poster

I agree a bit with rproffitt, but without actually running the code I noticed the following:

line 51 : elements[x++]= M[i][j]; x is not initialized. Might and might not start as 0.

line 64 elements[k] = elements[k+1]; In the first parse if elements[i] == elements [j] then this line sets elements[k] to be element[k+1] BUT the loop is for(k=j; k < *n;k++) which means that you have stepped passed the end of the array.

Beyond that I am not sure what you want the code to do -- so fix this simple stuff and if you can add an example of what the code should do and what it does do, e.g. for a 3x3 matrix or something small, that would help.

rproffitt commented: Nice find. I have a story about uninitialized variables. Big money, adventures were had. +11
StuXYZ 731 Practically a Master Poster

Your code is nearly correct but I am not 100% sure that it is giving your what your want:
So here is your code with some comments -- just to make sure:

  // This line sets the random number seed. 
  // you only need this call once only in your code 
  srand(time(NULL)); 

 // This is strange :  -- your code produces a random
 // number between 1.0 and 2.0. This is because rand() returns an 
 // integer between 0 and RAND_MAX so the division gives you 
 // a number between 0.0 and 1.0  and then you add 1. 
 float   random1 = (float(rand())/(RAND_MAX)) + 1;
 float   random2 = (float(rand())/(RAND_MAX)) + 1;

// Now this is fine
x = a* random1 + b* random2; 

// this line is strange becauses random1 and random2 have not changed.
// I expected to see ANOTHER copy of this:
random1= float(rand())/MAX_RAND +1.0;
 y = c* random1 + d* random2;

If you test your code with* a and *b then same value as c and d, I expect you want a different value for x and y.

(note : don't forget your semi-colons!)

StuXYZ 731 Practically a Master Poster

Sorry but I really don't like this piece of code from skaa.

The problem for me is that double precision numbers have floating point error, so you really don't know [most of the time] if they are going to be equal e.g. is 1e-76 equal to 1.1e-76. This kind of thing occurs all the time when doing arithmatic with double precision numbers and is often refered to as round off error.

For a 3d vector space, the one way to compare is by the Euclidian distance between two vector points. This can be readily found by calculating the inner product [often called the dot product] of the difference vector and then comparing the result, either to a fraction of the magnatude of the original vectors or to a fixed tolerance. e.g.

bool Point3D::operator==(const Point3D& A) const
{
  const double Tol(1e-8*1e-8);   // Squared tolerance  
  if (this!=&A)
    {
       const double D=(A.x-x)*(A.x-x)+
                      (A.y-y)*(A.y-y)+
                      (A.z-z)*(A.z-z);
       if (fabs(D)>Tol) return 0;
    }
  return 1;
}

Not the best code (as normally Point3D would provide its own dot product method and you would use that.

Note that a dot product would normal have a square root e.g. sqrt(D), but since it is only a comparison, we can avoid that extra computation.

However, that is not the only comparison that you might like to make. You might be interested in direction, e.g. are the point vector parallel regardless of size: e.g. a.b / |a| |b|
[which is is dot product of a with b divided …

StuXYZ 731 Practically a Master Poster

First of all I don't think I can tell you what is wrong to fix your current bug.. but here are a few thoughts, and I think we need to see the calling code for Picture. However, the following might help [hopefully!].

The key elements here are that your default constructor image() sets framecount to zero. Only the constructor called with a string
e.g. image test("xx.jnp"); actually sets framecount.

The call to void Picture(image imgIcon) worries me.

First, it seems you forgot to make it a const reference, e.g.
void Picture(const image& imgIcon) . That way you would avoid the double copy constructor call [I think the compiler MIGHT be able to optimise that out but not sure] as you do imgTest=imgIcon.

Next you also have no guard in the operator=. Almost ALL operator= methods should have the form:

image&
image::operator=(const image& A)
{
   if (this!=&A)
     {
       // stuff
     }
   return *this;
}

Yes that are exceptions -- BUT start with the standard form and if need be change and add a comment as to why it needed to be changed.

So in short, we need to see the call to Picture and the contruction of the object that was passed into the function Picture, if these couple of thoughts don't help.

p.s. Check the typo on line 218. you didn't mean

 pxxels[1] [X] [Y] = pixels[1] [X] [Y]+2;
 pixels[1] [X] [Y] = pixels[1] [X] [Y]+2;

it probabily should be [2] on the second …

cambalinho commented: thanks +3
StuXYZ 731 Practically a Master Poster

There are several problems with this code :

(i) you are using double's which can represent non-integer numbers. The problem with that is if you say if (a==b) but actually a=3.00000000001 and b=3.000000000000000, they are not equal. When you do arithmatic between two numbers you are certain to have "round-off error" this occures because the accuracy of the number is limited and by increasing/decreasing the size the least significant bits are dropped/added.

The solution is to (a) not to use double's , (b) put a test to say if the result is approximately correct e.g. if (fabs(result-correct)<1e-6) [note the fabs function gets the absolute (positive) value so even if correct is a little bit bigger than result it works].

(ii) You write out CORRECT and WRONG but they are (a) not initialised (you now have no idea what they start as) and (b) not incremented.

(iii) your code that test for correctness is repeated -- have a look at removing it from the switch statement.

(iv) I don't see were the do { is closed, I expected a } while (...); somewhere.

StuXYZ 731 Practically a Master Poster

The quick solution to this is to actually know the algorithm that the knights tour works on. It can be shown that for any size square and rectangular board (above a minimum size), that simply selecting the unvisited square with the least possible open moves from it, is the optimal strategy.

The use of an algorithm like that greatly simplifies the problem.

However, the question asked is how to do it with recusion and a stack.

There are several methods to do this but I will outline just one.

Consider the first move; the Knight has a number of positions to go to [we will ignore symmetry reductions that can simplify this further]

If we define a simple mechanism of ordering moves, say we use a clock trick e.g. the move at 1 o'clock is move 1, the move at 3o'clock is move 2 etc, if a move not possible [off side of the board], then just carry on counting to the next one.

Take the first move, put onto a stack the first option (always 1 in this case).

Next update the visited map, with the original square and the first move.

Take the second move, do the same, most likely the first square again so add another 1 to the stack. and update the visited map.

At some point you will get to a move were the first move (1) is blocked by a visited square, no problem look for the next move 2, if that is ok …

StuXYZ 731 Practically a Master Poster

Some of the things you will see are that you have looped over the end of your arrays. e.g. line 18 and 36. You have written:

double y[21];
// stuff...
for(int k=0;k<21;k++)
  {
    y[k+3]=-((a1*y[k+2])+(a2*y[k+1])+(a3*y[k]))/a0);
  }

Note that k+3 means that k goes to 24. That will cause memory corruption. Additionally, since you only use up to y[20] in the output, why leave the loop all the way to 20, why not stop at k==17.

(note that there is the same error with z as well.

If you are going to have another look at this code, you can write it without needing to use an array beyond size 3.

Finally, as you may know, the algorithm as given is highly numerically unstable. There are a number of re-normalising operations that can be done after each step to improve things.

ddanbe commented: Deep insights. +15
StuXYZ 731 Practically a Master Poster

Memory leaks in C++ are about balance and understanding scope [a bit].

First of the fundermentals: for every allocation there must be an equal deallocation.

That is about it.

Consider your example:

void 
afunction(int *x)
{
  // Memory allocation BUT no deallocation
  x=new int;
  *x=12;
}

int main()
{
  // No explicit memory allocation or deallocation in this function!
  int v=10;
  afunction(&v);
  cout<<v;
}

Here you can see one new but no delete. Thus you must have a memory leak.

This is insignificant in this programs, (one extra int) and when the program finishes the memory is returned to the system. BUT imagen that this function is called thousands and thousands of times ... then we have a problem.

Further on this subject : consider the following broken code:

void func(const int N)
{
   int* Array = new int[N];
   // ... do stuff with Array

   delete Array; 
}

Note it doesn't have the correct deletion operator, it should have had delete [] Array, as it was allocated as a block.

These examples are relatively simple, but a major cause of confusion it seems [if the code reviews I go to are anything to go on] are scope rule: Consider this code:

   void func(const int N)
    {
       int Array[100];
       if (N<100)
         {
            // ... do stuff with Array
         }
       delete Array; 
       return;
    }

Now we see a delete without a new. The array was not initialed with a new. So no delete! The array …

Ancient Dragon commented: explained very well +14
StuXYZ 731 Practically a Master Poster

The obvious uses are all around -- I will list a few:

Control room of a complex system (e.g. a nuclear reactor). In a typcially control room, you want the imformation that matters to be salient. That doesn't just include alarms but information that leads up to an alarm, e.g. water pressures or cooling flow rates, or temperature asymmetry in the core etc.

Second is dager type signals/signs. We have all tripped over the stupid "floor is wet" signs. They are NOT salient and represent a bigger hazard than the hazard that is actually signed about. But now consider that someone is using a radioactive source in a room that typically doesn't have one in. The sign should be salient, you really want to know. Same is true to signs on chemistry labs. There is lots of dangerous stuff in a typcial chemistry lab, but most chemists are aware of the normal stuff, but when you add something different/unusual even if it is not particularly dangerous, THAT saftey sign should to be more salient than the normal stuff -- but how should you achieve that?

Further what about reaction time incidents, anything were you want a rapid response from humans, and in particular crowds. The saliency of the signage can greatly influence the rate at which a human or a crowd of humans react to a change in situation.

The is also the salience relative to risk aspect. It seems [disputed and a subject to research] that the saliency of …

StuXYZ 731 Practically a Master Poster

There really is not enough to go on here. sorry.

But here is my memory error check list:

(a) Have you zeroed the pointer before trying to free it e.g.

CvPoint* PointArray=new PointArray[10];
  // stuff
  PoinitArray=0;
  delete [] PointArray;

(b) are you mixing C++ and C style memory allocations e.g. (or C and C++ delete.)

   CvPoint* PointArray=new PointArray[10];
      // stuff
      free(PointArray);

(c) Do you have loop variable that is not getting set to zero and continuously being increased and thus making your array too big to be allocated.

If all of the obvious doesn't solve the problem quickly, then tools like valgrind normally help lots.

Finally, you could post a little more code, especially the bits that allocate and deallocate memory.

StuXYZ 731 Practically a Master Poster

Ok there is lots and lots of little problems with this code.

But before I talk about some of them, I don't understand your use of the word iterator. In C++, the standard template library provides iterators to most container classes. In your instance you are providing a container class (OrderdList) and I would have expected your iterator to behave similarly to the existing iterators, i.e. as something that accesses members of the container in an order and without repeat.

In your case you have added ListIterator as a friend [high probability of poor design] and it does none of those things, but acts as a kind of "extra functionallity" namespace, that provides the equivilent of algorithms.

So for example: you write

OrderedList<Student> studentList(10);
Student s1("Hamza",12105092);
ListIterator<Student> studentIterator(studentList);
studentIterator.insertData(s1);

When something like, this would be expected:

OrderedList<Student> studentList(10);
studentList.push(Student("Hamaz",12105092);
//...
ListIterator<Student> IT;
for(IT=studentList.begin();IT!=studentList.end();IT++)
   std::cout<<"Student == "<<*IT<<std::endl;

Nothing intrisically wrong, in doing what you are doing, but it is unconvensional, probabily not what is asked for, and going to cause real confusion if you show it to someone else.

---------------

That said: What else is wrong with your code

(i) drop the constant on the assignment operator, and use this form:
OrderedList& operator= (const OrderedList&);

(ii) Care with array boundaries: Eg Image an ordered list of 10 items and 10 max size, in the method removeAt:

      bool removeAt(int index) {  

        if(index < 0 || index >= currentSize) 
          throw ILLEGAL_INDEX;
        else 
          {
            for(int i=index;i<currentSize;i++) 
          // i+1 …
StuXYZ 731 Practically a Master Poster

You error seems to be in the copy constructor/assignment operator of way.

If you add :

Way::Way(const Way& orig) : 
  id(orig.id),name(orig.name)
{}

Way& Way::operator=(const Way& A)
  {
     if (this!=&A)
       {
          id=A.id;
          name=A.id;
        }
     return A;
   }

All works as I would expect. [Note you also have to add a declaration to the class definition of Way for the assignment operator.

So let us look at WHY that is:

The problem comes from the methog you use to write a wayNode to the AreaDataRepository. First off you write

void SetOnGoingWay(Way onGoingWay) { this->onGoingWay = onGoingWay; }

This isn't good! Way is a large class why are you not using references. This is what I would have expected to see:

 void SetOnGoingWay(const Way& onGoingWay)  
   {
     this->onGoingWay = onGoingWay;  
   }

So in the first, we have a local constructor (copy constructor) called to make onGoingWay and then an assignment operator called in the actual function to do this->onGoingWay=onGoingWay. The second is an assignment operator call. [Note: Most optimizers remove one of those calls]. But that means that your version of the copy constructor does not set id or name, but instead relies on the default std::string consturctor which gives an empty string.

Other problems:

While we are looking -- if AreaDataRepository is to be used as a singleton - then make the constructor, copy constructor and assignment operator private because as it stands you can circumvent your singleton requirement.

StuXYZ 731 Practically a Master Poster

Ok this is a standard top-tail queue. Let us walk through the process of putting objects into the queue and taking them off again to see how it works:

The important variabes are front/back.

Empty Case

Let us first conside the test case that you build an empty queue: front=back=0, if get is called the assert fails and error messages is produced.

One Element case

Next case: we add one elements [address of new ListNode N1] : we do that via put. front = back = N1 . back does not have its next ptr set so we will assume it is left as zero.

Now let us remove that object: get correctly applied that result is a COPY of the memory given in put [this is necessary because we are about to delete that memory record -- there are other ways of doing it.]
tmp is set to zero -- back->next was not set for one element.
front is deleted [there was only one object] front/back =0 which means
we have returned to the empty queue state.

Two+ Element case

Now let us add two (or more elements). Start from the state after adding one element, back is NOT zero, so back->next is set to N2. But careful, back is the reasined to N2 afterwards at the point of setting back->next to N2,back is actually pointing to N1 !! . So we have

   N1->next   : N2
   N2->next   : 0
   back       : N2
   from       : …
StuXYZ 731 Practically a Master Poster

Actually, I would much prefer that you wrote line 19/20 as:

std::vector<CBoard *> Board;
std::vector<CPlayers *> Players;

because what is the point of having namespaces and then just importing everything.

Also putting using namespace std; in at header file!!! Somewhere later, you are just going to import that into your code and make a complete mess because you will have then polluted the global namespace will all of std without knowing it. Errors like that take for ever to debug.

Secondly, you are using pointers in a vector, that is not a problem (assuming you are careful), but you do not have a constructor, copy constructor or an assignment operator. All three are auto generated by the compiler, and the last two are 99.9% likely to be very very wrong and will cause your code to crash in very unexpected ways.

thines01 commented: Solid advice +12
StuXYZ 731 Practically a Master Poster

Not sure that you know what you are doing here. The main part of the program will not work at all. Simply delete it from line 2 to line 14. It is utter junk.

First off: Provide a default constructor/copy constructor / assignment operator and deletion operator. I know that the deletion operator isn't going to have anything in it but but you will most likely want something in it later.

Next: Get that working in a real compilable/runable program. Create a polynomial and copy it to anther. I know it does nothing yet but it really really helps to do it in small steps.

Next: Think about what is meant by adding one polynomial coefficient e.g. 6x^8, do you want to expand your vector to size 9, and have 8 zero coefficients, or do you want to keep track of the powers. The write a test program that works with constants in the main program. At that point you will be beginning to get a feel for how it works.

Next do : output/operator+ and operator-. They are simpler than the rest.

Then tackle the more difficult * operator. That is difficult because if you multiply say (x^3+x^2) by (x^2+2) you get a polynomial with powers 5,4,3 and 2. The increase in the number of terms is a little tricky.

StuXYZ 731 Practically a Master Poster

The most scary thing about the above code is the pow(double(-1),j+1) ...
please think, an alternating pattern is required.

I initial thought that it was homework help, but it is actually nicely formatted obstifacted C++. So am not sure that the code deserves the -1 reputation. :)

WaltP commented: Try Obfuscated ;) +17
StuXYZ 731 Practically a Master Poster

One of the first real fortran program every written was about the gamma function, and in typical mathematical notation it evaluated [tex]\sum_{i=1}^{6} \frac{\gamma_i}{1+\lambda_i \tau}[/tex] ina typical math function. Unfortunately I haven't seen the original code for it. But the use if i,j,... etc is extremely common for subscript in maths notation, so given that it was mathematicians that were the originators of computing, their notation become standard were it is applicable.

However, the first fortran compile manual [IBM 704] (published in 1956), uses i as a loop variable in the very first program (finding the largest number in a set).

StuXYZ 731 Practically a Master Poster

Minor comment to the above. You said

I know that constructors go into the .h file

Fortunately, that isn't true.

A lot of beginners (with C++/C) seem to think that there is some magic about .h and .cxx or .cpp files. There isn't. The compiler just (effectively) makes a single file out of all the #include "file" and the file that is being compiled. If you like you can declare all of your classes in .cpp files and your code in .junk files, it doesn't matter, it is just a convention.

Once you are aware of that, you only have to consider that the compiler tries to have the minimum information that can allow it to compile each file to make an object file, and that it is doing in top-to-bottom. The after object files have been made, the linker needs everything.

In practice that means that if you want to specify any function of a class, you need 100% of the class's declaration, and 100% of the declaration any other classed that you want to use, but 0% of any definition.

Example:

// this Is the DECLARATION 
class A
{
   private:
      // stuff..
   public: 
      void method();
};
// THIS CODE MUST BE BELOW the declaration either in the same file or by an include:
void A::method()
{
   // do stuff
}

The example is just what you need when you compile, the includes are a quick way to help you not have to copy …

StuXYZ 731 Practically a Master Poster

The error is in the function isanagrams. If you turn on the warnings on the compiler.
[No I don't know how to do it on Visual studio.] You will see that you are told that
fro that funciton: warning: control reaches end of non-void function.

The problem is that the value is the undefined, so you many get a true even if the strcmp(a,b) is not equal to 0.

Try changing the conditional to this: return (strcmp(a,b)==0); Additionally, you read the dictionary words into a VERY short character array (20 characters). I made the mistake of testing the program with a typical English dictionary file, and core dumped the program, because the longest words in the english dictionary are over 20 letters long. It is often important to be very careful about reading strings directly in to character arrays without checks on the maximum number of character that are allowed to be read.

StuXYZ 731 Practically a Master Poster

Several problems exist.

First if you reference an array out of bounds you are going to get a mess.

Consider your code:

for (int i=0;i<V;i++)
    {
      for (int j=0;j<H;j++)
	{
	  ne=0;
	  if ( A[i][j-1]=='*' ) { ne+= 1; }
// .....

What happens as j is zero?? Similarly what happens when i is V-1 or j is H-1. You are then accessing unknown memory locations.


The solution to your problem is the define char A to be char A[V+2][H+2]; . Then set the boundary to be dead and then work from 1 to V and 1 to H.


Next Error:

You update the map on a cell by cell basis. The game is normally set out in a form by which the state of the game at a point in time is used to calculated the change, and then the whole change is implemented at once across the whole map. This can be easily carried out by having two arrays and switching between them each move.

StuXYZ 731 Practically a Master Poster

The main problem is that you have no idea what is going on because you have not put print statements std::cout<<"I am here"<<std::endl; in your code.

Next you have a character string that you call string. You actually pass the pointer to the first character. In a typical c-style character string the last character is zero.

Now how do you access those characters, that you have to do by addressing it by one of several methods: Let us do a quick summary:

You can use array addressing, e.g.

char* Str="this is some string";
std::cout<<"The  fourth letter is : "<<Str[3]<<std::endl;

Note that arrays are offset from zero, so Str[3] refers to the forth character.

you might decide to access them by doing a pointer offset. See that Str [in the example above] points to the first character in the string. To see that you can do this:

char* Str="this is some string";
std::cout<<"The  first letter is : "<<*Str<<std::endl;
std::cout<<"The memory location of that letter is : "<<Str<<std::endl;

Not that the * operator is used [it is not multiply here... another confusing issue sorry], to indicate that the contents of the memory location are required.

Therefore you can do this:

char* Str="this is some string";
std::cout<<"The  forth letter is : "<<*(Str+3)<<std::endl;
std::cout<<"The memory location of that letter is : "<<Str+3<<std::endl;

So you have, effectively moved 4 characters further on, which is effectively the forth character.

Now, in your loop you take the address …

StuXYZ 731 Practically a Master Poster

It is not strange that it runs, however, the problem is that exitMinutes is uninitialized.

That means that when the value in exitMinutes is read, it could be ANYTHING. To see how that came about, consider your program.

In the main program you write int exitMinutes; which creates a variable called exitMinutes BUT does not actually set its value. So it is whatever that memory location previously had in it.

Then you have this:

void inputAndValidate(int entranceHourP, int entranceMinutesP, 
                       int exitHourP, int exitMinutesP)
{
	do
	{
	cout << "Enter the hour and minutes the parking garage was entered (each separated by a space): " << endl;
	cin >> entranceHourP >> entranceMinutesP;
	}
	while((entranceHourP > 0) || (entranceHourP < 24) && ((entranceMinutesP > 0) || (entranceMinutesP < 59)));

Now under NORMAL operation, you pass a value to the function and then set the values before you test it. Fine, BUT what happens when you fail to enter numbers into cin, but instead, enter letters (for example).

The line cin>>entranceHourP >> entranceMinutesP; exits WITHOUT setting entranceMinutesP and as std::cin sets its own error state. THEN you use it uninitialized in the while loop. Hence the compiler warning/error.

Finally, the values you pass to the function are completely unused, you could just as easily write void inputAndValidate() and define your variables below.

StuXYZ 731 Practically a Master Poster

In addition to gerard143's solution, which is fine. You may want to make getitsvalue virtual.

This is problem is a specialization of the common construction:

class Base
{
   public:
    // Stuff... 
    virtual void write(std::ostream&) const;   
};

class Derived : public Base 
{
   public:
   virtual void write(std::ostream&) const;
};

std::ostream& operator<<(std::ostream& Out,const Base& A) 
{
   return Out<<A;
}

I only really bring that up since it is so common, that you will see that construct in many different books and open-source programs. [Note the operator<< is sometime templated with all the different classes in the program.]

StuXYZ 731 Practically a Master Poster

There is a third part the equation here: To illustrate consider this:

class AClass
{
  public:
    AClass(const int,const int) { std::cout<<"Constructor"<<std::endl;}
    AClass(const AClass&) { std::cout<<"Copy Constructor called"<<std::endl; }
    AClass& operator=(const AClass&) 
       { std::cout<<"Assignment operator called"<<std::endl; 
         return *this; 
       } 
};

int main()
{
  
AClass Object(1,1);        // Object constructed with constructor 

Object = AClass(2,2);      // ASSIGNMENT operator called.

So it the case that you have given then the assignment operator is called.

I am assuming that you have written an assignment operator. If you haven't do so now and for EVER other class you have, and while you are at it the destructor. C++ writes you an assignment operator and desctructor and copy constructor if you don't provide one, BUT they are shallow copies and if your class has any kind of complex object you definately need to write your own. The Rule is:
If in 1% doubt about needing to write the constructors/destructors/assignment operator write all 4.

In addition, I see that TheClass is derived from Parent, but you don't call its constructor in your copy consstructor. That is almost certainly unclear if not wrong.

Finally, since somethings like m_NumberOfPixelsInRegion are not constant, you are most likely going to have to write this construct for the assignment operator

TheClass&
TheClass::operator=(const TheClass& A)
{
   if (this!=&A)
     {
        Parent::operator=(A);
        m_NumberOfPixelInRegion=A.m_NumberOfPixelsIRegon;
        // ... etc ...
     }
   return *this;
}
daviddoria commented: Thanks, that was exactly the problem! +5
StuXYZ 731 Practically a Master Poster

First welcome to Daniweb, second please use code tags! It make life easier for all of us.

The problem is possible with the use of srand(time(NULL)); . What you are doing is re-initializing the random number generator with a number and that number is from the time. HOWEVER, time(NULL) normally only gives an answer to the nearest millisecond, I guess that you are reinitializing the random number generator with the same seed number each so the first random number is always the same.

To cure the problem put srand(time(NULL)); or as easier for debugging,
write something like srand(53234231); but put in just after int main() and nowhere else.

Note: 53234231 is not special, it can be absolutely any number (except zero )

Hope that helps

StuXYZ 731 Practically a Master Poster

Without all the code to test this, we are just guessing. BUT: the for loop in the copy constructor looks horribly suspicious.

for (DLNode *sourcePtr = source.head->next->next;
	sourcePtr != tail;
	 sourcePtr = sourcePtr->next) 
{
  // ...
}

You see that tail is set to tail(new DLNode::DLNode(head, 0, NULL)) so despite the inserts it remains within the scope of the new list. Therefore,
sourcePtr which is within the realm of the old list cannot ever be equal to it.

This is just a guess. I don't feel I have enough of the code to evaluate it further. I hope this helps. [It is an easy one to put a test in for.]

StuXYZ 731 Practically a Master Poster

I expect that you wrote somthing like this: std::cout<<"result == "<<a<<std::endl; . The problem with that is the default precision, for output, is 6. The number itself is of a higher precision.
If you write this

#include <iostream>
#include  <iomanip>   
 
int main()
{
double a=1.2345678901234567890;

std::cout<<std::setprecision(20);
std::cout<<"results ="<<a<<std::endl;
// Not all numbers are now in precision 20.

The following code give you something like : 1.1234567890123456912
That is because [for my 64 bit machine -- sizeof(double) == 8 bytes] the precision of double is about 14/15 decimal places. [and I got lucky with the roundoff for the 16th ;) ]


You will get more decimal points and a bigger exponent range if you use long double. If you need a larger precision for something, then there is the gmp [gnu multiple-precision library] http://gmplib.org/, which is only limited by your computer memory and speed.

Overall I am normally very very disappointed with the way that formating is done in C++. I basically rely on the boost::format http://live.boost.org/doc/libs/1_39_0/libs/format/doc/format.htmlsystem which is much more printf like but with slightly more type checking, and slightly better error messages.

StuXYZ 731 Practically a Master Poster

Something that avoids all the graphical stuff, [Well use an external program - not part of the project] is a simulation. That can go from easy, to extremely complex. There are a number of classes of simulation, but it is easier to give an example, what about a set of stars in an galaxy. You obviously have [initially], very simple equations of motion. You can simulate just a few stars. You might like to simulate the expected constellations patterns in 50,000 years, or small scale like a simulation of our solar system.

Then you can take you project into a number of interesting directions, you can add relativity, you can add solar aging (mass of the start decreased), you can add the effect of a supernova, mass is ejected very fast, energy is ejected at speed of light. You might want to simulate a solar system, that again can get complex, with solar winds, thermal pressure, planet+solar spin, comets etc, or stay simple with just basic F=ma motion.

If you don't want to play with gravity, e.g. a pool simulation [game with 15 ball + white ball] etc, starts very very simple, gets very complex very quickly [spin,
table nap, cushions]

Almost every simulation of a system that has definate state, starts relatively simple but has underlying complexity, even the simple Newtonian star system, with three stars, is actually chaotic in many configurations, and can be enriched, until you have no more time, etc. Effort can …

StuXYZ 731 Practically a Master Poster

The first thing you need to do to fix this, is to realize that you are passing WHOLE strings and not individual characters:
Add this line like this:

while(iss >> str) 
    {
      std::cout<<"str::"<<str<<"::"<<std::endl;        // ADD THIS LINE
      //iss >> str;

That will show you that you have str that is a complete string, it is not individual characters. E.g. When you do the test str!="+" it will ALWAYS be true, since "4+3" is absolutely NOT "+". Thus your code simply think you have a single object, and the postfix and prefix versions of a single object/number are both the same, just the object.


When you put str to be an individual character (e.g. char). You got a whole pile of errors. THAT IS GOOD!!!! That is the compiler helping you fix your code that is simple wrong in many places.

So, in short, make str (a) a char , (b) a better name, that you are not going to mistype and find is a keyword. (c) fix all of the compiler errors and then try a little debugging, e.g. putting output statement and assert's etc.

Sorry that is a little harsh but that is the only way you are going to fix this code that I can see. When you have fixed the compiler errors, feel free to repost, I/others will happily help with the remaining runtime bugs.

Finally, about putting a qualifier on on the line T tmp = stack<T>::top(); . …

StuXYZ 731 Practically a Master Poster

First off welcome Daniweb.

Next, now that I have your code, what are the problems with it. Unfortunately, quite a few -- I have listed a few to get you going in the right direction.

The first is that you are using this code [lines 32-34]:

cin >> line;            // get a string into line up to the first space deliminter ] 
getline(cin, line);     // This reads the rest of the line 
istringstream iss(line);

That means that for the program to accept say 4*5 you have to enter: someJunk 4*5 . All the characters in the first expression up to the space ("someJunk") are thrown away.

So fix it by removing the cin>>line; Unfortunately, that isn't your only problem:

while(iss >> str) 
  {
    // Stuff...
  }

Since str is a string [not a char] then it assumes that you are entering you text like this: 5 * 4 + ( 6 - 32 ) .

The obvious way round that is to use a char. There are a lot of changes to that, you need to make the "+" into single character e.g. '+' .

Next you are using a very strange construct for handling the brackets

else if(str == ")") 
  {   //If the ) is encountered, pop the stack until the ( is found
    while(str != "(") 
      {
        // THIS LINE doesn't change the str so it is an infinite loop:
        // Until stack thows an exception.
	cout << stack.pop();
      }
StuXYZ 731 Practically a Master Poster

Obviously, it would be a lot easier to look at this if you had posted all the code.

However: Find_Trav_Subtree looks very strange [to me].

You see it calls the right branch if (current->Right) AND then always returns Find_Trav_Subtree from the right branch REGARDSLESS of it returning a NULL. I would have expected to see something like this:

BSTNodePtr Result(0);
// Note only return from right if a valid result is found:
if ( Current->Right && (Result = Find_Trav_SubTree(Current->Right,ID)) )
   return Result;
return ( Current->Left) ? Find_Trav_SubTree(Current->Left,ID) : 0;

There are many ways to do this.

Hope that helps, if not please post the full BSTClass so we can also compile and debug it.

StuXYZ 731 Practically a Master Poster

There are several mistakes in the above code. The main one is that you wrote the code in one long block without debugging each section. It is almost always easier to write the code in little bits, keeping a running program that does a little more each time.

Let us look at some of your common errors: Consider this code

while(i<phraseLength&&!letterFound)
   {
      if(nextLetter==phrase[i]);
      letterFound=true;
      i++;
   }

The first thing I did with your code was compile it with all the warnings on:
and I get several warnings and it tells me that I have an empty body in the if construction at 160.

The second thing I did, was reformat it with a code indenter. . That shows the problem. The if statement has an extra semi-colon so letterFound is ALWAYS true.


I am sure you can see your mistake now.


EDIT: theLamb beat me too it, sorry for the repeat, . But please compile with warnings and use an automatic code indenter. It really really helps show you were the errors are, just because things "look wrong".

thelamb commented: Put some good effort into this +3
StuXYZ 731 Practically a Master Poster

There is a nice tutorial at http://www.newty.de/fpt/index.html.

The ->* is a pointer dereference to a function pointer. Easiest way to google it is
to put it in quotes.

StuXYZ 731 Practically a Master Poster

On reading this I thought that the obvious solution is to use a functional.
The functional can be defined as a member of your class, or not.

Additionally, I wasn't actually sure what algorithm you were implementing, in this example I consider that you have an vector of values, and wish to find the positions of the lowest values without disturbing the vector.

consider if you did this:

class comp
{
  const std::vector<double>& RefVec;   

  comp(const std::vector<double>& A) : Ref(A) {}

  bool operator()(const int A,const int B) const
  {
    return (RefVec[A]<RefVec[B]);
  }
};

int main()
{
  std::vector<int> Index;
  std::vector<int> Out;
  std::vector<double> Value;
  
  for(int i=0;i<10;i++)
    {
      Value.push_back( ((i % 3)-1.0) * i*i);
      Index.push_back(10-i);
    }

  Out.resize(3);
  partial_sort_copy(Index.begin(),Index.end(),Out.begin(),
		    Out.end(),comp(Value));
 
  for(int i=0;i<3;i++)
    {
      std::cout<<"Lowest Values == "<<Value[Out[i]]<<std::endl;
    }
}

The function idea can be extended to include very complex ideas, since the functional has state.

Next: You can use member pointers to functions, but you need a calling function:
You just need a slightly different syntax. However, it wont help you (easily) here since you are going to have to do a lot of bind2nd etc to get it to work [or use boost::bind].

Note the comment the compiler gives you is correct, the key phase is address of an unqualified which is because you missed out the className:: part of the qualification.

class X
{
  public:
  // Two functions with same signature [ void () const ]
  void call() const { std::cout<<"CALL"<<std::endl; }
  void call2() const …
StuXYZ 731 Practically a Master Poster

Sorry I am not impressed with your teacher.

Most compilers of c++ and a lot of other compiled language split the program up into individual files. Each of those files are normally compiled into object files.
These normally have the .o extension. They are the machine code of each block of code. However, in many of these .cpp file, there is a call to a function that is declared (normally in a .h file), but not defined (the actual code for a function, in a .cpp file. The object file contains a symbolic link to that function but not a direct call. These is resolved by the linker that takes all the .o files and tries to resolve all the symbolic links to produce a single executable.

Normally the production of an object file is done first, and then the object files are linked together. The commands given would be

g++ -c Client.cpp
g++ -c Server.cpp
// All the other .cpp files etc....

The you would link it with the command g++ Server.o Client.o other.o and that would give you an executable called a.out. [if you used the -o option you can change that name]

However, for short programs, there is a quick way, just give g++ the list of files to compile and it will compile and link them .

Your command line was : g++ hw2.cpp

So unfortunately the linker says that it cannot link the files because it doesn't know …

thelamb commented: Thoroughly explained (Y) +3
StuXYZ 731 Practically a Master Poster

Ok the class is a policy class. So let us understand what you can do with the original code and your code.

First consider two classes:

class SimpleObj
{
  // Stuff..
  public:
    SimpleObj();  // Simple default constructor
};

class ComplexState
{
  public:
    ComplexState(const int,const std::string&);   // THE ONLY CONSTRUCTOR
};

Now with your code, you cannot use ComplexState, there is not default constructor.

However, with the original code, let me add this:

class ComplexBuilder
{
public:

  // No need for template as it is specialized:
  static void create( ComplexState*& ptr ) 
  {  
    ptr = new T(3,"testfile");
  }
};

Now I can use your class like this:

MakeT<ComplexState,ComplexBuilder>::instance()->doStuff();

The reason for a reference, to a pointer, is that the pointer value itself changes.

If I have mis-understood your problem, or I am not clear, feel free to add a reply.

mike_2000_17 commented: bullseye! +1
StuXYZ 731 Practically a Master Poster

Your problem is that you have masked length. You wrote this:

#
void file(char* Filename, int length)
{
  int length;   // UNINITIALIZED VARIABLE:
  for(int i=0;i<length;i++)
    {
    }
}

Note that length is NOT the same length as the one passed to the function, you have masked it with the later declaration. Any good compiler with the warnings on will tell you about that. (unused variable, and uninitialized variable warnings).

Also your test of the file eof is ONLY for the first element, why not at each stage.

Finally, don't use the keyword register. It is ignored by most compilers, and unless you are mixing assembler and C++, the chances of you actually betting the compilers optimizer are very very low.

jonsca commented: Good points! +5
StuXYZ 731 Practically a Master Poster

In this particular case, it seems to be that you are getting the exception from the line if (data[i].at(it->key) == check_word_key) . That is because it->key DOES NOT correspond to the size of the vector data.

This occurs because data is not a vector of size TABLE_SIZE but is of the number inserted. E.g. you insert keys 34, 56 and 78. Now you have three vectors with one component each. THEN you access from key 34, and you do data[34].at(34). Opps error.
Since data[34] only has one component.

To see this, modify your lines:

std::cout << "iteration through FOR loop" << std::endl;
std::cout<<"Size of data["<<index<<"]=="<<data[index].size()<<std::endl;
std::cout<<"Accessing "<<it->key<<std::endl;
StuXYZ 731 Practically a Master Poster

You have made the same mistake as the original [in a different place].

You bool operator does not know anything about the vector or the type, that is because it actually is the type contained in the vector:

it should be this:

bool operator==(const int& Index) const   // Get the value to be compared into index
{
  return (key==Index);
}

You have completely confused yourself by using the variable name key in just about every possible location in your code, thus it is a scope understanding master-class to figure out what is going on. Try this:

template <class RecordType>
bool table<RecordType>::is_present(int possibleTarget) const
{
  std::size_t i = hash(possibleTarget);  

  typename std::vector<RecordType>::const_iterator it;

  for (it = data[i].begin(); it != data[i].end(); ++i)
     {
       if (data[i].at(it->key) == possibleTarget)
          return true;
     }
  return false;
}

Finally, get hash() to return a reference to a data object and not an index value, you can use an empty vector for a "not found". [When you have it working!!!]

StuXYZ 731 Practically a Master Poster

you have two problems with your code:

(a) missing typename on the in the template file.

std::vector<RecordType>::const_iterator it;  // THIS IS WRONG

// This is correct:
typename std::vector<RecordType>::const_iterator it;

There are many explanations around for this, it basically is because since depending on what std::vector<RecordType> is specialized as, you can't figure out what const_iterator will be. [It might resolve to a value for example].

Next you have a small issue in resolving the repeating name issue and
in precedence order:

if (data[i].at(*it.key)  == key)  // THIS IS wRONG
// This is better [precedence order correct]
if (data[i].at(it->key)  == key)

That will leave you with one function to add, and that is a bool word_record::operator==(const int&) const; since you are comparing a RecordType with an int on the line 15 of the code.
you might like to write the completely unintelligible if (data[i].at(it->key).key == key) but then I don't think that is a good idea.

Additionally , you need to provide implementations of table::table and table::insert().

StuXYZ 731 Practically a Master Poster

It is a bit more complex than that. You have made a whole set of basic errors, and I think you have to fix all of them to get yourself out of jail here.

Let me start: First off you have TWO constructors, (a) the default constructor and (b) a valued constructor. The first is ok, [but ugly] and the second is junk.

The second is written like this:

Employee(std::string name, int number, int date)
    { set(name, number, date); 

void 
Employee::set(string name, int number, int date)
{ 
}

So obviously nothing gets set.

Next YOU DO NOT HAVE A copy constructor OR an assignment operator. If you have a non-POD [Plain Old Data] class, and in this case std::string is NOT a plain data object, ALWAYS write those two methods, unless you are into that super guru c++ class, which I can only aspire to.

This does not actually affect you here BUT it will as soon as you do anything else.

If you fix set, then this program works.

However, I don't understand why you get a ProdcutionWorker, and then create an Employee. I think that you wanted a production worker. so why not do a

// Remove this: 
// Employee info(name,number,date);
// and change it to this:
stats.set(name,number,date);
// change all instances of info to stats in the output.
StuXYZ 731 Practically a Master Poster

You unfortunately have several problems: So i am going to give you a quick summary of the problem and why I think you have made the mistake.

(i) In the constructor fraction::fraction(), you have written integral, numerator, denominator = 0; This DOES NOT do what you think, it does NOT set integral or numerator. If you wanted to do that then you should have done integral=numerator=denominator=0; . Much much better would have been to use an initializer list e.g. fraction::fraction() : integral(0),numerator(0),denominator(0) {} .
Note: Almost all modern compilers would have given you a warning about that if you had turned your warnings on. No beginner should ever compile without warnings on.

(ii) Input is not actually wrong but so ugly, surely improper fractions would be ok?
What about reading a line of input and then splitting it based on e.g. 4/5 or 3 4/5

(iii) friend fraction operator+ Never make something a friend that doesn't need to be. This just doesn't need to be a friend. It should also have this form: fraction operator+(const fraction&) const; Then it is (a) more efficient (b) much less likely to go wrong.

(iv) gcd and gcdr should be one function and not call each other backwards and forwards, or gcd_prep should call gcd (which would be the recursive function) once only [to allow the test for negative to the outside.]

(v) operator+ is written in such a way that I think you did not really …

StuXYZ 731 Practically a Master Poster

Absolutely fine, (IMHO), obviously it depends on your needs, but what about classes that return some kind of evaluation, e.g. a class that calculates the global density from all the objects in a simulation. It knows the materials, object locations and orientations. It has only one public function that returns something and that is the calcDensity function.

StuXYZ 731 Practically a Master Poster

As Vernon says, you are very lucky that line 40 and 46 compile. HOWEVER: if they do, the (which in your case they obviously do). The actual error is this: cout << letterlist [3][0]; .

That is because possible is an integer an actually over flows!!

for (int n = dicerolls; n>0; n--)
    {
        table [dicerolls - n] = possible;
        possible = possible * 2;
        std::cout<<"possible == "<<possible<<std::endl;
    }

Look at the output from this modification.

Integers only have a certain number of bit, e.g. 16. 1 is used for the sign and the other 15 are for the value, hence why it crashes after diceroll > 15.

StuXYZ 731 Practically a Master Poster

Ok the first and most important problem is simple that you have gone about this task in the wrong way. Start with the assumption that the compiler is your friend not your enemy, it is there to help, but if you write your whole program and then try to compile it you will have a very very hard time figuring out what you have done wrong.

So what you should have done it written the smallest compilable program that you can write and compiled, then fix the problems and then add a little more (basically in chunks of approximately one error per compile -- that changes with experience and later you can separate the compiler errors, you can write more, but initially that means no more than one method at a time).

So let us look at your code and figure out some of the types of error you are making.

First off, is that you seem to have no care for the capitalization of variables double avar; is different from double Avar; .
You consistently use random first letter capitalization.

Second you do this:

class A
{ 
   int hr;
   A(int);
};

// THIS IS WRONG:
A::A(int hr)    // THIS mask the class variable
{
   hour=hr;     // hour doesn't even exist anywhere
}

You should have written

A::A(int hour)
{
   hr=hour;
}

Word of caution, you write 06 and that is an octal number, i.e. 08 is illegal because there is no 8 …

anglwthnati2de commented: If only our Professors would walk us through this like you did, you wouldn't have people like me bothering you! (LOL) +0
StuXYZ 731 Practically a Master Poster

What doesn't work with this:

int
main()
{
  VStack<double> A;
  A.push(10.0);
  std::cout<<"A =="<<A.size()<<std::endl;
}

You remember that you have to use a template type in declaring VStack and that you might need to explicitly initialize the template if VStack and Stack are in separate files e.g. Add

template class Stack<double>;
template class VStack<double>;

HOWEVER: you may have made a complete mess, because you seem to have using namespace std; in your code, your names stack, etc are very close to the std names and you could easily be having real problems there.

StuXYZ 731 Practically a Master Poster

This seems a lot like you have coded before you have figured out what you are going to do. So let me point out a number of strange things that you may have overlooked.

First you initialize the board to 1,2,3, etc... e.g.

for(int y=0; y<n; y++)
    for(int x=0; x<n; x++)
      game_board[x][y] = value++;

Surely at the start of a game you are going to use say 0 to represent no counter, -1 for black and 1 for white. So you want to initialize the board to zero. By setting each point to a value, that is only used when you write out the board [and makes it almost impossible for the user to see the real game], you have made your life very very difficult.

Now you ask the poor used to enter the number based on the total position on the board. That is ok, but surely the coordinates would be better.

Now to calculate if you have a line of 5. Well first thing is you ONLY need to check
each of the positions from the point that you entered a new counter. Therefore,
you only have to check (at most) 8 directions. However, you do have to note that
a line can be formed up to 4 additional counters away from the point.

So the obvious way is to check from the point with a continuous addition: I will demonstrate the idea with a horrizontal forward direction line, …

jonsca commented: Nice one +5
StuXYZ 731 Practically a Master Poster

It seems like it is actually some other part of your code now. You could run your code in the debugger. If you can write a simple test case for array2d, then post the whole code here. [At the moment, we have nothing that we can actually run.]


You can also put a simple try { // your code here } catch (std::exception& A) { std::cout<<"Exception == "<<A.what()<<std::endl; } , around the code in int main(). [Assuming that you are not using throw in your array2d code.]

StuXYZ 731 Practically a Master Poster

As arkoenig said, as you have written it, it doesn't compile.

If you are using the construct operator new, you need to do it like this:

class X
{
  X() { std::cout<<"Constructor called "<<std::endl; }
  ~X() { std::cout<<"Destructor called "<<std::endl; }
};

int main() 
{
  X *x=new X;
  X *y= static_cast<X*>(operator new (sizeof(X)));
  delete x;
  delete y;
}

BUT be very careful: the constructor is only called ONCE. ie only for x, not for y. Be very very careful calling it, because if you don't have a POD [plain old data] type class, you are opening yourself to a nightmare of runtime problems.