0

I have a simple test program in C++ that prints out attributes of a circle

#include <iostream>
#include <stdlib.h>
#include "circle.h" // contains the Circle class

using namespace std;

void print_circle_attributes(float r) {
    Circle* c = new Circle(r);
    cout << "radius: " << c->get_radius() << endl;
    cout << "diameter: " << c->get_diameter() << endl;
    cout << "area: " << c->get_area() << endl;
    cout << "circumference: " << c->get_circumference() << endl;
    cout << endl;
    delete c;
}

int main(int argc, const char* argv[]) {
    float input = atof(argv[0]);
    print_circle_attributes(input);
    return 0;
}

when I run my program with the parameter 2.4 it outputs: 

radius: 0.0
diameter: 0.0
area: 0.0
circumference: 0.0

I've previously tested the program without the parameter, but simply using static values, and it ran just fine; so I know there's nothing wrong with the class I made...

So what did I do wrong here?

Electric Coffee
  • 11,733
  • 9
  • 70
  • 131
  • 2
    Please, whatever taught you to use `new` like that, ignore it. Just write `Circle c(r);`, no `new`, no `delete`, no pointers. Magic! – R. Martinho Fernandes Oct 29 '13 at 09:39
  • Oh, and there are [good learning materials here](http://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list). – R. Martinho Fernandes Oct 29 '13 at 09:39
  • I use the new keyword specifically because I wanted to create the object on the heap, not on the stack, it's safer since it doesn't risk stack overflows (although a single object won't do anything, but still) – Electric Coffee Oct 29 '13 at 09:41
  • `#include "hidden_functions.h" // contains the Circle class` (proceeds to argue about the _sasfety_ of manual memory management on the free store. Yup. Oodles of sense it makes) – sehe Oct 29 '13 at 09:44
  • and why is it wrong of me to include the header that gives me access to the class in the first place? – Electric Coffee Oct 29 '13 at 09:46
  • @ElectricCoffee The naming is completely at odds with the purpose. This is exmplified by the fact that you needed a comment to point out what should have been obvious (trivial comments are a code smell). – sehe Oct 29 '13 at 09:48
  • 2
    Hint: your 'solution' to stack overflow is fraught with a _lot_ more potential danger than what you were 'solving for' in the first place too. Please accept the gentle prodding of the SO [tag:c++] community as a sign that you are learning nice things :/ – sehe Oct 29 '13 at 09:50
  • the purpose of the program was to test out functions that weren't declared in the header, hence the name "hidden functions" – Electric Coffee Oct 29 '13 at 09:50
  • Even if I accept the reasoning as valid, for the sake of argument, it's still not a valid justification to use new like that (there isn't any?). `std::unique_ptr`, `boost::scoped_ptr` or even `std::auto_ptr` exist for a reason. – R. Martinho Fernandes Oct 29 '13 at 09:51
  • it's a practice program, it's not doing anything serious, if it works it works – Electric Coffee Oct 29 '13 at 09:54
  • @ElectricCoffee Okay, fair enough. Next time, `#include "circle.hpp"` will convey the same information to SO readers, but taking fewer brain cycles. – sehe Oct 29 '13 at 09:55
  • @ElectricCoffee *practice* means *learning*. And what the people here are trying to tell you is that you are practicing/learning/getting used to do dangerous, unsafe thechniques (aka raw pointers) while there are better alternatives without any costs. – Arne Mertz Oct 29 '13 at 10:15
  • I'm not gonna change the displayed code now – Electric Coffee Oct 29 '13 at 10:19

2 Answers2

4

argv[0] is the name of the executable being invoked.

Your first command line parameter will be in argv[1].

To make sure that your program does not silently fail again, you should check how many parameters you actually have and if the atof returns a value, and show a message to the user explaining the issue accordingly.

sehe
  • 374,641
  • 47
  • 450
  • 633
SJuan76
  • 24,532
  • 6
  • 47
  • 87
4

argv[0] is the program name. You want argv[1] for the first argument.

Also, check that argc is at least two before trying to access it. You might also consider std::stoi, std::istringstream or strtod rather than atoi for conversion, since they can detect bogus input.

Finally, why are using new when an automatic variable will suffice? You should get out of that habit straight away, or spend the rest of eternity debugging memory leaks.

Mike Seymour
  • 249,747
  • 28
  • 448
  • 644
  • I used new to create the object on the heap, rather than the stack; it was done on purpose – Electric Coffee Oct 29 '13 at 09:43
  • 2
    @ElectricCoffee: When you really do need dynamic allocation (which you don't here), you should always use [RAII](http://en.wikipedia.org/wiki/), in particular smart pointers, to bind the object to a scope and make sure the memory is released, even if an exception is thrown. But you don't need a dynamic object here at all, and using the heap when you don't need it is a bad habit to get into. – Mike Seymour Oct 29 '13 at 09:49