Adding YamlSerializer Interface #33

SherifWaly · 2017-01-25T20:02:40Z

This PR is for #30

coveralls · 2017-01-25T20:07:08Z

Coverage remained the same at 93.413% when pulling 3a1b12b on SherifWaly:#30 into 1678d1d on decorators-squad:master.

SherifWaly · 2017-01-25T20:08:05Z

@amihaiemil Please review the code and tell me if I misunderstood something about the issue and any other comments :D

coveralls · 2017-01-26T01:36:52Z

Coverage remained the same at 93.413% when pulling 9bba59a on SherifWaly:#30 into 1678d1d on decorators-squad:master.

coveralls · 2017-01-26T01:36:52Z

Coverage remained the same at 93.413% when pulling 9bba59a on SherifWaly:#30 into 1678d1d on decorators-squad:master.

amihaiemil

@SherifWaly I reviewed the PR and I have few comments :)

amihaiemil · 2017-01-26T07:23:21Z

src/main/java/com/amihaiemil/camel/YamlSerializer.java

+
+/**
+ * A Yaml serializer.
+ * @author sherif


@SherifWaly Here it should be your name and email address between parentheses (see how it's in other classes)

amihaiemil · 2017-01-26T07:23:39Z

src/main/java/com/amihaiemil/camel/YamlSerializer.java

@@ -0,0 +1,39 @@
+package com.amihaiemil.camel;


@SherifWaly Add here the copyright notice (take it from other files)

amihaiemil · 2017-01-26T07:25:10Z

src/main/java/com/amihaiemil/camel/YamlSerializer.java

+     * @return Yaml node
+     * @TODO #30 Implement the serialize method with Map input and unit-test it.
+     */
+    YamlNode serialize(final Map<Object, Object> map);


@SherifWaly The interface should have only one method: YamlNode serialize();. There will be 3 implementors of this interface. One implementor class will encapsulate a Map, another will encapsulate a Collection and the 3rd will encapsulate an Object.

e.g.

YamlSerializer objectToYaml = YamlObjectSerializer(object); YamlNode mapping1 = objectToYaml.serialize(); YamlSerializer mapToYaml = YamlMapSerializer(map); YamlNode mapping2 = mapToYaml .serialize(); YamlSerializer collectionToYaml = YamlCollectionSerializer(collection); YamlNode sequence= collectionToYaml .serialize();

Also, each implementor will override the return type of the method (you can do that, as long as the overriding type is a subtype of the initial one). E.g. YamlCollectionSerializer will have the method YamlSequence serialize(); - that is possible because YamlSequence extends YamlNode

The way you wrote it is also a way of doing it, of course, but a class with these 3 methods would just be a bag of functions, I don't think it would encapsulate anything. An object should always encapsulate something - if it just has some utility methods, it is not a proper object, that's what I think.

amihaiemil · 2017-01-26T07:43:28Z

src/main/java/com/amihaiemil/camel/YamlSerializer.java

+     * Serialize a Map<Object, Object> to YamlNode.
+     * @param map Map<Object, Object>
+     * @return Yaml node
+     * @TODO #30 Implement the serialize method with Map input and unit-test it.


@SherifWaly The format of the puzzle is almost correct, but not entirely. The letters todo need to be lowercase, and after #30, you need to add the estimation and role: :30m/DEV. Also, the text needs to be changed. One good puzzle would be:

@todo #30:30m/DEV This interface should have 3 implementor classes: YamlObjectSerializer, YamlMapSerializer and YamlCollectionSerializer. Each of these classes will encapsulate and serialize the mentioned types (i.e. Object, Map<Object, Object> and Collection<Object). The return type or method ``serialize()`` should be overridden with the proper subtype (e.g. YamlObjectSerializer will have the method ``YamlMapping serialize() {...}``

Notice that at the beginning of each new line, there is a space (it is important because otherwise, the bot won't parse it right). #30 is the parent ticket, while :30m/DEV is the estimation and role.

Now, of course, the puzzle asks for a lot of effort, which will take more than 30min, for sure, but this puzzle will be broken in more puzzles. Whoever will take it, will implement just one part of it (one of the classes), and will leave other puzzles in the code to cover the rest of the task. Makes sense? :)

coveralls · 2017-01-26T15:08:33Z

Coverage remained the same at 93.413% when pulling facd3f4 on SherifWaly:#30 into 1678d1d on decorators-squad:master.

coveralls · 2017-01-26T15:08:33Z

Coverage remained the same at 93.413% when pulling facd3f4 on SherifWaly:#30 into 1678d1d on decorators-squad:master.

SherifWaly · 2017-01-26T15:09:48Z

@amihaiemil please check the new commit :)

amihaiemil

@SherifWaly I got 2 comments. Let's discuss this a little, you gave me some ideas :D

amihaiemil · 2017-01-26T15:17:40Z

src/main/java/com/amihaiemil/camel/YamlSerializer.java

+ * proper subtype (e.g. YamlObjectSerializer will have the method
+ * ``YamlMapping serialize() {...}``
+ * 
+ * @todo #30:30m/TEST This interface should be tested by 3 unit tests: 


@SherifWaly This puzzle is not needed. :) It should be understood from the puzzle above that unit tests are also expected. I mean, with any implementation, a unit test is always required, right? How else do we know the new code works and doesn't break anything?

You are right for that but I thought it could be separated to make the puzzles smaller :D

amihaiemil · 2017-01-26T15:39:37Z

src/main/java/com/amihaiemil/camel/YamlSerializer.java

+ * of YamlNode (e.g. YamlObjectSerializer in objectYamlSerialize() test should
+ * return a proper YamlMapping)
+ * 
+ * @todo #30:30m/DEV New interface which is YamlPresenter should be implemented


@SherifWaly This is interesting. How did you think about this interface. How would it be used? :D
The specification says there are 3 stages: Representation, Serialization and Presentation. It is this diagram

@amihaiemil I opened the specification and found this diagram. I think the Serializer would create the Tree as each YamlNode has its children. So I think this interface would work on traversing this tree and creating a Presentation for this tree to be readable.

@amihaiemil Also should I add a puzzle for reversing this process for the other 3 stages: Construct, Compose and Parse ?

amihaiemil · 2017-01-26T16:32:18Z

@SherifWaly You got a good idea. There is only one misunderstanding (it's my fault).

The word "Serialization" from this interface had nothing to do with the serialization tree from the specification.

By "serializing" an object, map or collection, I simply meant "turn it into a YamlNode" (which, in fact, is the representation, the first step from the diagram).

Let's forget about this PR and clearly think about the "dumping" process (representing -> serialializing -> presenting). I think this would be a good way of doing it (turn a Student into a Yaml)

Student john = new Student ("John", "Doe", "Computer Science");
YamlMapping yaml = new Yaml(john).represent();
YamlMapping serialized = yaml.serialize();
String printed = serialized.present();

or, a shorter, more fluent version

Student john = new Student ("John", "Doe", "Computer Science");
String printed = new Yaml(john).represent().serialize().present();

What do you think? I think it's great because the user can choose to have the yaml serialized or not.
The way I see it, a YamlNode should have the method serialize() and present().

when you call serialize() on a YamlNode, it traverses itself (a YamlNode is itself a tree of other nodes)
when you call present() basically the YamlNode just preety-prints itself (we're not going to add comments and such, just alignment mostly)

Don't write any code, we're just discussing now ^^

SherifWaly · 2017-01-26T17:06:32Z

I think this a good idea as we will have several options for presenting the output. So know we should exchange YamlSerializer with YamlRepresenter which should do the same task and 2 extra interfaces will be as puzzles which are YamlSerializer (or another name as it will handle 2 options) and the YamlPresenter interface. I think you've meant that, right? :D

amihaiemil · 2017-01-26T17:27:52Z

@SherifWaly

we should exchange YamlSerializer with YamlRepresenter which should do the same task

Rename it YamlDump, it's better. And it would be used like this:

Student john = new Student ("John", "Doe", "Computer Science");
YamlDump yaml = new YamlObjectDump(john);
String printed = yaml.represent().serialize().present();

^ this will print the yaml serialized (with the duplicates replaced by aliases)
and if the user doesn't want that (which will usually be the case), he will just do

Student john = new Student ("John", "Doe", "Computer Science");
YamlDump yaml = new YamlObjectDump(john);
String printed = yaml.represent().present();

I don't think we need the other 2 interfaces. serialize() should be a method of YamlNode, while present() might even be missing (toString() could do it's job - print the YamlNode?).

Anyway, the direction sounds good. Let's do the representation and worry about serialization and presentation later.

SherifWaly · 2017-01-26T17:37:57Z

Ok, I will rename it to YamlDump and leave puzzles for the serialize() and present() methods

coveralls · 2017-01-26T18:13:26Z

Coverage remained the same at 93.413% when pulling 43fbcc0 on SherifWaly:#30 into 1678d1d on decorators-squad:master.

SherifWaly · 2017-01-26T18:14:59Z

@amihaiemil please check last commit and see if we could open issues for serializing and presenting stage.

amihaiemil

Just 1 comment

amihaiemil · 2017-01-26T18:14:47Z

src/main/java/com/amihaiemil/camel/YamlDump.java

+ * @since 1.0.0
+ * 
+ * @todo #30:30m/DEV This interface should have 3 implementor classes:
+ * YamlObjectDump, YamlMapDump and YamlCollectionDump.


@SherifWaly add a space at the beginning of each new line, otherwise the puzzels won't be well processed by the bot :)

@amihaiemil please check if it's fixed :)

coveralls · 2017-01-26T18:32:36Z

Coverage remained the same at 93.413% when pulling f13c6d1 on SherifWaly:#30 into 1678d1d on decorators-squad:master.

amihaiemil · 2017-01-26T18:35:05Z

@SherifWaly looks ok:)

