Alternatives to switches.

What would be more efficient than this? And by efficient I mean like how can i write a lot less code, and still have it do what I want.

Code:

	void move()
	{

		while(condition == 0)
		{
				direction = getch();
				switch(direction)
				{
				case 'w':
					if(w.y == 10)
					{
						break;
					}
					else
					{
						w.y++;
						break;
					}
				case 'a':
					if(w.x == -10)
					{
						break;
					}
					else
					{
						w.x--;
						break;
					}
				case 's':
					if(w.y == -10)
					{
						break;
					}
					else
					{
						w.y--;
						break;
					}
				case 'd':
					if(w.x == 10)
					{
						break;
					}
					else
					{
						w.x++;
						break;
					}

				}
					cout << w.x << ", " << w.y << endl;
		}
	}

Code:

if     (direction == 'w') w.y++;
else if(direction == 'a') w.x--;
else if(direction == 's') w.y--;
else if(direction == 'd') w.x++;
w.x = Bound(w.x, -10, 10);
w.y = Bound(w.y, -10, 10);

Where Bound() is a convenience function which looks like this:

Code:

template <typename T>
T Bound(T val, T min, T max)
{
    if(val < min) return min;
    if(val > max) return max;
    return val;
}

This Bound() function should come in handy all over the place, so it doesn't really even count against your line count.

w is for world. how do I keep the player and the world separate? Right now you see a function in player, move. How do I make the player class aware of the world class but keep them separate and encapsulated into different objects?

I like the bound template idea, simplifies the code needed to create limits in area's, i wonder how I would store different areas? Again keeping focus on the idea that the world and player are separate.

>> w is for world. how do I keep the player and the world separate?

Give each object methods with a single task to perform.

Player seems to own the point of location data, meaning the world should be required to access the player's location through the player interface, and draw the player on the map using one of its own methods.

Player's move() method updates the location data.

I quite like the switch variant, but it can be made much simpler:

Code:

	void move()
	{

		while(condition == 0)
		{
				direction = getch();
				switch(direction)
				{
				case 'w':
					if(w.y != 10)
						w.y++;
					}
					break;
				case 'a':
					if(w.x != -10)
					{
						w.x--;
					}
					break;
				case 's':
					if(w.y != -10)
					{
						w.y--;
					}
					break;
				case 'd':
					if(w.x != 10)
					{
						w.x++;
					}
					break;
				}
					cout << w.x << ", " << w.y << endl;
		}
	}

Of course, what brewbuck suggests is also fine. But the switch statement, in my mind, makes is more readable. By reversing the conditions and removing the unnecessary code, we get shorter, more manageable code.

--
Mats

And speaking just in terms of cosmetics, you could remove most of the brackets with the single-statement if-sections, and also put them on one line -- it still maintains readability:

Code:

case 'w':
   if (w.y < 10) w.y++;
   break;

Originally Posted by neandrake

And speaking just in terms of cosmetics, you could remove most of the brackets with the single-statement if-sections, and also put them on one line -- it still maintains readability:

Code:

case 'w':
   if (w.y < 10) w.y++;
   break;

Yes, you could do - but in my mind, braces go in whether they are needed or not. It prevents this type of problem:
Original code:

Code:

   if (w.y < 10) 
       w.y++;

Want to print w.y before change, so we add a prntf:

Code:

   if (w.y < 10) 
       printf("w.y = %d\n", w.y);
       w.y++;

Naturally, this will go horribly wrong.

It is an easy mistake to make, and I think you haven't been a programmer for long if you say "I've never done that". Using braces all the time makes that mistake a "no brainer" - and since doing things the same way all the time is a "no brainer" too, it makes life easier.

--
Mats

Coming back to the original problem, al alternative to the switch statement is (assuming w.x and w.y are of type int).

Code:

int delta = 1-2*(direction == 'a' || direction == 's');    // -1 for 'a' or 's', 1 otherwise
int &change((direction == 'a' || direction == 'd') ? w.x : w.y);
change = std::min(std::max(change + delta, -10), 10);

This has the effect of treating every letter other than 'a', 's', or 'd' as if it is 'w', and needs modification if you don't what that to happen.

Originally Posted by grumpy

Coming back to the original problem, al alternative to the switch statement is (assuming w.x and w.y are of type int).

Code:

int delta = 1-2*(direction == 'a' || direction == 's');    // -1 for 'a' or 's', 1 otherwise
int &change((direction == 'a' || direction == 'd') ? w.x : w.y);
change = std::min(std::max(change + delta, -10), 10);

