I would worry about line 31 : for (int I = 10; I >= 0; I--)
When you read in the data
for (int I = 1; I <= 10; I++)
{
cout << I << ")"; cin >> N[I]; cout << "\n";
}You populate the value N[1], N[2]... etc. BUT you do not populate the value N[0] that is never set. But in line 31 you loop over I until and including I == 0. Thus the value printed will be undefined (i.e. anything!)
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.
Doesn't matter which method you use -- pointers and new (remembering the delete when you have finished with the array) is perfectly fine, however, so is declaring three arrays of size 20 and filling only the first part of each.
Note you need to check the user input isn't bigger than 20 before you do anything with the arrays. Also be careful about the user being "clever" and entering -1 elements or such like.
Finally, post some code if you get stuck.
Tiny typo --
Line 38 : s.setval(5.1,0);
Should have beens1.setval(5.1,0);
There are ways round this kind of error (scoping the two test etc) but in reality better naming is more important. I hate any name that can be confused with another name by one character missing/changed etc. However I guess you wrote this as a test code and didn't worry about it -- we all have done that!
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.
The problem here is ownership and responsibility.
The aggregate class would normally take ownership of the object being passed to it. [Sometimes that aggregate class would build the object via a factory object -- but let us keep this simple].
So in words this is what I think you want to happen:
(a) allocate memory for an new object [e.g. int* p=new int[10];]
(b) Populate the object : [e.g. for(int i=0;i<10;i++) p[i]=i; ]
(c) give the object to the aggregate class AND let it take ownership [e.g.aggregateClass obj(b,p) : ; ]
(d) when objB goes out of scope the destructor of bar is called.
[e.g. aggregateClass:~aggregateClass() { delete b; delete [] p; } ]
The important think is one delete per new, and the aggregate class deletes the memory it is given to manage.
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!)
You seem to be asking a lot of questions about developing a C++ 3D geometry system of some sort. I think it would be a lot better to show us the framework you have built, and the problem domain. For example, the answer to this questions needs us to understand how your 3d vector space is implemented. If it is spherical coordinate system, then all you do is set r equal to 1.
If you show us were you are at we can help.
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 …
The chances are that your error is actually the accuracy of the return value from the dot product.
With floating point calculations you often get a little bit of an error, so you might be returing -1.0000000001 from your vector dot product. The quickest way to check that is to simply do
std::cout<<"Should be zero: "<<Point1.dot(Point2)+1.0<<std::endl;
If you see a very very small negative number then that is why acos will return NaN. There are ways round this either simple tests (e.g. checing the number is >1.0 or <-1.0) or by using mathermatical identies that force the number into range regardless of the floating point roundoff error.
[Note: in this case I think that if A and B are unit vectors +/- roundoff error then (A.dot(B)/(A.dot(A)*B.dot(B)) is bound within +/-1.0 -- but I am not 100% and you should check this before you rely on it -- I just wanted to show the kind of things I was talking about]
The problem here is (a) the typo error with write *num1= num2; rather than *num1=*num2; (which doesn't actually affect the problem). And (b) what you are defining by const.
Let us start by looking at the definition of limit:
const int limit=100;
That says limit is 100, and the compile [except in very special circumstances] will try to keep limit equal to 100 at all times, so you, the programmer have less difficulty in following your own (or someone elses program).
Then you write this:
void passingAddressOfConstants(const int* num1 , int* num2)
{
// Stuff [doesn't matter what]
}
This is ok BUT you have told the compiler that the address pointed
to by the variable num1 will be constant. Thus calling it with
passingAddressOfConstants(&limit,&limit) is 100% not ok, because you are allowed to change the second variable, which implies that limit will be changed.
However, the function is also illegal (simplified to):
void passingAddressOfConstants(const int* num1 , int* num2)
{
*num1=20; // compile error
}
you promissed that the value pointed to by num1 will not change, and then you try to change it!
However, even if it is not possible to guess what is going to happen at run time this is legal:
void passingAddressOfConstants(const int* num1 , int* num2)
{
int sum(0);
std::cout<<"ADDRESS of num1 : "<<num1<<std::endl;
for(int i=0;i<10;i++)
sum+= *num1++
std::cout<<"ADDRESS of num1 "<<num1<<std::endl;
std::cout<<"Sum : "<<sum<<std::endl;
}
Don't expect a pretty …
NullPtr is 100% correct -- just a litle brief I think!
Your question is what is wrong with this code. So as was pointed out the error lies in the line
temp >> num[i][j];
Let us consider a similar expression:
int a=5;
int t=2;
int x = t >> a;
std::cout<<"X == "<<x<<std::endl;
Now what I have done here is use >> in its ORIGINAL form (i.e. what C++ took from C) Here the >> operator is used as a right shift operator. That is if we take the binary representation of 5 : 000101
and shift the bits to the right, discarding any bit that get pushed off the end of the number, and do that work twice (because t=2) twice we get 1.
consider the operation as
000101 -> 00010 -> 00001
Now, before we go on -- hopefully you remember/or will find out soon, that any class object [which is going to act as a complex data store with function that apply to it] can have MOST operators overloaded. We could write classes like this
class number
{
int value;
public:
number(int N) : value(N);
//..
// rather strange operator overload !!
int operator*(const number& A) { return value+A.value; }
};
int main()
{
number X(5);
number Y(7);
// slightly strange output
std::cout<<"Output == "<<X*Y<<std::endl;
}
In this gives Output == 12 !!! Note that I have overloaded the multiply operator and I can do …
I am really guessing here because you don't tell us what you expected and what you got. But maybe you got no output -- if that is the case was it because you forgot to put a std::endl in your code e.g. add line 25 :
cout<<endl;
The algorithm is a suffle down deletion, i.e if you have a set of number : 11 12 13 14 15, and you want to delete the number in the second position (12). you want to modify the array to be :
11 13 14 15
BUT not that because you have reduced the size of the array by one, the element in position 5 (i.e LA[4]) can be anything and it this case your output would have been
11 13 14 15 15
That is because in you code you wrote
for(int P=0;P<=4;P++)
cout<<" "<<LA[P];
However better might have been:
for(int P=0;P<N;P++)
cout<<" "<<LA[P];
If that doesn't cover the problem, tell us what you got and what you expected and we will try to help.
Nothing complex just a typo/minor miss-understanding.
You wrote this:
property <string> caption{std::bind(&tooltip::GetCaption, *this),std::bind(&tooltip::SetCaption, *this, std::placeholders::_1)};
Note the *this , that should be just : this. You are trying to binding a class method pointer to a class instance pointer. If the class instance pointer is no such thing then things go wrong. You can use a reference but you convert it with std::ref( X ) which is a bit odd looking if you already have the pointer this!
If we take your orginal protery class and add two methods:
std::string Get() const { return getf(); }
void Set(const std::string& Input) { setf(Input); }
We can write:
class testCase
{
std::string Hold;
public:
testCase() : Hold("WRONG") {}
void setCaption(const std::string& Out)
{ std::cout<<"SetCaption "<<Out<<std::endl;
Hold=Out; }
std::string getCaption() const { return Hold; }
void build() {
property <std::string> captionInner
(std::bind(&testCase::getCaption,this),
std::bind(&testCase::setCaption,this,std::placeholders::_1));
captionInner.Set("gamma");
std::cout<<captionInner.Out()<<std::endl;
return; }
};
int main()
{
testCase A;
A.build();
}
Not that you can try with *this and see the wrong result -- however I believe this is undefined behaviour -- no 100% sure.
Fundermentally, the loops are highly inefficient.
You have the condition that a+b+c == 1000, so why not loop on a and b BUT calcuate c.
Second you can limit the loop extent based on the idea that c>b>a.
Thus a can never go beyond 333 (1000/3) and b extends from a+1
to 1000-3*a.
This would leave you with two loops only.
// THIS is a long way off efficient.
for(int a=1;a<333;a++) // b>a and a>c thus (3*a)<1000
for(int b=a+1;b<1000-3*a;b++)
{
int c=1000-a-b;
if (a*a+b*b==c*c)
return SUCCESS;
}
return FAILED;
You have three mistakes:
(a) Your output is outside of the loop, you need to output each time that you get a new letter.
(b) you don't output the letter just the number.
(b) you don't deal with the special case of temp=1, i.e. you don't need to output the number.
That should help .. let us know if that you are still stuck.
No you don't HAVE to add const, but either you should because that is what you intended [and the compiler can make you faster machine code] or it is not what you intended hence it is a bug.
However, you don't have to add const and references [note: const image& is not an address operator].
You are highly likely to get memory errors. You mix C++ style constructors with C style malloc. You have a line like this:
e.g.
img=new Image(towstring(filename).c_str());
free(pDimensionIDs);
Now when is img deleted??
There is another problem, img is not initialized in the default constructor, so you are going to call img->Clone() without the object. That is certainly going to cause problems. Initialize img to zero and then if you do make the call, you get a seg-fault at the location you messed up.
Also in the operator=(const image& cSource), you have the same line...
img=cSource.img->Clone();
Now three things (a) img might have a value already, so you have memory leak. (b) cSource.img may be nullptr so what happens (c) if you have the equivilent to A=A, then you have undefined behaviour
Also I noticed this:
if(intSelectFrame<0)
intSelectFrame=0;
else if (intSelectFrame>=framecount)
intSelectFrame=framecount-1;
intSelectFrame=selectframe;
Obviously that is the same as just
intSelectFrame=selectframe;
Your problem as posted still needs the code that actually called Picture for us to figure out.
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 …
The principle of this type of hash map is that when two objects have the same key they are stored in the hashmap using a linked list. i.e
if you store say 12345 and 23456 at key number 7, then both will be stored at location 7.
Now if it was put to a different location if then (a) it would not be findable under the initial key (b) what would you do when you have more than 128 (your array limit) ?
The way this was coded effectively is like someone has an old address book with each page has a letter e.g. A , B , C etc. Then as a person is added it gets added directly under the correct letter but in no particular order, e.g. under S it might be Smith, Simon, Steve.
When you are looking for Steve's address, you have to look at all the
S entries until you find him, but don't have to look at A, B ,C etc.
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.
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 …
That is a LOT of if statements for a nice output. Acutally there is a quicker way to do it [ assuming that you don't want to use printf or boost format etc ]
#include <iomanip>
// other stuff...
cout<<setfill('0');
cout<<setw(2)<<hour<":"<<setw(2)<<min<<endl;
cout<<setfill(' ');
What that does is set the fill character to be 'zero', normally it would be a space. Then it forces the output to occupy two characters.
Thus if the output is going to be 1 it will be written as 01, but if the output is going to be 12, then no problem it will be written as '12'.
You will see that I have unset the fill character on the next line as it is persistant. [and if you forget very annoying!]
Also please don't put using namespace std; in include files [I actually prefer not to ever put it], as you will include that file in someother file and then find hundreds of names that are in namespace std have just come into your global space, and when you accidentally mistype a variable etc and find that you have used one of the std names, the compiler error is a nightmare.
Second, you don't need to include "Time.cpp" in your main program, just compile and link to it.
It is unlikely that you really want a switch statement. The problem is basically a set of decision based on two different criteria: age/gender. That ends up as a two state switch statement embedded in another two state switch statement... uck!!
What is wrong with
float membershipFee;
if (age < 12)
{
if (Gender == 'm')
membershipFee=XXX;
else
membershipFee=YYY;
}
else if (age < 18)
{
if (Gender == 'm')
membershipFee=ZZZ;
else
membershipFee=AAA;
}
else
{
// etc......
}
// ....
std::cout<<"Membership fee = "<<memberShipFee<<std::endl;
Obviously you have to replace XXX, YYY by your actual constants and complete the if else statment.
If you are comfortable with this construct you might like to replace the inner if statement if (Gender=='m') part with
membershipFee = (Gender=='m') ? XXX : YYY;
but if that is not familiar to you, don't worry it is just a short hand for the inner if statement and you can ignore that and you the slightly longer form.
I think you have made another mistake: Consider the line of code
f.write((char *)&array[i][j],sizeof(array));
Now for your test example you have an array of [3][3], which if we assume you have 4 byte integers: sizeof(array) evaluates to 36.
Ok so you then take each point on the array in turn and write out 36
characters (9 integers). I am certain that is not what you wanted.
Try without the loop:
f.write(reinterpret_cast<char *>(array),sizeof(array));
Same is true for your read statement.
Have you looked to actually see what you have done with memory allocations. It seems that you have the following :
rol=(struct roll*)malloc(t1*sizeof(struct roll));
yr=(struct year*)malloc(t2*sizeof(struct year));
mn=(struct month*)malloc(t3*sizeof(struct month));
da=(struct day*)malloc(t4*sizeof(struct day));
But then given that month has the structure :
struct month
{
day* da;
};
However, at not point do you make da point to anything. That the program doesn't seg.fault is chance I think.
You have two problems, (i) you allocate say 12 months of memory,
BUT only allocate 31 days of memory -- that does for January but
nothing for February etc. (ii) You don't make da point to anything,
it is just a floating pointer.
What you need is to put some code after your massive memory allocations, which sets the pointers to point to something e.g.
int mIndex;
mn=(struct month*) malloc(12 * sizeof(struct month));
for(mIndex=0;mIndex<12;mIndex++)
{
mn[mIndex]=(struct day*) malloc(31 * sizeof(struct day));
}
Finally, I don't know what this code does but this cannot be a good way to do it. I think a re-think is required. Start small, and test litte bits and get them working, e.g just try a single day first, then try a month etc.
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.
The problem with this code is firstly that it just doesn't work, but much more problematic it shows some little mis-understandings in the basics of type conversion.
First of consider types: By this I mean the storage for a variable or constant. Examples are int (an integer type), or
double (a floating point number). Those two examples are simple types. C++ allows you to construct your own types and use previously constructed types that you load via libraries. One very common library is the standard template library, which you have loaded part to give you string. It is also why you wrote using namespace std; at the top of your code.
HOWEVER, in the middle of your code you start using char array, (char data[50];) to store 50 char (small unsigned integers really). The try to either compare (because the line data==secret_word; actually evaluates if data is equal to secret_word, BUT you can't do data=secret_word; because data is a pointer to the first memory location in the array of char.
There are ways to convert data, but although all basic types can be converted directly, e.g. (int a; int b(50); a=b;) arrays normally need a more sophisticated copy e.g. you CANNOT do int a[5]; int b[5]={3,4,5,6,7}; a=b; and expect it to work as expected.
To come back to your code, you are using string, and I would have used the string class. e.g.
std::cout<<"Number of letters == "<<secret_word.size()<<std::endl;
Additionally, I would think you should make use of …
Sorry but you have steped into one of those nasty legacy areas from C.
There exists a function [in stdlilb.h which is included via cstdlib] long int random(). Thus the first of your errors.
To avoid this either (a) rename your functions e.g. random100 or something (b) give it an argument (e.g. random(const int N) (c) put it in a namespace.
Then we have the scary stuff:
You write
for (int i = 0; i < ArraySize + 1;i++)
{
array[i] = randomInt();
}
Hang on a second!! you only allocated ArraySize items for the array.... so now you are filling ArraySize+1 items. That is not good, and is corrupting memory!
Finally!!
Please stop putting using namespace std; at the top of your code!! Global things like random() left over from C, have been (slightly) mitigated by putting many things into the namespace std. So you have just pulled in about 2500 different names. Each one is waiting to cause a difficult to find bug. So if you want to use an item from namespace std. Either
(a) write stuff like this std::cout<<"This is some output"<<std::endl;
(b) Just get the bit you need e.g. put this code at the topusing std::cout; Obviously you need to add everything you are using e.g. endl etc...
(c) Put your using namespace std; in just small scopes (e.g. the function/method.
I prefer (a) over (b) over (c) ...but many will have a different order.
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 …
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 …
I think the code should have been :
if (size==next_element)
{
// THIS Function doesn't work yet
p_values= growArray(p_values,size);
}
p_values[ next_element++ ] = val
But then you will have to fix growArray that also doesn't work. I will leave that for you to figure out.
Ok -- it compiles and that is great! Well done...
However, that is were the good stuff stops [Sorry]
I am not going to post a working and viable solution. But I am going to point out some of the problems that are leading to segmentation faults/undefined behaviour, and things that probabiliy are giving you logic errors etc [or at least will unless you are lucky]
Copy Constructors/ Assignment operator
First off : MOST serious. All classes have a copy constructor and an assignment operator. There are two cases (a) the programmer defines it and writes it, or (b) the compiler writes it for you. BUT the compiler writes a simple byte copy version. In the case that any of the members of your class do their own memory allocation it is 99% certain that you are not going to get what you want.
There is a solution : [Something we enforce at my work]. You ALWAYS write a copy constructor and an assignment operator. ALWAYS, NO EXCEPTION. If you don't want it called by any function declare it private.
In your case you have class Card for example, it has std::string members, therse will not be copied correctly. Worst still is Player!!
You have defined your own memory pointers in a vector and you still don't write a copy constructor/assignment operator!!
Loop issues
for (int i = 0; i < 2; --i){
cout << displayPlayer4Cards << endl;
player4->playGame();
// ..
}
99.9999% certain …
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.
It is just that you have left a trailling '\n' in the input stream.
You can tidy that up with code like this:
for (int i=0; i<MAXSTUDENT; i++)
{
s1[i].enterDetails();
std::cout<<"Enter number of modules:" ;
std::cin>> x;
for(row=0;row<x;row++)
{
std::cout <<
" Enter module code and marks :" <<(row+1)<<" ";
for(col=0;col<2;col++)
std::cin >> M[row][col];
}
std::cin.ignore
(std::numeric_limits<std::streamsize>::max(),'\n');
Note the last line is std::cin.ignore(Number,character).
What is happening is that the std::numeric_limits<std::streamsize>::max is effectively a value.
It is telling ignore to read at that many characters and ignore all of them until the first instance of '\n' AND if it doesn't find one, then read to the end of the string.
If you were to write say :std::cin.ignore(10,'X'); then that would skip the input stream until (a) 10 characters have been read and discarded, (b) it is at an X (including skipping the X). (c) until the end of the stream IF there were less than 10 characters.
NOTE: If you want std::numeric_limits you need to add the header#include <limits> to your C++ code.
Your problem is one or two things:
(a) the line STOP in your input only has 11 characters in it.
(b) You don't check the input size in your code before cutting.
If you put this in your code :
operand = (opCode.size()>12) ? opCode.substr(13,6) : "";
the code runs to completion.
Obviously, I would also do the same for the instructions = ... line above.
However as well as that problem, which you can solve as suggested above. You will find that value is constant for long period of the loop.
That is because you have done
int t = n/freq;
value[n] = amp*sin(2*pi*freq*t);
n is an integer and freq is an integer, and using integer division, t start as zero for the first 1000 items, then it is 1 for the next 1000 and so on.
Why you divide t by freq and then multiple by freq ? Just dovalue[n] = amp*sin(2*pi*n);.
The next problem is that is hightly unlikely to be what you really wanted.. I am guessing that you actually need this:
value[n]=amp*sin((2.0*pi*n)/freq);
If you are actually trying to round the number to create this effect, then I suggest (a) a comment and (b) stepping through the loop in steps of freq.
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 …The first error is that you have a single collon (:) and then the word Rainful [line 14]. After that without Rainfall.h is would take me too much time to guess what you are doing. So please post that file.
There are a number of issure with various aspects of this coe:
int GetGuess(), I assume is there to return the number of guesses that the player took to get the correct number. However, you use the variable guesses twice. This a scoping issue. First, there is the variable guesses which is defined at line 83 and is within scope of the whole function, and then their is the variable guesses that is defined on line 90, that is only within the for loop. Those two variables are different.
Thus the return from the function is ALWAYS 1.
Next, the function DrawNum, maybe not how I would have done it BUT it works. You can check that by doing the simple substitution,y = static_cast<int> (1 + 0* (max / x)); andy = static_cast<int> (1 + RAND_MAX* (max / x)); and you see you get the result 1, and max (as required).
Then there is the issue of calling GetGuess from main [you also call it from CalcNewMoney.
Also why is there a loop in CalcNewMoney. Delete lines 119,120. and
replace with something like:
if (guesses!=7)
win=1;
else
win=0;
[Note: that there is a short for to do that in c/C++ win=(guesses==6) ? 1 : 0 but if you haven't seen it before the code above works fine.
Also note that GetGuess returns 7 if you don't successfully guess.
You also have a common mistake: cout << "You have won " << (wins*100)/games << "% of …
You logic is flawed because you put == instead of >= . Consider the lines 15 -19 .
while (num ==900) {
cout <<"CM";
num = num - 900;
}
Now if number is 901, that isnt going to be executed, so you want to change the == to >= . Beyond that it now mostly works I think.
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.
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 : …ok -- you start with some input and checking -- kind of ok -- Note that you have not checked if the user inputs that he wants 50 numbers... you program would break.
After than finding the min positive value: That is a bit of a mess of logic/coding etc. What you want to do is establish a start value, and then
continue. You seem to be doing that with a flag so lets keep that idea. But to help lets use variable names that mean something [always do that!]
int initialFlag(1); // set the inital flag to true
int minValue; // No need to initialize as min value controlled by flag
for(int i=0;i<n;i++)
{
// This if statement described below -- complex
if (z[i]>0 && (initialFlag==1 || minValue>z[i]))
{
minValue=z[i]; // store new best value
initialFlag=0; // We have a value
}
}
// some checks after the loop.
if (initalFlag==0) // every number was negative
{
std::cout<<"All values negative"<<std::endl;
}
else
{
std::cout<<"Min value = "<<minValue<<std::endl;
}
Look at the if statement very carefully! There is a lot going on there.
First off it has three sub-tests. The most important thing to realize with C++ (and many languages like it) is that logical expressions (things that evaluate to true/false) can be combined with boolean operators, the two commonest ones are && which means AND, and || which means OR.
So What happens with that line : The first test is 'is the nubmer positive'
…
There are several issues with this program that will result in problems.
First off the cause of the error is likely to be the lines:
cin >> end;
bool arr[end];
You just can't do that in standard C++. The array is sized at compile time not runtime. [I know several compilers actually work with this but ... ]
The size of arr could be anything so fix it like this : [Assuming that you don't want to use a resizable container like std::vector]
int maxTestNumber
std::cin >> maxTestNumber;
if (maxTestNumber<0 || maxTestNumber>10000)
{
std::cout<<"Test number out of sane range "<<std::endl;
return -1;
}
bool* arr=new bool[maxTestNumber];
// ... Your code here
// REMEMBER :: memory allocated with new should be deleted
delete [] arr;
return 0;
}
Next not actual an error, BUT if you intend to put `using namespace std;``
into your code. Do you really want to be using variable names like "end" which is just one character away from endl and a whole pile of pain to find the error.
Next: you have the last loop going from i=start-1 to AND including end. Thus on line 25 you do if (arr[i]!=0 && i!=0) Unfortunately, you have just accessed arr +1 past the last member of the array!
After that you shoudl be in a postion to fix your algorithm..
I made a slight error : the line: if (data[i]>0 || data[i]<101)
should have read if (data[i]>0 && data[i]<101).
Obviously I just typed the code into the post, I didn't actually compile/run it -- I will learn... :-)
So I suspect that you have a data value that is above 100 or below zero....
If that doesn't work can you give the full program that you are testing and we can have a longer look..
The first thing I see is that the frequency code is extremely inefficient. It has a double loop, i.e. you loop over 100 times over all the quanties. If you had 300 numbers to read then you would have 30000 steps!!
There is also a horrible memory error. Your array is 100 units long. but you index freq[100]. Since array start from 0. You only have memory up to freq[99]. [Note that the array index is a short way of saying how many steps away from the start item
I suspect that you have made the same error with the array data. That may be effecting your result, because data[100] may contain corrupted data.
What about this:
int freq[100];
for(int i=0;i<100;i++)
freq[i]=0;
for(int i=0;i<quant;i++)
{
if (data[i]>0 || data[i]<101)
freq[data[i]-1]++;
else
{
std::cout<<"Data out of range :"
<<data[i]<<std::endl;
}
}
That gets your frequency a bit better...
Now to the actual question:
What you have their is a mess, if you have let use say this:
3,3,3,4,4,4,7,7,7. Then you would want the average of 3+4+7.
So you would need to (a) store the running total, (b) store the number of contributions to the running total, (c) store the number of items in the biggest mode value, so that if you then had this : 3,3,3,4,4,4,7,7,7,8,8,8,8, you would discard the running total of 3+4+7, in favour of just 8.
// Declare and initialize our three variables:
int modeCnt(0); // Count of blocks contributing to sum.
int …Observe that you are calculating factorials as the division : So the quickest route is to remove the common factor.
E.g
int calc(const int I)
{
int sum(0);
int x(I);
// D=(pow(i,5.)/120)-(pow(i,4.)/12)+(23*pow(i,3.)/24)-(83*i*i/12)+(571*i/30)
sum+=(120/30)*571*x;
x*=I;
sum-=(120/12)*83*x;
x*=I;
sum+=(120/24)*23*x;
x*=I;
sum-=(120/12)*x;
x*=I;
sum+=x;
return sum/120;
}
Note that I have used the 120 as a factor and you could put it into a constant etc. but it shows the basic trick.
Typcially I would have used an array like this:
int calc(const int I)
{
const int topFactor[]={571,-83,23,-1,1};
const int baseFactor[]={30,12,24,12,120}
const int commonF(120);
int sum(0);
int x(1)
for(int j=0;j<5;j++)
{
x*=I;
sum+= topFactor[j]*(commonF/baseFactor[j]);
}
return sum/commonF;
}
Finally the value D that you are calculating is not an interger function, and you would get rounding effects.
I am assuming that I understood the question correctly, if not please say!
The first problem is that it is very very difficult to figure out without the class definition, so I could be extemely wrong.
(a) I have not idea why you can't add two identical matrixes together, e.g. MatrixClass Sum=A.sum1(A);
So I think lines 9-11 are strange .
(b) The code for both the constructor AND the sum have the following memory leak: [See comments]
MatrixClass MatrixClass::sum1(const MatrixClass &rhs) const
{
MatrixClass C;
C.dataptr = new double[M * N]; // ALLOCATE HERE
// This test is wrong, consider that sizeof(rhs.dataptr) is NOT
// the number of M*N but actually the size of a pointer.
if(M*N*sizeof(this->dataptr) == M*N*sizeof(rhs.dataptr))
{ //... }
else
{
C.dataptr = 0; // MEMORY NOT FREED
}
// C IS A MATRIX CLASS BUT N / M ARE NOT SET CORRECTLY
return C;
}
You can see that you both have a memory leak and do not set M and N in object C. Since although you allocate the memory, you do not set the sizes. That will have nasty effects later.
Your test should have been : if (M*N==rhs.M*rhs.N) Your actual test make the wrong assumption about what sizeof(rhs.dataptr) returns, which is not a runtime value but a compile time value. It is going to return the size of a pointer [typically 8 for many compilers]
It certainly doesn't defeat the point of using templates, that is that you write one function and have it work for multiple types.
The simple template form can be resolved. It is just here that the compiler needs some help because it cannot know which one you ment. [I have figured out that my previous example was a little obtuse: sorry]
Consider: FILE 1 with this code:
template<typename T>
struct A
{
typedef int XInt;
};
Consider that you write something like this:
// this can be in a .h include file
template<typename T> void func(typename A<T>::XInt&);
// this can be in a .cpp file
template<typename T>
void func(typename A<T>::XInt& x)
{
std::cout<<"x == "<<x<<std::endl;
}
BUT in another file, somewhere else you write this:
// Specialization for when <T> is double
template<>
void func(typename A<double>::XInt& x)
{
std::cout<<"double == "<<x*x<<std::endl;
}
Now the poor compile sees you code
int main()
{
int i;
func(i);
}
What can it possibly tell about the intent that you had. It can't know. Both versions correctly match, and it doesn't know that maybe in another file somewhere that will be discovered at link time, or written by another programmer linking to your library
is a specialization of func<strangeClassType>(int&). So it complains and throws an error.
You can easily replace struct A by std::vector, and you can see that there are hundreds of different std::vectorthat have identical std::vector<int>::iteratorbecause you can specialize vector by the allocator. That is why …
Sorry it is a error -- not sure it is a beginners error, but it is an error type I think you should know about.
Solution is to write
display<int>(myBeginIterator,myEndIterator);
Reason:
What you have done is not give the compiler enough information to determine the type of the iterator. That comes about because iterator itself is a derived templated type. There is insufficient information to determine the exact type.
I have a simplified understanding of that (anyone else want to chip in with an explaination please do).
What happens is that you want a class value type. Let us write a simple one :
template<typename T>
struct A
{
typdef int XInt;
typedef T* XPtr;
};
Now it is perfectly possible to use XInt like this: A<double>::XInt i(20);
And obviously you can use it like this:
template<typename T>
void func(typename A<T>::XInt& x) { }
Everything is ok (at the moment) but the problem comes in that <T> does not actually define what you are getting as the type of XInt . You can get multiple versions of the same type, when all the compiler actually can resolve is the base type (int in this case). Thus to the compiler it is ambiguious which is forbidden. So you have
to specify e.g. int i; func<double>(i); func<int>(i);
Hope that helps, if anyone would like to make that clearer or point out the errors in my simple understanding please do.