Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flyweight #28

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Flyweight #28

wants to merge 6 commits into from

Conversation

pedrodaviddev
Copy link
Member

@pedrodaviddev pedrodaviddev commented Mar 30, 2017

Edit by @tonilopezmr: Solved #19

Implemented flyweight pattern. Maybe you are thinking... why are you merging with state branch???
This is because the great @tonilopezmr said that Flyweight can be implemented with State, so I make pull to see if this can be done. And I cant see the similaritys between these patterns so, here's my version of flyweight! So much love <3

import org.jetbrains.annotations.TestOnly

class SoldierFactory {
companion object {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this factory can be improved. It is so Java style...

val TYPE = 1
}
init {
Flyweight.objectInstances++
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dangerous ⚠️ If variable is public it can be overrided unsafely.

Cotel
Cotel previously approved these changes Mar 30, 2017
Copy link
Member

@Cotel Cotel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems okay overall 👍 Now you can look how to improve this code with Kotlin style 😉

Copy link
Member

@tonilopezmr tonilopezmr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -0,0 +1,19 @@
package oop.Flyweight

import java.awt.Point
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh my fucking god ❌


class Admiral : Soldier {
companion object {
val TYPE = 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOPE ❌

Flyweitght.objectInstances++
}

val attack = 6
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOPE ❌

}

val attack = 6
val salary = 23000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOPE ❌


class Captain : Soldier {
companion object {
val TYPE = 2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOPE ❌

@@ -0,0 +1,7 @@
package oop.Flyweight

import java.awt.Point
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOPE ❌ NOPE ❌ NOPE ❌ NOPE ❌ NOPE ❌

@@ -0,0 +1,13 @@
package oop.Flyweight

import java.awt.Point
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL data class Point(val x: Int, val y:Int)


class SoldierFactory {
companion object {
var admiral: Admiral? = null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kotlin? null? seriously? ❌

var admiral: Admiral? = null
var captain: Captain? = null

fun getSoldier(type: Int): Soldier{
Copy link
Member

@tonilopezmr tonilopezmr Mar 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getSoldier(type: Int) lol Do you know what is a sealed class? 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know*

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Cotel at least I can stop crying hard with this

return captain!!
}
}
throw IllegalArgumentException()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xDDDDD fann1

Copy link
Member

@tonilopezmr tonilopezmr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tonilopezmr
Copy link
Member

@Cotel Seems ok overall?

@Cotel
Copy link
Member

Cotel commented Mar 30, 2017

Seems okay if you want to treat Kotlin like oldstyle Java, thats why I told him to update his code 🤣

@Cotel
Copy link
Member

Cotel commented Mar 31, 2017

I don't understand anything of this pattern. I've been reading but it is still unclear to me. Can you explain it? I need to know how to review this before I get to it 😵

@tonilopezmr
Copy link
Member

The last one explains what is, in resume, when you have a data structs with duplicates, you remove all your object duplication and share with other objects.

In the @Nhemesy example, he wanted to have a Soldiers that if are the same "type" (same soldier) only create once because are two object with the same values.

@tonilopezmr
Copy link
Member

@Nhemesy What do you think? you need see the last changes.

We need merge this

@tonilopezmr
Copy link
Member

@Nhemesy please

@Cotel Cotel changed the base branch from state to master April 24, 2017 09:06
@tonilopezmr
Copy link
Member

Don't forget add Flyweight in README and complete with an example

@tonilopezmr
Copy link
Member

tonilopezmr commented May 13, 2017

@tonilopezmr

@Cotel

@Nhemesy

@Cotel
Copy link
Member

Cotel commented Jun 14, 2017

We are back! 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants