Want to ask something...

Hey guys,

I know that what i'm about to ask is a time consuming thing, but i'm starting to learn c++ and just made a simple prog to calculate currency exchanges.

What i would like to ask is if any of you would take a look in my code and give me some hints, critics, suggestions, etc.

If any of you will do this, then you can get my code and the text file with currencies here:

https://drive.google.com/file/d/0By0q1HlvGjiWNFNQVlRPNG41SnM/edit?usp=sharing
https://drive.google.com/file/d/0By0q1HlvGjiWcVlqYzI2dE94Mkk/edit?usp=sharing

Thanks in advance!
Last edited on
You are illegally declaring an array of currency statically with a dynamic size. Replace the line with something like the following:
1
2
3
4
5
/* currency coins [lines]; */
currency* coins = new currency[lines];

// use the array... when done, clean up:
delete[] coins;


Also, I don't really get what you are doing with your loop. Why don't you just put each one in sequence to each other, rather than skipping the irrelevant ones each time? If you want to escape quickly, simply throw in a quick call of return 0;, and if you want to loop until valid input is given, then use a 'normal' loop to do so (e.g. a do {...} while (...) is often appropriate). I'll admit that it was clever, but often readability is preferred over cleverness.

In your loop in your get_max_length function, rather than having duplicated code in your function, instead a single if/else would suffice. However, you could just as easily use the 'max' function in <algorithm>:
1
2
3
4
5
6
void get_max_length(...) {
    maxlength[0] = std::max_element(coins, coins + lines, 
        [](currency& c) { return c.country.length(); });
    maxlength[1] = std::max_element(coins, coins + lines, 
        [](currency& c) { return c.coin.length(); });
}
Last edited on
Thanks for the tips NT3.

I'll fix the array declaration like you said.

As for the loop, i must confess that it's a bit confusing even for me... :)
The problem is that i want to make several things at the same time:

1. print help or end the prog at any time;
2. if user inputs "help" then keep track of what he was inputing so that i can ask again for the same input;
3. in the end ask if another exchange is to be made and, if so, repeat all again.

It was a bit difficult to get all this done, since my c++ isn't quite good...

As for the 'max' function in <algoritm>, i didn't have the time to get over all the things in c++ reference... I'm only programming in my spare time... But i'll take look and try to simplify this.

And again thanks for the comments!
Hey guys.

I have remade my loop in main(). I think that now it's more readable and easier to understand.
I simplified also my get_max_length(...) function.

here are the source files:

https://drive.google.com/file/d/0By0q1HlvGjiWQ0trU0RRc0JnUW8/edit?usp=sharing
https://drive.google.com/file/d/0By0q1HlvGjiWcVlqYzI2dE94Mkk/edit?usp=sharing

Any more tips?

@NT3:

I didn't manage to put the max_element function to work as you sugested...
Here are the errors i got:

In function 'void get_max_length1(currency*, int, int*)':
278|error: invalid conversion from 'currency*' to 'int' [-fpermissive]|
280|error: invalid conversion from 'currency*' to 'int' [-fpermissive]|


Thanks for the tips.


Last edited on
Seems no more tips...
Topic archived. No new replies allowed.