okyanus-mc / runtime

Okyanus server runtime
Apache License 2.0
4 stars 1 forks source link

OOP'ye aykırı ve yanlış kodlar #2

Closed portlek closed 5 years ago

portlek commented 5 years ago
Admicos commented 5 years ago

Class isimlerinde ki Impl. ler silinip yerine daha uygun isimler getirilmeli.

ServerImplementation, PlayerImplementation türünden kullanıyorum Impl'ı bana göre uygun ama senin önerilerini de merak ediyorum

ServerImpl.class class'ın da getInternal() methodu kaldırılmalı. Ayrıca Type casting ler de kaldırılmalı bu class da.

getInternal'i neden kaldırıyorsun acaba, merak ettim. Artı olarak, bu class'daki tek type casting (MinecraftServerLoggable), Mixinlerin büyük bir limitini geçebilmek için kullanılıyor. O yüzden kaldırıp ne ile değiştireceğine fazla emin değilim, ama bir fikrin varsa beklerim.

Static methodlar kaldırılmalı onun yerine objeler getirilmeli, her şey önceden belirlenmiş ve dizayn edilmiş olması gerek, dependency injection kullanılmalı.

DI konusunda haklısın, onu yapmam lazım bir ara. Ama static'leri neden kaldırman gerekiyor onu da merak ettim. Basit, argüman alıp bir şey döndüren, kendi içinde hiç bir etkisi olmayan şeyler için uygun bence.

PlayerImpl.class class'ın da Type casting ler kaldırılmalı cotr.'lar da. 82. satır da görüldüğü üzere aynı isimde ki class'ların isimleri değiştirilmeli, karmaşaya yol açıyor.

O type cast'leri neler ile değiştirebiliriz ki? Ve artı olarak, güvenli değil diye bir şey olduğunu sanmıyorum, ki biri bizim runtime'ımızı kullanıyorsa, her zaman bizim ServerImpl'ımız oraya gelmeli.

Enum class'ları kullanmamalıyız, bunun static kullanmaktan bir farkı yok. Bunun yerine dediğim gibi objeler kullanılmalı.

Enumları, bir şey listelemek gerektiğinde kullanıyorum sadece, öyle kendi içlerinde etki falan yok. O yüzden bunu pek anlamadım, açıklayabilirsen sevinirim

Interface'ler de default methodlar ya da her hangi bir static method bulunmamalı. bkz. Server interface'i

Dependency Injection getirirsek onlar da gidecek büyük ihtimalle zaten.

Annotation'lar kaldırılmalı.

Hangileri, tam olarak?

Null kesinlikle kullanılmamalı, command class'larında bolca mevcut.

Peki bir şeyin bulunmadığını göstermek için ne yapmalıyız? Exception falan atarsak kullanıcının kodu null kullandığımız kadar temiz olacak mıdır? Bir annotation kütüphanesi çekip Nullable annotation'u falan koyup javadoclarda da uyarsak yeter bana göre.

Disappointing paketinde ki class oldukça gereksiz yerine sabit olan, static ve null olmayan field'lar getirilmeli ve bunlar dependency ile aklatılmalı.

Disappointing pakedi direkt olarak gitmeli. O paketten nefret ediyorum

Builder class'ları olmamalı, yerlerine daha sağlıklı gerçek class'lar getirilmeli ve kapsülleme tekniği kullanılmalı.

Buna örnek verebilir misin? Builder'ların ne yanlışı var merak ettim.

Class isimleri -er -ar şeklinde bitmemeli.

Buna katılıyorum, fakat ne ile değiştirebiliriz ondan emin değilim işte.

Mod interface i yerine bir class olması daha iyi olucaktır, dependency injection'ı daha rahat yapabilmek adına. Bu olmazsa sürekli type casting ve static instance'ler ile uğraşılıcak ve bu çok daha kötü.

Bu, Fabric Loader'ın bir limiti diye biliyorum. Sponge mesela DI'ı Inject annotationları ile yapıyor, ona da bakaibliriz belki

Mod interface'i aynı bu şekilde olması gerekiyor; https://github.com/utsukushihito/core/blob/master/api/src/main/java/io/github/utsukushihito/core/api/Core.java her şeyin önceden belirlenip protected/public final yapılıp, runetime ve diğer test modlarında extends Core şeklinde yapılması gerekiyor, bu şekilde olmayınca annotation lar ile yapılıyor ve annotation kullanılımı genel olarak oop'ye çok ters ve hiç bir şekilde neler olup bittiğini anlayamıyorsun Mod interface'in de.

