To Veterans: Have a better way of coding this?

This is a discussion on To Veterans: Have a better way of coding this? within the C++ Programming forums, part of the General Programming Boards category; Welcome!! I'm adding my source code in here to recieve criticism from other programmers....

  1. #1
    Registered User
    Join Date
    Nov 2002
    Posts
    18

    To Veterans: Have a better way of coding this?

    Welcome!!

    I'm adding my source code in here to recieve criticism from other programmers.
    Attached Files Attached Files

  2. #2
    Registered User matheo917's Avatar
    Join Date
    Sep 2001
    Posts
    279
    just for reference...what's this supposed to be???

  3. #3
    Skunkmeister Stoned_Coder's Avatar
    Join Date
    Aug 2001
    Posts
    2,572
    Im sorry to say this but its not too good at all.

    first u mix include headers using some using std namespace and some not.This isnt a good start especially as none of those headers are used in main.h.Then using namespace std brings the whole of the namespace into scope when in reality you are not using any of it.
    You should prefer using initialisation list instead of constructor body to initialise members.Only use body if u have to.No checking either for bounds. Not sure how class is to be used. Will negative values here make sense? Should u check X and Y before initialising members.
    Just for aesthitic value a virtual destructor wouldnt have gone amiss. That would make this class more value as a base class.
    operator + is ok as u have done it but again would this make sense still if x and y are negative?
    friend int main() WTF what,why,are you trying to do? Why on earth should main be a friend of this class.Thats preposterous.
    Information need not be a friend either. It uses accessor functions to access the classes internal data and does not need direct access.
    Magnitude needs rewriting and also does not need to be a friend for same reasons as Information.
    Your accessor functions are laughable. Where is your const correctness?!
    Another word of advice.... In c++ we have references. They can dramatically reduce the need for passing pointers into functions.

    I'd seriously consider a rewrite.
    Free the weed!! Class B to class C is not good enough!!
    And the FAQ is here :- http://faq.cprogramming.com/cgi-bin/smartfaq.cgi

  4. #4
    Registered User matheo917's Avatar
    Join Date
    Sep 2001
    Posts
    279
    i strongly agree w/ Stonge_Coder...

  5. #5
    Blank
    Join Date
    Aug 2001
    Posts
    1,034
    Here is a better way. You have to be careful
    with the normalization since you can have 0 vectors.
    Right now I comparing floating point numbers to 0.0 but to
    make it more robust you should compare it to a range of
    numbers near 0. You can improve this further by maybe using
    templates, adding substracting, multiplication etc. Another
    way to handle the normalization is to return another vector
    and make the action function const.

    Code:
    #ifndef VECTOR_H__
    #define VECTOR_H__
    
    #include <stdexcept>
    #include <cmath>
    
    class Vector {
        double x;
        double y;
    public:
        Vector(double new_x = 0.0, double new_y = 0.0) : x(new_x), y(new_y) { }
        Vector& operator+=(const Vector& rhs) {
            x += rhs.x;
            y += rhs.y;
    
            return *this;
        }
    
        Vector operator+(const Vector& rhs) {
            Vector& v = *this;
            v += rhs;
            return v;
        }
    
        double get_x() const { return x; }
        double get_y() const { return y; }
        
        double magnitude() const {
            return std::sqrt(x * x + y * y);
        }
    
        void normalize() {
            double m = magnitude();
            if (m == 0.0)
                throw std::runtime_error("Divide by 0");
            
            x /= m;
            y /= m;
        }
    };
    
    #endif
    
    #include <iostream>
    using std::cout;
    using std::endl;
    
    #include "Vector.h"
    
    
    int main(void)
    {
        Vector v(1, 9);
        Vector u(4, 5);
        Vector w;
        
        cout << "v = (" << v.get_x() << ", " << v.get_y() << ")" << endl;
        cout << "u = (" << u.get_x() << ", " << u.get_y() << ")" << endl;
        w = u + v;
        cout << "w = u + v" << endl;
        cout << "w = (" << w.get_x() << ", " << w.get_y() << ")" << endl;
        
        cout << "v's magnitude = " << v.magnitude() << endl;
        cout << "u's magnitude = " << u.magnitude() << endl;
        cout << "normalizing" << endl;
        v.normalize();
        u.normalize();
        cout << "v = (" << v.get_x() << ", " << v.get_y() << ")" << endl;
        cout << "u = (" << u.get_x() << ", " << u.get_y() << ")" << endl;
    
        cout << "v's magnitude = " << v.magnitude() << endl;
        cout << "u's magnitude = " << u.magnitude() << endl;
           
        return 0;
    }

  6. #6
    Senior Member joshdick's Avatar
    Join Date
    Nov 2002
    Location
    Phildelphia, PA
    Posts
    1,146
    Originally posted by Stoned_Coder
    Just for aesthitic value a virtual destructor wouldnt have gone amiss. That would make this class more value as a base class.
    How would that be done? I thought that delete could only be used if new was used. What would you put into the implementation of the destructor, then?
    FAQ

    "The computer programmer is a creator of universes for which he alone is responsible. Universes of virtually unlimited complexity can be created in the form of computer programs." -- Joseph Weizenbaum.

    "If you cannot grok the overall structure of a program while taking a shower, you are not ready to code it." -- Richard Pattis.

  7. #7
    Skunkmeister Stoned_Coder's Avatar
    Join Date
    Aug 2001
    Posts
    2,572
    declare it in the class as virtual ~classname(); and in the implementation file add a do nothing implementation.

    class MyClass
    {
    virtual ~MyClass();
    };

    MyClass::~MyClass() {}

    If you have no intention of ever using this as a base class then this is probably overkill,if on the other hand you intend to derive from this class and use polymorphically then a virtual destructor is necessary.
    Free the weed!! Class B to class C is not good enough!!
    And the FAQ is here :- http://faq.cprogramming.com/cgi-bin/smartfaq.cgi

  8. #8
    Registered User
    Join Date
    Aug 2002
    Posts
    57
    Heres the revision
    Attached Files Attached Files
    "A Programmer being told to 'go to' hell sees the 'go to' part of the sentence as the worst part." - Master Foto

  9. #9
    Skunkmeister Stoned_Coder's Avatar
    Join Date
    Aug 2001
    Posts
    2,572
    ok here we go again.

    accessor functions should be const.
    PrimTypebyValue GetPrivateMember()const { return PrivateMember;}
    or
    const UserDefTypebyConstRef& GetPrivateMember()const { return PrivateMember;}

    operator += should return *this

    iostream is not used in main.h it should be included in main.cpp instead

    main does never need to be prototyped.

    Information() should take a reference not a pointer.

    (*Vector).getX() is better written Vector->getX()
    Free the weed!! Class B to class C is not good enough!!
    And the FAQ is here :- http://faq.cprogramming.com/cgi-bin/smartfaq.cgi

  10. #10
    Skunkmeister Stoned_Coder's Avatar
    Join Date
    Aug 2001
    Posts
    2,572
    Nick your operator + is wrong. It changes *this....

    a=b+c you dont expect b to be changed. *this should be const throughout operator +
    Free the weed!! Class B to class C is not good enough!!
    And the FAQ is here :- http://faq.cprogramming.com/cgi-bin/smartfaq.cgi

  11. #11
    Registered User
    Join Date
    Nov 2002
    Posts
    18
    Why should accessor functions be const?

  12. #12
    Skunkmeister Stoned_Coder's Avatar
    Join Date
    Aug 2001
    Posts
    2,572
    const objects are only allowed to call upon const member functions and cannot call non const member funcs.

    i.e.

    const CVector vec(10,10); // a const object
    int a = vec.getX(); // ERROR unless function is const member and it should be because it doesnt change the state of the object.
    Free the weed!! Class B to class C is not good enough!!
    And the FAQ is here :- http://faq.cprogramming.com/cgi-bin/smartfaq.cgi

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Replies: 9
    Last Post: 03-20-2009, 05:22 PM
  2. Coding Guideline ....!!
    By imfeelingfortun in forum Tech Board
    Replies: 8
    Last Post: 10-08-2006, 07:09 AM
  3. Before Coding
    By cyberCLoWn in forum C++ Programming
    Replies: 16
    Last Post: 12-15-2003, 01:26 AM
  4. Coding Contest....
    By Koshare in forum A Brief History of Cprogramming.com
    Replies: 46
    Last Post: 10-14-2001, 04:32 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21