Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

If we have a set of classes or structs such as:

struct Point
{
    float x;
    float y;
};

struct Square
{
    Point topLeft;
    float width;
};

struct Circle
{
    float radius;
    Point middle;
};

If we need to add some functionality to to these, does something like below make sense?

inline Point middleOf(const Square &square)
{
    float halfWidth(square.width/2.0f);
    return Point{
        square.topLeft.x + halfWidth,
        square.topLeft.y + halfWidth};
}

inline Point middleOf(const Circle &circle)
{
    return circle.middle;
}

class MoveSquare
{
public:
    MoveSquare(Square &squareToMove)
    :
    squareToMove(squareToMove)
    {}

    void to(const Point &target)
    {
        squareToMove.topLeft = offsetHalfWidth(target);
    }

private:    
    Point offsetHalfWidth(Point p)
    {
        const float halfWidth(squareToMove.width / 2.0f);
        return Point{
            p.x - halfWidth,
            p.y - halfWidth};
    }

private:
    Square &squareToMove;
};

class MoveCircle
{
public:
    MoveCircle(Circle &circle)
    :
    circleToMove(circle)
    {}

    void to(Point position)
    {
        circleToMove.middle = position;
    }

private:
    Circle &circleToMove;
};

inline MoveSquare move(Square &square)
{
    return MoveSquare{square};
}

inline MoveCircle move(Circle &circle)
{
    return MoveCircle{circle};
}

The main idea is being able to have code that reads like a sentence. Such as:

int main() {

    Square aSquare;
    aSquare.topLeft = Point{1,1};
    aSquare.width = 3;

    Circle aCircle;
    aCircle.middle = Point{1,1};
    aCircle.radius = 2;

    move(aSquare).to(Point{1,1});
    move(aSquare).to(middleOf(aCircle));

    move(aCircle).to(middleOf(aSquare));

    return 0;
}
share|improve this question
2  
If you want your code to read like English, try objective-c. ;) I'm not sure how well accepted this would be in the C++ community, but we definitely strive for verbose code in ObjC. –  nhgrif Jul 9 at 23:16
    
I think this will break OOP, just a little... What if you'll want to inherit Square, with RoundedSquare? And that move, should have global visibility, if you want to use it for readability. Otherwise you have to use namespace it. I don't think this is a good idea. Do you thought about koenig lookup, for this? –  tower120 Jul 9 at 23:23
    
@tower120 hmm koenig lookup. What would using that look like for this? –  PeterSW Jul 10 at 14:30
    
Why not: aSquare.moveTo(Point{1,1}); or aSquare.moveTo(aCircle.center()) –  Loki Astari Jul 10 at 18:08
    
@LokiAstari for many cases I agree that would be a good solution and reads nice and clearly. The situation I had in mind was one where the types are from a third party library. In that case you can not change them at all. –  PeterSW Jul 10 at 20:02
show 2 more comments

6 Answers 6

up vote 7 down vote accepted

Firstly, a negative remark. The power of C++ means that it is not uncommon for new C++ developers to try and make C++ look like a language they feel more familiar with - sometimes English, sometimes a programming language they already know. This is almost always a bad thing - you need to speak like a native.

However, whilst C++ used for mainstream development does have generally understood idioms, one of its capabilities and original design aims is to try to support different styles of programming.

As well as trying to create an application syntax which is more English like, you are externalizing functionality into proxy objects rather than using the OOP paradigm of member functions. IMHO, this is perfectly legitimate as an experiment.

What you'll likely find is that as you pursue this you gave to make various compromises that finally result in you deciding that maybe this is not he way to go. Or maybe not. See what you find.

If I were to go down this path, I'd probably play with using templates to try and get some generic behaviour, given that the OOP approach us not being used. How exactly this goes will depend on whatever other functionality is needed.

For example:

template<class T>
class Move
{
public:
    Move(T& thing)
        : thing_(thing)
    {}
    virtual void to(const Point& position) = 0;
    T& thing() { return thing_; }
private:
    T& thing_;
};

class MoveSquare : public Move<Square>
{
public:
    MoveSquare(Square &squareToMove)
        : Move<Square>(squareToMove)
    {}

    virtual void to(const Point &target)
    {
        thing().topLeft = offsetHalfWidth(target);
    }

private:
    Point offsetHalfWidth(const Point& p) 
    {
        const float halfWidth(thing().width / 2.0f);
        return Point{ p.x - halfWidth, p.y - halfWidth };
    }
};

class MoveCircle : public  Move<Circle>
{
public:
    MoveCircle(Circle &circle)
        : Move<Circle>(circle)
    {}

    virtual void to(const Point& position)
    {
        thing().middle = position;
    }

};
share|improve this answer
    
that method shouldn't be virtual –  BЈовић Jul 11 at 9:43
    
also, what is the point of the Move? It does nothing –  BЈовић Jul 11 at 10:14
    
If this is C++11 then it could be good to add override or final to communicate more clearly and get more help from the compiler. –  PeterSW Jul 11 at 18:44
add comment

It's an interesting concept, but not exactly how I would expect this to be structured.

First, it's superfluous to have an external function to return the middle of a circle when it's already available though it's property.

I would expect a class method to move the object. Something like circle.move(x,y). If you're concerned with making the size immutable, simply set it in a constructor and disallow external access to the radius property.

share|improve this answer
    
I was thinking more for the cases when you can't add anything directly to the classes. I guess I should have been clearer about that in the question... –  PeterSW Jul 9 at 23:42
1  
@PeterSW: Just in general, or do you have some examples? Even if this were a factor, you would have to stick to structs as having classes would suggest that the data members should be private, thus preventing the use of free functions. –  Jamal Jul 9 at 23:45
    
@Jamai In using free functions you're limited to the public interface... but there's often plenty of scope to build more functionality from that. –  PeterSW Jul 9 at 23:56
    
@PeterSW: Right, which is why I asked. I'm not saying that this is plain wrong, but unexpected for basic use (as you've said, clarification should've been added, but it would also invalidate this answer). –  Jamal Jul 9 at 23:59
    
I'm going to go ahead and call out that I don't know this language. I'm speaking from a completely abstract design point of view. It looks like I may have misunderstood the question. –  ckuhn203 Jul 10 at 1:11
show 1 more comment

It reads like a mandate, and not necessarily a sentence which makes this point trivial.

Notice how all are English:

Run this code!

The code ran

The code is running

This design doesn't let you define movement inside the object that moves, so you need to look into different places to find out how the object moves.

Your way is also weird and unfamiliar. Consider how Qt does it:

void QGraphicsItem::setPos(const QPointF & pos)

Sets the position of the item to pos, which is in parent coordinates. For items with no parent, pos is in scene coordinates.

The position of the item describes its origin (local coordinate (0, 0)) in parent coordinates.

share|improve this answer
add comment

There are few problems. I will start with minor, and end with major.


Minor

  1. MoveSquare::to accepts a parameter by const reference, but MoveSquare::offsetHalfWidth accepts the parameter by value. Since offsetHalfWidth doesn't modify it's parameter, it should be passed by const reference
  2. MoveSquare::to just assigns the result of MoveSquare::offsetHalfWidth to some variable. Why not merge these two methods?

Major

I am particularly fond of the KISS principle, therefore I cringed when I saw your code. I would say it is a good example of when NOT to use a proxy pattern.

I would add a method to each of the structures with this signature :

void MoveTo(const Point &target);

it makes the code simpler, and therefore easier to understand.

Also, instead of having overloaded functions to calculate the middle point, I would add a method to the structure.

They would look like :

struct Square
{
    Point topLeft;
    float width;

    void MoveTo(const Point &target);
    Point MiddlePoint() const;
};