İlla "pure oop" kullanacağız diye bir şey yok açıkçası, annotation'lar benim gözümde hiç sıkıntılı değil.

portlek commented 5 years ago

Class isimlerinde ki Impl. ler silinip yerine daha uygun isimler getirilmeli.

ServerImplementation, PlayerImplementation türünden kullanıyorum Impl'ı bana göre uygun ama senin önerilerini de merak ediyorum

  • OkyanusServer, OkyanusPlayer, OkyanusEntity gibi.

    ServerImpl.class class'ın da getInternal() methodu kaldırılmalı. Ayrıca Type casting ler de kaldırılmalı bu class da.

getInternal'i neden kaldırıyorsun acaba, merak ettim. Artı olarak, bu class'daki tek type casting (MinecraftServerLoggable), Mixinlerin büyük bir limitini geçebilmek için kullanılıyor. O yüzden kaldırıp ne ile değiştireceğine fazla emin değilim, ama bir fikrin varsa beklerim.

  • getInternal'i kaldırmak değil asıl olay aslında, bu methodu kaldırıp o method'a ihtiyaç duyan diğer class'ların işlevlerini server class'ına taşımak, yani get/set olaylarını kaldırmak tam olarak yapmak istediğim. bunu yapmak o kadar kolay değil ancak bunun en başta kodlara başlamadan önce tasarlanmış olması gerektiğini düşünüyorum, eski kodları düzeltmek için onların üstüne yeni kodlar eklemek pek akıllıca olmaz bu durumda ve muhtemelen o şekilde kalması gerekicek.

Static methodlar kaldırılmalı onun yerine objeler getirilmeli, her şey önceden belirlenmiş ve dizayn edilmiş olması gerek, dependency injection kullanılmalı.

DI konusunda haklısın, onu yapmam lazım bir ara. Ama static'leri neden kaldırman gerekiyor onu da merak ettim. Basit, argüman alıp bir şey döndüren, kendi içinde hiç bir etkisi olmayan şeyler için uygun bence.

  • olay tam olarak oop ye uygun olup olmaması ile alakalı, "arguman alıp bir şey döndüren ve kendi içinde hiç bir etkisi olmayan şeyler" bu tam olarak oop ye zıt bir görüş.

PlayerImpl.class class'ın da Type casting ler kaldırılmalı cotr.'lar da. 82. satır da görüldüğü üzere aynı isimde ki class'ların isimleri değiştirilmeli, karmaşaya yol açıyor.

O type cast'leri neler ile değiştirebiliriz ki? Ve artı olarak, güvenli değil diye bir şey olduğunu sanmıyorum, ki biri bizim runtime'ımızı kullanıyorsa, her zaman bizim ServerImpl'ımız oraya gelmeli.

  • type casting'ler genel olarak kodu hard-code'a doğru iter, ilerde farklı bir ServerImpl gerektiğinde ya da her hangi bir şekilde o method ordan kaldırıldığında o type casting'İn hiç bir işlevi olmayacak. asıl olay type casting'te hard-code stilini arttırması, bu da oop ye zıt bir stil.

Enum class'ları kullanmamalıyız, bunun static kullanmaktan bir farkı yok. Bunun yerine dediğim gibi objeler kullanılmalı.

Enumları, bir şey listelemek gerektiğinde kullanıyorum sadece, öyle kendi içlerinde etki falan yok. O yüzden bunu pek anlamadım, açıklayabilirsen sevinirim

  • class'ları obje deposu olmaktan kurtarmak için yapılması gereken bir işlem. class'lar içinde veri saklanan bir yapı değil yalnızca behavior'ı olan max. 3-4 tane methoddan fazla method olmayan yapılar şeklinde tanımlanır, bu sebepten ötürü enumlar tüm class-obje ilişkisini bozuyor. tüm olay tamamen oop ye aykrı olması, object-pool kullanılabilir, ancak bunu yapıcak bir alt yapı gerekiyor, sadece olmaması gerekn/yanlış kısımları gösteriyorum.

Annotation'lar kaldırılmalı.

Hangileri, tam olarak?

  • Mod interface'inde kullanılan functional annotation gibi

Null kesinlikle kullanılmamalı, command class'larında bolca mevcut.

