Help with code for slef similar melodies

[inkexit]

I am a very amatuer c++ programmer and a somewhat accomplished
composer. I am trying to write some code that creates 'self similar'
melodies from a base melody the user inputs. This musical idea was
created by Tom Johnson. Apologies to those who are not musically
literate, but as this is basically a mathmatical problem I think most
here can follow the logic.

Lets keep eveything in C Major, that's all the white keys on the piano.
Being as that we are in C Major, the note C will be our base note.

So:
C=0
D=1
E=2
F=3
G=4
A=5
B=6
(next octave up)
C=7
D=8
E=9
F=10
G=11
A=12
B=13
etc...

Using this, we can enter a base melody:

0, 5, 6, 4, 1, 2, 5, 0

The idea of creating self similar melodies is to take all the intervals
(the distance inbetween the neighboring notes) and double them. So,
from this base melody, the first iteration would be:

0, 10, 12, 8, 2, 10, 0

next iteration:

0, 20, 24, 16, 4, 20, 0

etc...

So anyway, here's my code. It won't compile. Please help, and thanks
for wading through this.


#include <iostream>
#include <iomanip>
#include <string>
using namespace std;

void exp_melod_self_similar(int [], int);

int main()
{

int note_number = 1;
int melod_length;

cout << "Enter length of melody in 1/8 notes:";
cin >> melod_length;

int base_melod[melod_length];

do
{
cout << "note " << note_number;
cin >> base_melod[note_number];
note_number++;
}
while (note_number < melod_length);

exp_melod_self_similar(base_melod, melod_length);


return 0;
}

void exp_melod_self_similar(int new_melod[], int melod_length)
{
int new_melod[];
new_melod[0] = base_melod[0];
for (i=1, i<=melod_length, i++)
{
new_melod = base_melod * 2;
cout << new_melod;
}
}

 

[Alf P. Steinbach]

* (e-mail address removed):

So anyway, here's my code. It won't compile. Please help, and thanks
for wading through this.

I fixed the syntax errors.

What remains is to fix the logic.

You should also try to use more self-descriptive names.

#include <iostream>
#include <iomanip>
#include <string>
using namespace std;

void exp_melod_self_similar(int [], int);

int main()
{

int note_number = 1;
int melod_length;

cout << "Enter length of melody in 1/8 notes:";
cin >> melod_length;

int base_melod[melod_length];

do
{
cout << "note " << note_number;
cin >> base_melod[note_number];
note_number++;
}
while (note_number < melod_length);

exp_melod_self_similar(base_melod, melod_length);


return 0;
}

void exp_melod_self_similar(int new_melod[], int melod_length)
{
int new_melod[];
new_melod[0] = base_melod[0];
for (i=1, i<=melod_length, i++)
{
new_melod = base_melod * 2;
cout << new_melod;
}
}

Syntax errors (but not logic) fixed -- but you should really use the
std::vector 'at' member function, which provides some safety, rather
than the '[]' indexing operator, and then you'll find at least one bug:


#include <iostream>
#include <iomanip>
#include <string>
#include <vector>

void exp_melod_self_similar( std::vector<int>& base_melod )
{
std::vector<int> new_melod( base_melod.size() );
new_melod[0] = base_melod[0];
for( int i = 1; i <= base_melod.size(); ++i )
{
new_melod = base_melod * 2;
std::cout << new_melod;
}
}

int main()
{
int melod_length;

std::cout << "Enter length of melody in 1/8 notes:";
std::cin >> melod_length;

std::vector<int> base_melod(melod_length);

for( int note_number = 1;
note_number < melod_length;
++note_number )
{
std::cout << "note " << note_number;
std::cin >> base_melod[note_number];
}

exp_melod_self_similar( base_melod );
}

 

[int2str]

I am a very amatuer c++ programmer and a somewhat accomplished
composer. I am trying to write some code that creates 'self similar'
melodies from a base melody the user inputs.

An excellent book on learning C++ is "Accelerated C++" by Koenig &
Moo.

