palra / univ-mmorpg Goto Github PK

When the game begins, each character has a knife and a helmet

https://github.com/palra/univ-mmorpg/blob/feature/serialization/src/main/java/fr/univdevs/mmorpg/engine/character/Inventory.java#L150-L155

The methods should return the result of this.items.put

https://github.com/palra/univ-mmorpg/blob/feature/serialization/src/main/java/fr/univdevs/mmorpg/engine/character/Character.java#L13-L14

I think that MAX_RESISTANCE and MAX_HEALTH constants are bad, because it implies that the engines decides what the gameplay should be like. This is a bad thing : an engine should not make assumptions on the gameplay that the final user want to implement.
(this is not a major issue, so it can be delayed after the due date)

https://github.com/palra/univ-mmorpg/blob/develop/src/main/java/fr/univdevs/mmorpg/bridge/FightCommand.java#L22-L38

This throws CommandException, what if I need to throw an InvalidArgumentException ?

From frature/drattak

I think Inventory should know for which character it is created

For the moment, moving a character of one case on the map costs 1 action point. When do we check that the character has enough AP to move ? In the MoveCommand, or in the MoveAction ?
It seems more logical to do it in the MoveCommand, as the user can immediately know that he can't move anymore, but in that case, we have to pre-calculate the path that the character will take, meaning a new public method in World, and we still can't predict the fact that a player (that theorically can be collidable, but not always, it depends if he has redefined the isCollidable method) can cross the pre-calculated path, so the promise we did to the player that we could move him is broken.

Entity#getDisplay returns an ANSIChar, meaning that to be able to be registered in the world, the entity must have implemented the way he has to be displayed on a terminal. If we want to render it with JFrame, this method becomes unusable. We have to move out the display strategy of the entity interface, so implementing classes, like Character, can be used in any graphical context. But where do we move it ? How do we finally implement different rendering strategies ?

https://github.com/palra/univ-mmorpg/blob/feature/serialization/src/main/java/fr/univdevs/mmorpg/engine/GameManager.java#L39-L49

Also rename saveTo to saveToFile

https://github.com/palra/univ-mmorpg/blob/feature/serialization/src/main/java/fr/univdevs/mmorpg/engine/character/Inventory.java#L174-L182

In Java, HashMap<K,V>#containsValue takes an Object in parameter, even it its generic form, so we should follow the rules.
See : http://docs.oracle.com/javase/7/docs/api/java/util/Map.html#containsValue(java.lang.Object)

We should also change the remove method so it takes an Object in parameter too, as HashMap<K,V>#remove(Object)

• has(Item) -> has(Object)
• remove(Item) -> remove(Object)

See https://github.com/palra/univ-mmorpg/blob/feature/serialization/src/main/java/fr/univdevs/mmorpg/engine/character/Inventory.java#L109-L121

Also rename Item#getTypes() to Item#getCategories() : https://github.com/palra/univ-mmorpg/blob/feature/serialization/src/main/java/fr/univdevs/mmorpg/engine/character/Inventory.java#L123-L134

In Item.java, empty methods onRegister(Character c) and onUnregister(Character c) are missing, and I can't compile the project. so I add them.

For the moment, we don't clone the attributes of the class we clone. Should we do this ?
We will, then, have to implement the Cloneable interface, in order to have a polymophic clonable behavior.

How do we specify the cure we want to use ?
Now, we type the ID, but that's not user-friendly

It is not the role of Inventory to add an event, but the responsability of actions, for instance the MoveAction for InventoryAddEvent

https://github.com/palra/univ-mmorpg/blob/feature/serialization/src/main/java/fr/univdevs/mmorpg/engine/character/Item.java#L146-L166

The speed of a character is linked to the weight of the items he has on it. So, we can set the speed of a given Character from here, as every item has a weight. The child classes, then, may have to call super.onRegister(Character)

You should refactor class fr.univdevs.mmorpg.engine.event.FightEvent.FightEvent to fr.univdevs.mmorpg.engine.event.action.FightEvent. Not only FightEventis an invalid package name (it should be in lowercase), but it doesn't respect the event package naming convention set at #5.

For the moment, apply method is defined only in Cure : https://github.com/palra/univ-mmorpg/blob/feature/serialization/src/main/java/fr/univdevs/mmorpg/engine/character/item/Cure.java#L65-L72
Why wouldn't we move this method to item, allowing every item to apply a given effect on a given Character ? This method should not be abstract, as onRegister and onUnregister, allowing the possibility to an child item to not redefine this method, if it doesn't use it.

In Protection.java :

    /**
     * Protection constructor
     * A protection is something the character carries to improve his defence
     *
     * @param protectionName       the name //TODO remove the name var
     * @param protectionCategory   the Category of the item //TODO remove from constructor, here category is Protection
     * @param protectionCost       the cost
     * @param protectionWeight     the Weight, is added to character's weight
     * @param protectionRobustness is equivalent to efficiency of the protection
     */

Why do you want to remove the protectionName parameter ?
And you don't have to remove protectionCategory, as it will be used by the children of this class.

Before the deadline, we need to clean the code and respect the same code style :

• Use this when accessing attributes inside a class
• Use iterators when browsing a Collection (List, Map ...)
• Use explicit loop conditions instead of returning inside of them : while(it.hasNext() && !hasFound) for example

https://github.com/palra/univ-mmorpg/blob/feature/serialization/src/main/java/fr/univdevs/mmorpg/engine/character/Item.java#L41-L48
The weight argument should be explicit, for readability purposes

There's two ActionEvent classes in code, different packages, logger and game, which one do we keep ?

We should stop put events in inner classes, for more readability. Also, it is more easier for a future developer to find the events classes in a dedicated package than searching on a class. Inner classes should be used if we really need to access inner attributes of the parent class, but that's not the case, as we pass them in argument.

My proposal is to create a package event in our working package (for example : fr.univdevs.mmorpg.engine.event) and put subpackages for each topic (fr.univdevs.mmorpg.engine.event.action)

Skill package is useless

https://github.com/palra/univ-mmorpg/blob/feature/serialization/src/main/java/fr/univdevs/mmorpg/engine/character/Inventory.java#L233-L236

Isn't that a (bad) duplicate of Inventory#getByType(String) ?

https://github.com/palra/univ-mmorpg/blob/feature/serialization/src/main/java/fr/univdevs/mmorpg/engine/action/CureAction.java#L26

Create a NotInInventoryException instead of throwing a IllegalArgumentException.

We need to kill, and stop the game when a character is dead

https://github.com/palra/univ-mmorpg/blob/feature/serialization/src/main/java/fr/univdevs/mmorpg/engine/character/Item.java#L64-L71
Putting acces on the static arary of ids is useless and not secure, as anybody could add and remove ids from the array

https://github.com/palra/univ-mmorpg/blob/develop/src/main/java/fr/univdevs/mmorpg/engine/world/World.java#L159-L171
It is not used anymore

Methods to change :

• addEntity
• removeEntity
• getEntity
• getEntities
• isCollidableAt

https://github.com/palra/univ-mmorpg/blob/feature/serialization/src/main/java/fr/univdevs/mmorpg/engine/GameManager.java#L24-L29

https://github.com/palra/univ-mmorpg/blob/feature/serialization/src/main/java/fr/univdevs/mmorpg/engine/action/CureAction.java

CureAction should accept only Characters that implements CanCure. The same with FightAction and CanFight. It should check it at the constructor, and associated exception should be created in the skills package

• CureAction(), CanCure
• FightAction(), CanFight

The package logger should not be a part of the engine, just a dependency of it.
Move the logger package to fr.univdevs.logger