This has the effect of treating every letter other than 'a', 's', or 'd' as if it is 'w', and needs modification if you don't what that to happen.

Never mind that most people won't actually understand the code, and if you want to add a 'h' for "hyperspace jump to random location" it will definitely make things difficult.

Edit: Added to that, I very much doubt the compiler will make better code out of it - it does one extra compare (because it does both max and min) compared to the original posted code, and the number of direction compares are equivalent.

--
Mats

I would generally replace such switches with some kind of map. For example:

Code:

#include <iostream>
#include <map>
#include <cstdio>
#include <algorithm>

class Character
{
    private:
        int x, y;
        static const int max_x = 10;
        static const int min_x = -10;
        static const int max_y = 10;
        static const int min_y = -10;
    public:
        Character(int x, int y): x(x), y(y) {}
        void move_by(int x_inc, int y_inc)
        {
            x += x_inc;
            y += y_inc;
            x = std::max(std::min(x, max_x), min_x);
            y = std::max(std::min(y, max_y), min_y);
        }
        int get_x() const { return x; }
        int get_y() const { return y; }
};

struct Point
{
    int x, y;
    Point(): x(0), y(0) {}
    Point(int x, int y): x(x), y(y) {}
};

typedef std::map<char, Point> KeyMap;

void move(Character& character, const KeyMap& key_map);

int main()
{
    Character character(0, 0);
    KeyMap keys;
    keys['a'] = Point(-1, 0);
    keys['d'] = Point(1, 0);
    keys['w'] = Point(0, -1);
    keys['s'] = Point(0, 1);
    move(character, keys);
}

void move(Character& character, const KeyMap& key_map)
{
    while(true)
    {
        char direction;
        std::cin >> direction;
        std::cin.ignore(128, '\n');
        KeyMap::const_iterator it = key_map.find(direction);
        if (it != key_map.end()) {
            character.move_by(it->second.x, it->second.y);
        }
        std::cout << character.get_x() << ", " << character.get_y() << std::endl;
    }
}

anon: Now that is the type of thing we like!
Make it flexible, and short/neat.

--
Mats

I suggest replacing this:

Code:

KeyMap keys;
keys['a'] = Point(-1, 0);
keys['d'] = Point(1, 0);
keys['w'] = Point(0, -1);
keys['s'] = Point(0, 1);

with:

Code:

KeyMap keys;
keys.insert(std::make_pair('a', Point(-1, 0)));
keys.insert(std::make_pair('d', Point(1, 0)));
keys.insert(std::make_pair('w', Point(0, -1)));
keys.insert(std::make_pair('s', Point(0, 1)));

since there is no point first default constructing a Point object and then copying over the desired Point object.

My version reads better, IMO, and the map is set up just once. It does explain, though, why I had to provide a default constructor to Point (outside of this demo one would be probably needed anyway) .

Originally Posted by matsp

Never mind that most people won't actually understand the code, and if you want to add a 'h' for "hyperspace jump to random location" it will definitely make things difficult.

Your point about understanding aside (that's subjective) I doubt the approach would make life all that difficult for adding other cases: just wrap in an additional if (direction != 'h') {the_code_above} else {...} and other cases can be accomodated.

Originally Posted by matsp

Edit: Added to that, I very much doubt the compiler will make better code out of it - it does one extra compare (because it does both max and min) compared to the original posted code, and the number of direction compares are equivalent.

I never claimed the compiler would create optimal output. The measure of efficiency of interest in the original post was reducing the amount of code to be entered by the programmer.

As a rough rule, techniques to achieve short code in C or C++ often involve a trade-off against understandability or run-time performance.

Originally Posted by laserlight

I suggest replacing this:

Code:

KeyMap keys;
keys['a'] = Point(-1, 0);
keys['d'] = Point(1, 0);
keys['w'] = Point(0, -1);
keys['s'] = Point(0, 1);

with:

Code:

KeyMap keys;
keys.insert(std::make_pair('a', Point(-1, 0)));
keys.insert(std::make_pair('d', Point(1, 0)));
keys.insert(std::make_pair('w', Point(0, -1)));
keys.insert(std::make_pair('s', Point(0, 1)));

since there is no point first default constructing a Point object and then copying over the desired Point object.

Of course that goes against the aim here which was to reduce the amount of code. (Yeah it's not necessarily a goal I fully agree with).