amihaiemil · 2017-01-26T18:35:23Z

@rultor merge

rultor · 2017-01-26T18:36:19Z

@rultor merge

@amihaiemil OK, I'll try to merge now. You can check the progress of the merge here

rultor · 2017-01-26T18:37:49Z

@rultor merge

@amihaiemil Done! FYI, the full log is here (took me 1min)

SherifWaly added 2 commits January 25, 2017 21:32

Created YamlSerializer Interface

826391a

Added methods and puzzles for YamlSerializer

3a1b12b

SherifWaly added 2 commits January 26, 2017 03:18

Removed useless generic

6c607c0

Removed generic (Fixed checkstyle error)

9bba59a

amihaiemil reviewed Jan 26, 2017

View reviewed changes

Changed design to one method and added 3 puzzles.

facd3f4

amihaiemil reviewed Jan 26, 2017

View reviewed changes

Replaced YamlSerlializer by YamlDump interface.

43fbcc0

amihaiemil reviewed Jan 26, 2017

View reviewed changes

Fixed spaces.

f13c6d1

rultor merged commit f13c6d1 into decorators-squad:master Jan 26, 2017

SherifWaly deleted the #30 branch January 26, 2017 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding YamlSerializer Interface #33

Adding YamlSerializer Interface #33

SherifWaly commented Jan 25, 2017

coveralls commented Jan 25, 2017 •

edited

Loading

SherifWaly commented Jan 25, 2017

coveralls commented Jan 26, 2017

coveralls commented Jan 26, 2017

amihaiemil left a comment

amihaiemil Jan 26, 2017

amihaiemil Jan 26, 2017 •

edited

Loading

amihaiemil Jan 26, 2017 •

edited

Loading

amihaiemil Jan 26, 2017 •

edited

Loading

coveralls commented Jan 26, 2017

coveralls commented Jan 26, 2017

SherifWaly commented Jan 26, 2017

amihaiemil left a comment

amihaiemil Jan 26, 2017

SherifWaly Jan 26, 2017

amihaiemil Jan 26, 2017

SherifWaly Jan 26, 2017

SherifWaly Jan 26, 2017

amihaiemil commented Jan 26, 2017

SherifWaly commented Jan 26, 2017

amihaiemil commented Jan 26, 2017 •

edited

Loading

SherifWaly commented Jan 26, 2017

coveralls commented Jan 26, 2017 •

edited

Loading

SherifWaly commented Jan 26, 2017

amihaiemil left a comment

amihaiemil Jan 26, 2017

SherifWaly Jan 26, 2017

coveralls commented Jan 26, 2017 •

edited

Loading

amihaiemil commented Jan 26, 2017

amihaiemil commented Jan 26, 2017

rultor commented Jan 26, 2017

rultor commented Jan 26, 2017

Adding YamlSerializer Interface #33

Adding YamlSerializer Interface #33

Conversation

SherifWaly commented Jan 25, 2017

coveralls commented Jan 25, 2017 • edited Loading

SherifWaly commented Jan 25, 2017

coveralls commented Jan 26, 2017

coveralls commented Jan 26, 2017

amihaiemil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amihaiemil Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

amihaiemil Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

amihaiemil Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

coveralls commented Jan 26, 2017

coveralls commented Jan 26, 2017

SherifWaly commented Jan 26, 2017

amihaiemil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amihaiemil commented Jan 26, 2017

SherifWaly commented Jan 26, 2017

amihaiemil commented Jan 26, 2017 • edited Loading

SherifWaly commented Jan 26, 2017

coveralls commented Jan 26, 2017 • edited Loading

SherifWaly commented Jan 26, 2017

amihaiemil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 26, 2017 • edited Loading

amihaiemil commented Jan 26, 2017

amihaiemil commented Jan 26, 2017

rultor commented Jan 26, 2017

rultor commented Jan 26, 2017

coveralls commented Jan 25, 2017 •

edited

Loading

amihaiemil Jan 26, 2017 •

edited

Loading

amihaiemil Jan 26, 2017 •

edited

Loading

amihaiemil Jan 26, 2017 •

edited

Loading

amihaiemil commented Jan 26, 2017 •

edited

Loading

coveralls commented Jan 26, 2017 •

edited

Loading

coveralls commented Jan 26, 2017 •

edited

Loading