Player Move Command

[Ted Gress]

I'm trying to update my player's coordinates in my player class and for some reason its not updating. Its stored in a hashmap. I'm not sure if I have to do a *put* back into the hashmap or what the issue is, but the coordinates aren't updating.

Code:

	public static void PlayerMoveTo(int x, int y)
	{
		CJournal.Journal(GeneralCommands.class, "Moving player position to x: " + x + " y: " + y);
		WSFirstPersonPlayerCharacter player = Main.webServiceFactory.retrieveFirstPersonCharacterInstance();
		player.x = x;
		player.y = y;
		CJournal.Journal(GeneralCommands.class, "New player x is " + player.x  + " y is " + player.y);
		Main.webServiceFactory.StoreFirstPersonCharacterInstance(player);
		
	}

This method above updates the player.x and player.y that it gets from the Main.webServiceFactory.retrieveFirstPersonCharacterInstance() method.

public WSFirstPersonPlayerCharacter retrieveFirstPersonCharacterInstance()
	{
		WSFirstPersonPlayerCharacter w = (WSFirstPersonPlayerCharacter)wsobjectRegistry.get("WSFirstPersonPlayerCharacter");
		return w;
	}

The method above retrieves the CreateFirstPersonPlayerCharacterInstance from a hashmap called wsobjectRegistry.

public static WSObject launchWebServiceInstance(String instanceName) {
		Element e = null;
		
		if(instanceName.equals("WSFirstPersonPlayerCharacter")) {
			
			WSFirstPersonPlayerCharacter c = new WSFirstPersonPlayerCharacter(e);
			wsobjectRegistry.put("WSFirstPersonPlayerCharacter", c);			
			return c;
			
		}
		
		if (instanceName.equals("VectorCameraInstance")) {
			return new WSObject(e);
		}

		
		if (instanceName.equals("BoxInstance")) {
			return new WSObject(e);
		}
		
		return null;
	}

The code above creates the characterinstance object and puts it in the hashmap.

And...

public class WSFirstPersonPlayerCharacter  extends WSObject {

	 public WSFirstPersonPlayerCharacter(Element e) {
		super(e);
		// TODO Auto-generated constructor stub
		e = new Element("WSFirstPersonPlayerCharacter");
		
		//default start hexagon
		
		hexagon.height = 200;
		hexagon.width = 300;
		hexagon.lower_right_x = 200; 
		hexagon.lower_right_y =0; 
		hexagon.midleft_x = 0; 
		hexagon.midleft_y = 100;
		hexagon.upper_left_x = 100;
		hexagon.upper_left_y = 200;
		hexagon.lower_left_x = 100;
		hexagon.lower_left_y = 0;
		hexagon.upper_right_x = 200;
		hexagon.upper_right_y = 200;
		hexagon.midright_x = 300;
		hexagon.midright_y = 100;
		
		//default start position
		
	}

	public CHexagonCoordinates hexagon = new CHexagonCoordinates();
	
	//position in x,y
	
	public int x = 150;
	public int y = 150;
	
}

The above class is the object that is being stored in the hashmap.

Sorry if this is too complicated but it is how I want the architecture to work for what I'm doing.
So anyway, the move command isn't updating the player object.

[Liutauras Vilda]

Welcome to the Ranch.

1. Your x and y parameters do not convey the idea of your mentioned coordinates. Consider having coordinates or location class.
2. Why you let directly modify x and y having them declared as public?
3. launchWebServiceIntance() and all those new WSFirstPersonPlayerCharacter(e). Why you passing a null reference to its constructor?
Also you have checks for VectorCameraInstance and BoxInstance, regardless, you create same object instance, which appear to be WSObject

Don't know why it isn't saving, you haven't showed us StoreFirstPersonCharacterInstance(player);. Why method start with an upper case?

I'd say you need to re-think your architecture and look for improvements as your methods do more than they supposed to do.

Consider this method:

public static void PlayerMoveTo(int x, int y)
{
    CJournal.Journal(GeneralCommands.class, "Moving player position to x: " + x + " y: " + y);
    WSFirstPersonPlayerCharacter player = Main.webServiceFactory.retrieveFirstPersonCharacterInstance();
    player.x = x;
    player.y = y;
    CJournal.Journal(GeneralCommands.class, "New player x is " + player.x  + " y is " + player.y);
    Main.webServiceFactory.StoreFirstPersonCharacterInstance(player);
     
}