struct Circle
{
    float radius;
    Point middle;

    void MoveTo(const Point &target);
    Point MiddlePoint() const;
};

This way, all calculations for a specific structure is localized, and easier to find.

Overloaded functions are making the code harder to understand. When number of such structures is high, then number of functions is high as well. Finding the correct one can be challenging.

share|improve this answer
    
Does C++ have inheritance? It looks like both of those could inherit MoveTo from a shared parent class. –  ckuhn203 Jul 11 at 10:21
    
@ckuhn203 Yes, but why? Those structures are PODs, and those functions are specific (and different) for each structure. –  BЈовић Jul 11 at 11:14
    
Maybe I'm missing something, but it looks like the MoveTo method in this answer is "copy paste" code. There's nothing different about them. –  ckuhn203 Jul 11 at 12:00
1  
@ckuhn203 What are you talking about? What is copy paste code? The bottom code is the suggested signature of the structures. Those methods would still need implementation. –  BЈовић Jul 11 at 12:05
2  
@ckuhn203: In C++ it is possible to use duck typing interfaces without a common base class (it is called static polymorphism). Introducing inheritance hierarchies with virtual functions is to be avoided until they are truly needed. –  Nobody Jul 11 at 13:51
show 1 more comment

If we need to add some functionality to to these, does something like below make sense?

In some contexts, it probably does (though I can't think of any). It does look (to me) like a case of the "I will write C in any language" syndrome.

I would find this implementation cumbersome and obscure, especially since the equivalent canonical code is much simpler:

struct Point
{
    float x;
    float y;
};

struct Figure { virtual void move(const Point& abspos) = 0; }
// TODO: add virtual destructor to Figure

struct Square: Figure
{
    Point topLeft;
    float width;
    void move(const Point& abspos) override { topLeft = abspos; }

    inline Point middle() const
    {
        float halfWidth(width/2.0f);
        return Point{
            square.topLeft.x + halfWidth,
            square.topLeft.y + halfWidth};
    }
};

struct Circle: Figure
{
    float radius;
    Point middle;
    void move(const Point& abspos) override { middle = radius; }
};

Here's an updated main:

int main() {

    Square aSquare;
    aSquare.topLeft = Point{1,1};
    aSquare.width = 3;

    Circle aCircle;
    aCircle.middle = Point{1,1};
    aCircle.radius = 2;

    // move(aSquare).to(Point{1,1});
    aSquare.move(Point{1,1});

    // move(aSquare).to(middleOf(aCircle));
    aSquare.move(aCircle.middle);

    // move(aCircle).to(middleOf(aSquare));
    aCircle.move(aSquare.middle());

    return 0;
}

Note: The algorithm is not equivalent to yours, but it's the interface/client code that looks interesting, not necessarily the implementation of the functions.

share|improve this answer
    
I'm just wondering what you reasons for thinking that the question code suffers from "I will write C in any language" syndrome? –  PeterSW Jul 11 at 18:50
    
@PeterSW, The "I can write C in any language syndrome" is a name I use for the attempt to force constructs in a language, to resemble constructs in another language, solely due to the familiarity level of the developer. –  utnapistim Jul 17 at 10:57
    
I think I understood what you meant, I'm more wondering why you think this is the case for the original code? I'm pretty sure it's impossible to write such a construct in C. It it because it doesn't use inheritance? –  PeterSW Jul 17 at 11:20
add comment

Object names in the form of aSomething are not only un-useful, but also prevent additional objects. Otherwise, you'll have anotherSomething and thirdSomething and so on.

Sure, it may still sound better in English, but it just makes your code messier and more restrictive. Although striving to have the code look more English-like is nice, you can still use looser naming that's still nicely readable by others.

Alternatives to this may depend on your design, as already mentioned by others. As good naming is already one of the hardest things to do in programming, it may be worth looking at similar designs to see how it's done by others.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.