#include <iostream>
#include <iomanip>

#include <string>
using namespace std;

void exp_melod_self_similar(int [], int);

int main()
{

int note_number = 1;
int melod_length;

cout << "Enter length of melody in 1/8 notes:";
cin >> melod_length;

int base_melod[melod_length];

Use std::vector< int > instead.
Then you don't even have to ask how long the melody is as the vector
auto-expands.

do
{
cout << "note " << note_number;
cin >> base_melod[note_number];
note_number++;
}
while (note_number < melod_length);

A for() loop may be more readable here.

exp_melod_self_similar(base_melod, melod_length);

You are not freeing the memory for base_melod here.
Use:

delete [] base_melod;

return 0;
}

void exp_melod_self_similar(int new_melod[], int melod_length)

The parameter name is wrong. Use "base_melod" instead of new_melod
here.

{
int new_melod[];
new_melod[0] = base_melod[0];

Why not do this in the loop?
Interesting btw. because somebody pointed this out on this list not too
long ago that people make this mistake. Could you explain your
reasoning, since I don't understand why you would do this?

for (i=1, i<=melod_length, i++)

You need to declare i here. Also, use semicolons to separate the
condition etc. Like so:

for ( int i=0; i<melod_length; ++i )

{
new_melod = base_melod * 2;
cout << new_melod;

If you're only printing the new melody, no need to store it into
new_melod first.

}

You're leaking memory again.
After the loop:

delete [] new_melod;

}

Overall, using std::transform() on a std::vector<int> here would work
great.

Cheers,
Andre

Cheers,
Andre

 

[inkexit]