So in the first print you say you are moving player to x and y. Then you have another print which says New player x and y... exactly the same, what is the intent? Why this player is called new? Are you trying to confirm it was successfully moved? But you would need to confirm after storing player object to ensure the task was accomplished fully, not before that.

I'd look for refactoring to achieve better design. Difficult to discuss about that as to me looks very cryptic.

[Junilu Lacar]

Ted Gress wrote:... this is too complicated but it is how I want the architecture to work for what I'm doing

That's like saying "Sorry if this hurts but I really want to feel pain."  If a code or design is starting to be too complicated that means that you're already in trouble and you should back up and try to simplify things a little.

Programmers are an optimistic bunch though and they often think they can muddle through the pain and just get done. The (irrational) rationalization is often that it's going to take more time to go back and fix things than it will to just bite the bullet and power through to the end. Things hardly ever work out and most people who tried to do that end up biting off their own tongues instead.

So, take it from folks who've been there, done that. Go back and refactor. Simplify. And most of all, clarify.

[Junilu Lacar]

This is particularly smelly.

Ted Gress wrote:

	
public static void PlayerMoveTo(int x, int y)
{
   ...    
   player.x = x;
   player.y = y;
   ...		
}

And...

public class WSFirstPersonPlayerCharacter  extends WSObject {
   ...
	
   //position in x,y
	
   public int x = 150;
   public int y = 150;
	
}

You have created the x and y fields of this object as global variables. This is antithetical to Object-Oriented Programming and in particular, the principles of encapsulation and data hiding.

A more object-oriented way to design this is:

   player.moveTo(x, y);

   // or perhaps better yet
   player.moveTo(new Point(x, y));

And

public class WSFirstPersonPlayerCharacter  extends WSObject {
   ...
   private Point currentLocation;

   // or
   // private int x;
   // private int y;

   public void moveTo(Point newLocation) {
       currentLocation = newLocation;
   }

   // or
   // public void moveTo(int newX, int newY) {
   //     this.x = newX;
   //     this.y = newY;
   // }
}    

[Junilu Lacar]

This has a strong smell of confusion:

public static WSObject launchWebServiceInstance(String instanceName) {
   ... // nothing whatsoever that remotely resembles "launching" anything
}

The method signature says one thing, "this method launches a web service instance using the given name" but there's no code in there that does anything remotely resembling that. All the code in there does is create some kind of object and return it or return null. What exactly is being launched? How do you define launch? Putting an object into a map hardly qualifies as "launching". Besides, you only put the object you create into the map if the instanceName is a specific value. Any other value of the parameter does nothing but return a new kind of object or null. That's inconsistent with what the method name implies at best.

This tells me that you, the designer of this code, are yourself confused as to what you're doing in there.

Again, back up and think this over. What was your intent? How do you clearly relay that intent in your code? What names and statements can you write that tell a story that is n line with that intent?

[Junilu Lacar]

You might be wondering right now, "Why don't these guys just help me with the problem I asked about instead of lecturing me about what's wrong with my design?"

Well, for one thing, when we see problems that are as glaring as the ones your design has, it's hard to see past that. It's like a kid coming up to us saying, "Mister, can you help me button my shirt?" and we notice that he's torn his jeans and is bleeding profusely from a cut on his knee.

The other thing is that you really haven't given us enough information about this moving problem. How is this PlayerMoveTo() method being called? What values are you passing to it? In another thread, you've already made the mistake of passing in the wrong values. Is this another case of mistaken identity? We won't know unless you tell us how you were calling this method. Even then, we still might not know enough because of the problem with global fields that I pointed out earlier. There could be multiple places where you're accessing those public fields directly. Unless we see the entire flow of execution and point out where there might be conflicts in accessing and setting those global filed values, we'd still not have the entire view that will allow us to pinpoint exactly where the bug is.

By the way, the method name PlayerMoveTo flouts Java convention for naming methods. Method names should start with a lowercase letter. Methods that start with a capital letter can be confusing to read because at a passing glance, they look like constructors. You have to do a double take and see the return type clause to realize that it's really a method that's not following normal naming conventions.

[Junilu Lacar]

I wrote:The other thing is that you really haven't given us enough information about this moving problem. ... we'd still not have the entire view that will allow us to pinpoint exactly where the bug is.

Also, what made you say "it doesn't work"? Just saying "It doesn't work" is not very helpful.

Setting the values of the global fields that you defined is pretty straightforward and there's not much that can go wrong there. What else were you expecting to happen that didn't happen? Is there some kind of "redraw" method that is executed after setting the X and Y values?