Peki bir şeyin bulunmadığını göstermek için ne yapmalıyız? Exception falan atarsak kullanıcının kodu null kullandığımız kadar temiz olacak mıdır? Bir annotation kütüphanesi çekip Nullable annotation'u falan koyup javadoclarda da uyarsak yeter bana göre.

  • objelerin dünyasında "olmayan bir şey" diye bir şey olmaz, yani her şey önceden oluşturulmuş ve kullanıma hazır olması gerekiyor bu sebepten ötürü "null" olmamalı yani. her null kullanıldığında npe atma olasılığı o kadar artıyor ve arttıkça kodun güvenilirliği de bir o kadar da azalıyor.

Builder class'ları olmamalı, yerlerine daha sağlıklı gerçek class'lar getirilmeli ve kapsülleme tekniği kullanılmalı.

Buna örnek verebilir misin? Builder'ların ne yanlışı var merak ettim.

  • builder mantığı gereğinden fazla karmaşık objeler oluşturur, bunları constructor'da çözebiliriz, daha sonradan değişmişcek şeyleri oluştrurmak için mesela komutlar için, constructor'da oluşturmak daha mantıklı.

Class isimleri -er -ar şeklinde bitmemeli.

Buna katılıyorum, fakat ne ile değiştirebiliriz ondan emin değilim işte.

  • -ed ile bitebilir Manager ise Managed gibi, objeler başkalarını değil kendilerini yönetir ve bunu yaparken de zaten atıyorum parser olmuş şeyleri kullanır yani obje aslında parser değil parsed'dır.
Admicos commented 5 years ago

OkyanusServer, OkyanusPlayer, OkyanusEntity gibi. Direkt Bukkit'ten özenelim diyorsun yani, (CraftServer, CraftWorld, ...)

getInternal'i kaldırmak değil asıl olay aslında, bu methodu kaldırıp o method'a ihtiyaç duyan diğer class'ların işlevlerini server class'ına taşımak, yani get/set olaylarını kaldırmak tam olarak yapmak istediğim. bunu yapmak o kadar kolay değil ancak bunun en başta kodlara başlamadan önce tasarlanmış olması gerektiğini düşünüyorum, eski kodları düzeltmek için onların üstüne yeni kodlar eklemek pek akıllıca olmaz bu durumda ve muhtemelen o şekilde kalması gerekicek.

Bu dediğin aslında mantıklı. Player constructor'unda getInternal.getPlayerManager yapacağımıza, Server'da getPlayerByName yapabiliriz. Bunu yapmak o kadar kolay değil diyorsun da dediğin metodun sadece tek bir class'da kullanımı var. (PlayerImpl)

olay tam olarak oop ye uygun olup olmaması ile alakalı, "arguman alıp bir şey döndüren ve kendi içinde hiç bir etkisi olmayan şeyler" bu tam olarak oop ye zıt bir görüş.

Hm. Ben biraz functional/declarative mantığıyla gidiyorum static metod'lare. Ama full OOP gidelim diyorsan, Onu da yapabiliriz sanırım.

type casting'ler genel olarak kodu hard-code'a doğru iter, ilerde farklı bir ServerImpl gerektiğinde ya da her hangi bir şekilde o method ordan kaldırıldığında o type casting'İn hiç bir işlevi olmayacak. asıl olay type casting'te hard-code stilini arttırması, bu da oop ye zıt bir stil.

O type castları API kullanıcılarına herhangi bir internal class göstermeden silebilirsen bir PR beklerim, ki OOP mantığında düşününce dediklerin doğru geliyor.

class'ları obje deposu olmaktan kurtarmak için yapılması gereken bir işlem. class'lar içinde veri saklanan bir yapı değil yalnızca behavior'ı olan max. 3-4 tane methoddan fazla method olmayan yapılar şeklinde tanımlanır, bu sebepten ötürü enumlar tüm class-obje ilişkisini bozuyor. tüm olay tamamen oop ye aykrı olması, object-pool kullanılabilir, ancak bunu yapıcak bir alt yapı gerekiyor, sadece olmaması gerekn/yanlış kısımları gösteriyorum.

Yani Blocks.REDSTONE_BLOCK yerine class RedstoneBlock extends Block yapalım diyorsun. API kullanıcılarına internalleri göstermeden yapabilirsek bu da mantıklı olabilir.

Mod interface'inde kullanılan functional annotation gibi O annotation'u Fabric kendi interface'inde kullanıyor diye koymuştum, fazla neden göremiyorum ama onun için

objelerin dünyasında "olmayan bir şey" diye bir şey olmaz, yani her şey önceden oluşturulmuş ve kullanıma hazır olması gerekiyor bu sebepten ötürü "null" olmamalı yani. her null kullanıldığında npe atma olasılığı o kadar artıyor ve arttıkça kodun güvenilirliği de bir o kadar da azalıyor. Aslında, Java 8'de Optional'lar var, onları kullanabiliriz sanırım null döndüreceğimiz yerlerde.

builder mantığı gereğinden fazla karmaşık objeler oluşturur, bunları constructor'da çözebiliriz, daha sonradan değişmişcek şeyleri oluştrurmak için mesela komutlar için, constructor'da oluşturmak daha mantıklı.

Komutların mesela ne kadar argümanı, ne kadar alt komudu olacağı bilinmiyor, ondan builder olarak yaptım onu, ki teknik olarak builder olduğunu bile sanmıyorum, getXyz metodları var içerisinde.

-ed ile bitebilir Manager ise Managed gibi, objeler başkalarını değil kendilerini yönetir ve bunu yaparken de zaten atıyorum parser olmuş şeyleri kullanır yani obje aslında parser değil parsed'dır.

EventManaged, CommandManaged? Bunlar biraz garip durmuyor mu? Aslında Registry yapabiliriz onları, ki kullanıcıların yaptığı tek şeyler register ve get.


Bu arada, sitede yeni bir sayfa açtım projeye katkı yapmak ile ilgili, ilgini çekebilir. https://okyanus-mc.github.io/tr/dev/okyanus/contributing.html

En önemli şey, sonraki issue'larını İngilizce açarsan güzel olur. Bunlarda Türkçe konuşabilirsin ama hala.

portlek commented 5 years ago

Bir yapı oluşturuyorsun, aynı bir oyuncuyu oluşturur gibi;

new Player(
    new Human(
        new LivingEntity(
            new Entity(..)
        ) 
    )
);

biraz karmaşık gibi ancak, komutlar yaratması oldukça zevkli ve göze hoş gözüküyor, tasarımsal olarakta yeterince iyi gibi

Admicos commented 5 years ago

Flutter'ın UI sistemi gibi bir şeyi komutlara (ve benzer şeylere, envanterlere falan) yapabilsek çok güzel olur aslında, Öyle bir şey yapılabilir mi sence?

portlek commented 5 years ago

flutter'I inceledim, güzel gözüküyor, benim de zaten yapmak istediğim o şekil bir şey, bir yapı oluşturmak için gereken ekipmanlar/class'laır geliştiricinin elinin altına verip, onlarla komutlar/entityler vb. şeyler gibi yapıları istediği şekilde oluşturmak, başlamadan önce bunun tasarımı yapılması gerekiyor öncelikle. geliştiriciler komutları nasıl oluşturucak gibisinden, eğer bi üstte ki CorePlugin.java class'ında 23. satırda ki gibi/onun bir class'a dökülmüş hali gibi yapılabilirse oldukça kullanışlı ve kolay kullanımlı bir sistem olabilir, bence yapılması mümkün. dediğim gibi en önemli olan nasıl kullanılacağı ile ilgili tasarımı yapmak bence.

Admicos commented 5 years ago

Hm. Öyle bir şey için planlama evet gerekecek. Arka planında hiç bir implementation olmayan, sadece API kısmı gibi bir şey hazırlayıp onun üzerinden gitmek mantıklı olabilir.

portlek commented 5 years ago

ayrıca geliştiricilerin reflection'lara ihtiyaç duymaması için, geliştiricilere tüm araçları açmamız gerekiyor, reflection'dan pek haz etmiyorum, oldukça sağlıksız bir olay, bukkit'te de kanser ediyordu zaten kullanmasıdır her sürüm farklı bişi olmasıdır vs.

Admicos commented 5 years ago

Onu da düşünüyorum aslında, sorun Minecraft'ın büyüklüğü ile alakalı. O kadar büyük bir APIyi bir, veya iki kişi halledemeyiz diye düşünüyorum.

(Aslında Fabric Discord'undan da bir kaç yardımcı gelebilir, ama ondan önce bir şey çıkartmamız lazım ortaya)


Ekstra olarak, elimizde reflection'dan daha büyük bir araç var (Mixinler) ve modlar/pluginler onları da kullanabilecek. Onlara da dikkat etmemiz gerekebilir belki.