int main()
{

int note_number = 1;
int melod_length;

cout << "Enter length of melody in 1/8 notes:";
cin >> melod_length;

int base_melod[melod_length];

Use std::vector< int > instead.
Then you don't even have to ask how long the melody is as the vector
auto-expands.

Well, that's cool. I had no idea about vector, other than the
mathmatical idea of a direction and a velocity. I have only been
studying c++ for about 2 months.

do
{
cout << "note " << note_number;
cin >> base_melod[note_number];
note_number++;
}
while (note_number < melod_length);

A for() loop may be more readable here.

Okay, but when I tried doing input with a for loop on some other code
for class my teacher marked me down. He said to always use a do while
loop for input becuase it is guarenteed to execute once.

{
int new_melod[];
new_melod[0] = base_melod[0];

Why not do this in the loop?
Interesting btw. because somebody pointed this out on this list not too
long ago that people make this mistake. Could you explain your
reasoning, since I don't understand why you would do this?

Well, the idea behind self similar melodies is that they all start with
the same note, so since it isn't altered, I did it outside of the loop
to make sure it was always identical.

for ( int i=0; i<melod_length; ++i )

{
new_melod = base_melod * 2;
cout << new_melod;

If you're only printing the new melody, no need to store it into
new_melod first.

For now I'm only printing it, but in the future I will be using it in
other functions, creating a second iteration, for example.

Overall, using std::transform() on a std::vector<int> here would work
great.

Well, that's all greek to me for now. Thanks for the reply.

 

[inkexit]

I fixed the syntax errors.

What remains is to fix the logic.
Thanks!


Syntax errors (but not logic) fixed -- but you should really use the
std::vector 'at' member function, which provides some safety, rather
than the '[]' indexing operator, and then you'll find at least one bug:

Haven't learned to vector yet. You say it makes it safer, are you
taking about over-run?

#include <iostream>
#include <iomanip>
#include <string>
#include <vector>

void exp_melod_self_similar( std::vector<int>& base_melod )

I don't know what's going on here. Are you passing two arguments?

{
std::vector<int> new_melod( base_melod.size() );

Don't get this at all, what is this statement even doing?

new_melod[0] = base_melod[0];
for( int i = 1; i <= base_melod.size(); ++i )
{
new_melod = base_melod * 2;
std::cout << new_melod;

}
}

int main()
{
int melod_length;

std::cout << "Enter length of melody in 1/8 notes:";
std::cin >> melod_length;

std::vector<int> base_melod(melod_length);

for( int note_number = 1;
note_number < melod_length;
++note_number )

<note_number++> isn't acceptable?

Well, thanks for trying to help but I think I've got more questions now
than I cam in with!

 

[Alf P. Steinbach]

* (e-mail address removed):

I fixed the syntax errors.

What remains is to fix the logic.
Thanks!


Syntax errors (but not logic) fixed -- but you should really use the
std::vector 'at' member function, which provides some safety, rather
than the '[]' indexing operator, and then you'll find at least one bug:

Haven't learned to vector yet. You say it makes it safer, are you
taking about over-run?

Yes, with regard to 'at'.

std::vector also makes things safer by taking care of memory allocation
and deallocation, copying the contents, adding new elements (which you
should consider instead of asking the user for the number of elements),
and much more.

Especially as a beginner you should not use raw arrays; use std::vector.

I don't know what's going on here. Are you passing two arguments?

What's the single <&> all about, I've only used <&&> for boolean
statements?

It passes the argument by reference. Which is like passing a pointer to
the actual argument, but without the pointer syntax. You can
(simplistic explanation) regard a reference as a pointer where the
compiler adds & and * and whatever as required, so you don't have to.

If that doesn't make sense to you, consider a reference as a name for
some other variable.

For a formal argument that name stands for that calling code's actual
argument, so anything you do with the formal argument is actually done
with the calling code's actual argument.

I left out using 'const' because your code didn't have 'const'.

It would probably be a good idea to add a 'const' there.

Don't get this at all, what is this statement even doing?

It declares a local variabable called 'new_melod', just as your original
code did.

The type of the variable is std::vector<int>.

The initial size of that vector is specified to be base_melod.size(),
i.e. the same size as the vector passed in as function argument.

new_melod[0] = base_melod[0];
for( int i = 1; i <= base_melod.size(); ++i )
{
new_melod = base_melod * 2;
std::cout << new_melod;

Why add the <std::> before the cout here?

Because it's generally an UnGood idea to use 'using namespace std;';.

E.g., you can get in trouble if you'd like to use some user-defined
class called 'string'.

It's generally better to qualify names from the standard library, as
above.

<note_number++> isn't acceptable?

Acceptable but not perfect... ;-)

Well, thanks for trying to help but I think I've got more questions now
than I cam in with!

That's good, that's what learning is all about.

 

[int2str]

Well, that's all greek to me for now. Thanks for the reply.

Here's what I mean...
Let me know what you think and if you have any questions.

#include <iostream> // cout, cin, endl
#include <vector> // vector
#include <algorithm> // copy, transform
#include <string> // string
#include <sstream> // stringstream
#include <iterator> // ostream_iterator

using namespace std;

class make_self_similar
{
public:
make_self_similar() : first_( true ) {}

int operator()( const int & note )
{
if ( first_ )
{
first_ = false;
return note;
}

return note * 2;
}

private:
bool first_;
};

int main()
{
cout << "Enter melody and press ENTER." << endl;
cout << "Separate notes with spaces (ex. 0 5 6 3)." << endl <<
endl;
cout << "Melody: ";

string melody_str;
getline( cin, melody_str );

stringstream ss( melody_str );
vector< int > melody;

int note = 0;
while ( ss >> note )
melody.push_back( note );

transform( melody.begin(), melody.end(), melody.begin(),
make_self_similar() );

cout << "Self similar melody: ";
copy( melody.begin(), melody.end(), ostream_iterator<int>(cout,"
") );
cout << endl;
}

 

[Jonathan Mcdougall]

I am a very amatuer c++ programmer and a somewhat accomplished
composer. I am trying to write some code that creates 'self similar'
melodies from a base melody the user inputs. This musical idea was
created by Tom Johnson. Apologies to those who are not musically
literate, but as this is basically a mathmatical problem I think most
here can follow the logic.

<snip>

You've been given valuable help, but it seems no one pointed out the
errors in the code. I do not recommend using arrays (std::vector is
the way to go), but just so you know why it doesn't compile.

#include <iostream>
#include <iomanip>
#include <string>
using namespace std;
http://www.parashift.com/c++-faq-lite/coding-standards.html#faq-27.5

void exp_melod_self_similar(int [], int);

Well, nothing prevents you from defining the function here.

int main()
{

int note_number = 1;
int melod_length;

Always initialize your variables.

int melod_length = 0;

And by the way, try to either abbreviate names or type them fully. Not
in between.

int melody_length = 0;

cout << "Enter length of melody in 1/8 notes:";
cin >> melod_length;

int base_melod[melod_length];

That's illegal. Array sizes must be compile-time constants, such as

int base_melody[10];

or

const int size=10;
int base_melody[size];

If you want to use arrays (but you should not), either

1) allocate them on the heap, which allows you to specify the number
of elements at runtime

int *base_melody = new int[melody_length];
// ...
delete[] base_melody;

or

2) allocate too much elements and keep track of the number of used
ones:

const int MAX_MELODY_SIZE = 100;
int base_melody[MAX_MELODY_SIZE];

Then you'll still have to ask how many eighth notes the user wants
and use that number to fiddle with the array.


As you can see, std::vector makes it much easier:

# include <vector>

typedef std::vector<int> Melody;

int main()
{
Melody base_melody;

while (true)
{
int pitch = 0;
std::cin >> pitch;

if (pitch == -1)
break;

base_melody.push_back(pitch);
}

std::cout << "You entered " << base_melody.size() << " notes.";

// print all the notes entered
for (int i=0; i<base_melody.size(); ++i)
std::cout << base_melody << " ";

// or better, use size_type from the vector as the index type

for (Melody::size_type i=0; i<base_melody.size(); ++i)
std::cout << base_melody << " ";

// or still better, use iterators (kind of pointers)

for (Melody::iterator itor = base_melody.begin();
itor!=base_melody.end();
++itor)
std::cout << *itor << " ";

// or the trendy way, include <algorithm> and do

std::copy(base_melody.begin(),
base_melody.end(),

do
{
cout << "note " << note_number;
cin >> base_melod[note_number];
note_number++;
}
while (note_number < melod_length);

A for loop would be better here:

for (int i=0; i<melody_length; ++i)
std::cin >> base_melody;

exp_melod_self_similar(base_melod, melod_length);

return 0;
}

void exp_melod_self_similar(int new_melod[], int melod_length)
{
int new_melod[];

That's illegal. You must specify a size. This is the same problem
as above.

new_melod[0] = base_melod[0];
for (i=1, i<=melod_length, i++)

Just to make sure you know it, arrays in C++ are indexed from 0 to
size-1. So an array like

int test[10];

has elements test[0], test[1]... test[8] and test[9]. That's it. If
your declaration of the base_melody array in main() had been legal
(remember, that was "int base_melody[melody_length]"), this would be
an out of bounds access when i==melody_length.

So make that

for (i=1; i<melody_length; ++i)

{
new_melod = base_melod * 2;
cout << new_melod;
}
}

I think you'll get stuck pretty soon here. Arrays can't be copied
easily. For example,

int[] f(int base_melody[], int melody_length)
{
// define and fill and new_melody array somehow

return new_melody;
}

is illegal. You'll have to either pass an "output" array to the
function or create one dynamically and return it.


As you can see, arrays are trouble. You *don't* want to use arrays.
You want to use std::vector instead, which correctly grows when
elements are added and can be copied and passed arount freely.


Jonathan

 

[Greg Comeau]

Well, that's cool. I had no idea about vector, other than the
mathmatical idea of a direction and a velocity. I have only been
studying c++ for about 2 months.

I don't know what books your instructor has suggested,
but have a look at http://www.comeaucomputing.com/booklist
and treat yourself to a few of them.