[Liutauras Vilda]

Small point about the hexagon dimensions within your WSFirstPersonPlayerCharacter. But first, I find this class name confusing and unnecessarily too long. It has some fancy bits in it, which makes class name look and carry too much information. For instance, what extra information Character gives? What extra information WS gives? To me as a reader, latter gives absolutely nothing as I'm not familiar with game, but that is maybe particular domain's identifier. Let's assume we are left with FirstPersonPlayer. How first and second and might third players differ? Understood, it defines the perspective of the view you see. Maybe Player(Perspective.FIRST_PERSON...). Would the third and first person view perspectives would differ in what player can do, its characteristics? Might not worth having this FirstPerson in its name at all. Just dropping an idea which might is worth researching further. But I'm not about that.

How you ensure you don't mess up with hexagon dimensions? Maybe it is worth calculating at least height and width automatically? As far as I understand, this hexagon defines shape of the player character?

[Stephan van Hulst]

It looks like you have a web service that keeps track of a turn based game between two remote players. Most web application frameworks allow you to inject data you need into the classes that control the data. Assuming that your application can handle multiple concurrent games between different players, when a player sends a request to make a move, they need to authenticate themselves and identify the game they want to make a move for. The application can then retrieve a model of the game from a repository. All of this is done using instance fields and methods, no static fields.

For concrete examples, we first need to know what web application framework you're using in your web service.

[Ted Gress]

Hi,

I think it is unfair to criticize my architecture when parts of it are not complete (launchWebService for example) and the entire span of the architecture isn't posted.
With that said, the PlayerMoveTo command is called from a shell and is there for debugging. I guess a Point class passed in would be more OO but the act of
doing so seems to be a waste of resources to me, considering it is two coordinates and that is all. In my previous thread I used a HexagonCoordinate class because
of the OO issues, being that HexagonCoordinate takes in 6 vertices, I did not feel that that level of encapsulation was necessary for PlayerMoveTo.

When I commented it may be a bit complicated, I did so meaning that for the scope of the code that I posted it may be hard to follow - that is, not seeing
the entire picture. I did not mean it was complicated to code or use. Its actually quite simple. You need an object, and the factory returns it. That's it.

For the issue with global variables, I see nothing wrong with using global variables (technically they aren't global variables, which by definition would
be outside of the class - not possible in java - but are instance variables) if they are encapsulated within the class. Tell me, what is wrong with that
besides it might not be OO enough for you. Where would I store these coordinates if not inside the class header? These are long term variables. I can't
put them in a method, they aren't static, so what is the problem with storing them in a class as "global" variables?

My knee is far from bleeding. Your advice/criticism is misplaced. You frequently commented on my architecture and minor things when I had a technical
question regarding, basically, the HashMap and how it stores information. I needed to know if I create an object (or get one created for me which
the factory does) and I modify it and put it back in the HashMap will it store the new data. Not make sure to camel case your method names or
your architecture is garbage.

-TW

[Liutauras Vilda]

It seems waste of time to discuss any further regarding improving basic things (as naming methods properly or variables or giving them appropriate access modifiers), so let's discuss about the direct things you are asking.

Ted Gress wrote:I needed to know if I create an object (or get one created for me which
the factory does) and I modify it and put it back in the HashMap will it store the new data.

import java.util.HashMap;

public class Test {

    public static void main(String[] args) {
        InHashMap inHashMap = new InHashMap();
        System.out.println("Value of object while it isn't in hash map " + inHashMap.x);

        HashMap<String, InHashMap> test = new HashMap<>();
        test.put("key", inHashMap);
        System.out.println("Object value after putting to hash map " + test.get("key").x);

        inHashMap.x = 4;
        System.out.println("Changed object's value while object in hash map " + inHashMap.x);

        System.out.println("Object value in hash map after changed value of object outside hash map " + test.get("key").x);
    }
}

class InHashMap {
    public int x = 0;
}

That should answer to your question. Of course you were able to check that yourself since you know it seems a lot yourself already.

[Dave Tolls]

As Liutauras shows (as I just noticed the reply as I'm typing this!), if you change the state of an object retrieved from a Map, then that is the same object as the one held in the Map, so any further retrieval will reflect this state change.
So no need to put the object back in the map.
If the state is not being reflected then you might need to debug the code and check exactly where the state change is being lost.

As for comments about camel case, a standardised way of writing code in a language makes it a lot easier for people to read that code.  This covers naming conventions, and things like indentation.

This also goes with things like static methods, and public variables.  These are red flags to a lot of us as they often signal a bug in the making.

[Stephan van Hulst]

If you think we're criticizing your architecture because we get a kick out of it, you're mistaken. Well... maybe we do, but the primary reason is that we want you to learn. Learning by receiving comments on actual real life code that you wrote is much easier than learning through contrived examples that you can't relate to.

There's a reason why we hammer on OO principles so much. Many aspects of OO programming are in place to prevent common issues that occur in code that is designed without OO principles in mind. Using globally accessible state is one of these things that can completely wreck your application in the long run. You can not easily guarantee that a piece of code that uses global state works correctly, because you have to test the ENTIRE application in all its possible permutations of configuration settings. Fixing bugs in such code, or adding new features to such code invariably leads to spaghetti.

As for why a Point object to encapsulate an x and a y coordinate is not a waste of resources, each method that accepts the loose coordinates must check that they are in the valid range before they do anything. You do check your parameters, don't you? If you don't, you will run into some incredibly hard to debug problems later on. A Point object will guarantee that the coordinates are in range, and all that's left to do is validate that you don't pass null where a point is expected.

Have you written unit tests for your application, or for other projects?

I'm sorry you don't appreciate our advice. Many people are willing to pay good money for this kind of tutoring. If you want help in a more straightforward Q&A format, I suggest you have a look at StackOverflow.

[Indigo Montes]

Don't feel too bad about our criticism. We just comment on your code, not on you personal. Stephan already pointed out why we do that.

[Junilu Lacar]

Stephan and others have already explained our motivations here quite eloquently.

I just want to add that it's natural to react defensively to criticism, especially if you've put a lot of work into something. However, what may seem simple in your head is often not so to others who are seeing your work for the first time and from a different perspective. That, too, is a red flag you should learn to spot. Writing code is just as much about helping others understand what you're thinking as anything else. So, don't make it about you. Learn to practice egoless programming.

[Liutauras Vilda]

Ted Gress wrote:...if I create an object (or get one created for me which
the factory does)...

1. Think carefully if it is reasonable to return null from your factory method. That adds extra complexity to your program's flow path, because every time you call it, you need to check if returned value isn't null. Consider whether throwing an exception would be a better approach. i.e. NotSupportedException

2. Also in case you forgot, you have two if statements to create the same object, you could combine them into one and give your code reader a clue, that this is your actual intent rather than copy/paste error. I have in mind 2nd and 3rd if's which create a WSObject.

3. One more thing. With the same factory method. For the case of WSFirstPersonPlayerCharacter object creation, you put that to a Map before you return it, in the further cases you returning right away. Consider whether you're not mixing different ideas. Factory methods usually create objects of the same kind, thinking how to express that in words..., they are creating objects which are equal in their importance. Now looking to your factory, it seems like WSFirstPersonPlayerCharacter has some higher importance than WSObject, because latter doesn't need to be registered (putted into map). So your might need two factory methods, where each of those would return objects of the same kind of significance. Maybe Characters factory method and game Inventory factory.

[Junilu Lacar]

Ted Gress wrote:I'm trying to update my player's coordinates in my player class and for some reason its not updating. Its stored in a hashmap. I'm not sure if I have to do a *put* back into the hashmap or what the issue is, but the coordinates aren't updating.

No, you don't need to call put() again, unless you want to change the key that you're using to map to the object or change object properties that are involved in calculating equals(). The Map only holds references to the objects you put into it. There is no difference between operating on an object via an external reference vs. a reference you obtained by calling the get() method of a Map, you're still affecting the same exact object.

If anything, there is probably something else going on that you missed, either having to do with the values you think you are using but are not, the interactions between objects you think are happening but are not or vice versa, or a combination of these and/or other things. But I highly doubt it's because you didn't put an object back into the Map after changing its properties that are not related to the map key or have any part in determining equals() for that object.

[Junilu Lacar]

Ted Gress wrote:
For the issue with global variables, I see nothing wrong with using global variables (technically they aren't global variables, which by definition would
be outside of the class - not possible in java - but are instance variables) if they are encapsulated within the class.

See http://wiki.c2.com/?GlobalVariablesAreBad for a laundry list.

Technically, anything that is declared as public in a class is globally accessible, therefore, any class member variable (aka field) that is declared as public is, in fact, a global variable. If that field is also declared as final, thus making it immutable, then there's no problem (Edit: as long as that variable is a primitive type or a reference to an immutable object, like String. Otherwise, you do have problems). The problems that are explained in that wiki page start when globally accessible variables of a class/object are mutable. Then you have broken encapsulation and when you break encapsulation in an OO design, there are many serious consequences.

So, when you declare fields like this:

public class Foo {
    public int x;
    public int y;

    public void bar() {
        // do something with x and y
    }
}

you have broken encapsulation and opened yourself up to a whole mess of problems.

[Stephan van Hulst]

Junilu Lacar wrote:Technically, anything that is declared as public in a class is globally accessible.

This is not true. Public instance members are only accessible by code that already have a reference to the instance. By globally accessible state I mean any non-final field that can indirectly be mutated through a static member. A private static non-final primitive field is one such example. An object can't make any guarantees about its value, because other objects can mutate it freely.

The static keyword should almost never be used for fields if those fields are non-final or of a mutable reference type. The only exception I would make to this rule are cross-cutting concerns that are intrinsically related to the class object itself, for instance, when having static references to loggers. Ideally, a language has built in language support for such concerns.

[Junilu Lacar]

Stephan van Hulst wrote:

Junilu Lacar wrote:Technically, anything that is declared as public in a class is globally accessible.

This is not true. Public instance members are only accessible by code that already have a reference to the instance. By globally accessible state I mean ...

Ah, gotta be careful when there are smart programmers in the audience  

I wouldn't say "it's not true" - that's kind of like saying it's not true that my local park is a publicly accessible place because people in Norway can't access it. True, it would be more precise to add on the qualification that you need to have a reference to an instance to access publicly declared instance members but I don't think that lack of precision in my statement makes it entirely false.

I was, however, referring to OP's assertion (which I quoted) that global variables can only be declared outside a class, which of course is impossible in Java.

[Campbell Ritchie]

I think what Junilu means is that public fields default to being accessible throughout the package, or everywhere if the class is public. Of course, you can some trick to restrict accessibility of instances.

[Ted Gress]

Thanks for the help. And yeah, instance variables within a class aren't globally accessible. You need a reference to the class unless they are declared static. Then they are class variables.

My main question was about the HashMap. Thanks for the answer. I'll go ahead and go over my code again. I'm not sure what you mean by the multiple ifs...? I'm not sure where in the code you are referring to. The factory returns one object per method within the factory class. So I'm not sure what you were talking about there either....

I don't think the encapsulation is really broken. My factory class has static methods but I think that is for obvious reasons. The important objects related to the state of the application are stored as objects - as returned from the factory.

Is there anything else I didn't reply to?

[Campbell Ritchie]

Ted Gress wrote:. . . And yeah, instance variables within a class aren't globally accessible. You need a reference to the class unless they are declared static. . . .

I think you have misunderstood what we were saying about non‑private fields. They default to being accessible outside the class, and depending on the combinations of access modifiers, they can be globally accessible. Remember most classes which can be instantiated can be instantiated from outside their own package, so it is possible to obtain an instance anywhere. Unless you have an agreement about non‑private variables, they are prone to interference from elsewhere.
I think you mean reference to an object.

[Liutauras Vilda]

Ted Gress wrote:And yeah, instance variables within a class aren't globally accessible. You need a reference to the class unless they are declared static. Then they are class variables.

Nevermind, that is more of a terminology. In JavaScript they are called globals. It doesn't change the fact that you have them public, meaning can be modified without you knowing by other classes. And that is bad - but you have been told that already trice, so it is up to you either you try to back-up yourself with some mysterious arguments or you give them more appropriate access modifier as guys suggested earlier.

Ted Gress wrote:I'm not sure what you mean by the multiple ifs...? I'm not sure where in the code you are referring to.

By mentioning multiple if's I meant exact place where multiple if's appear, you don't have many places where you have multiple if statements, in fact, you have only one place where you have if statements at all - so go line by line in your code and you might find it.

Ted Gress wrote:The factory returns one object per method within the factory class. So I'm not sure what you were talking about there either....

I think fancy terminology you have learned is the biggest stopper for you to go succesfully further with this project. WebServices, Factory methods, non-broken encapsulation... By the way, factory class there is no such thing in the terminology you are refering to.

I personally don't see anything special in your project yet as just a typical game. Which in my opinion requires improvements before moving onto further tasks.

Ted Gress wrote:Is there anything else I didn't reply to?

No, not that I know.