Few Code Smells - Githubissues

AniAdamPashut commented 4 months ago

Description

Disclaimer

This are my own opinions, if you don't like it don't use it and move on. This suggestions will probably trigger a refactor, maybe even a huge one. Keep in mind that this are the things that I found while scouting the code. I do not criticize the code or it's writers. However, I will point out stuff I don't like.

Few important notes

I have read the contributing.md file and it was not written as okay or to disregard.
Every point will be discussed in the "Concerned Code" section.
Parts that are minor and don't really affect the code just makes it look worse marked with [MINOR]. These should be semantic changes only meaning they would not affect the running code.
If you find reasons to ignore things listed here. Please tell me why.
I am against this interface driven architecture but due to the project depending on it I would disregard it.

Main Problems

Interfaces with property inside them. This one is a more personal take, but I would definitely use an abstract class here. This just breaks DRY and forces everyone to implement the public property inside the class. I'll provide examples. Another code smell that arises from this is the virtual properties. Also, Why would you need this? An abstract class would have solved it as you wouldn't need to override properties and you could've defined them as protected.
[MINOR] Inconsistencies when defining properties. In some places they are prefixed with public in other they are not. Just adds more text with no value that might even confuse some people. Especially in interfaces, The default access modifier
[MINOR] Weird Naming in some places

Concerned code

Properties inside interfaces

Let's talk about a core interface, The IStorable interface. Located at src/Files.Core.Storage/Storables/IStorable.cs sits the most important core interface of this project. This interface does not contain any methods, but it does contains properties. The reason C# allows properties inside interfaces are because properties are get-set methods behind the scenes. However, there is a better alternative to this. Allow me to introduce, The Abstract Class. Why does it matter?

First off all, properties are required to have a public getter, but no restrictions on it's setter. This means that every class that implements this interface have to define it for itself. Which leads into my second point, Code Repetition. Having properties inside interfaces causes massive code repetition. Every class that implements this interface will have to re-declare the property. This would also solve the virtual properties. Overriding a property seems kinda weird and is also redundant in some cases (like in the FTPStorable).

Access Modifier Inconsistencies [MINOR]

In some places you favor specifying the access modifier explicitly, in some other places you don't. Doesn't really matter what you choose, just pick one and go with it. Keep in mind, that the default access modifier inside interfaces is public (a private interface is kinda useless isnt it?) In the IStorable interface you have properties one with an explicit access modifer, the other - implicit. Both mean the same thing, the only outcome is confusion. It seems like in other interface you chose to remove the access modifier so I suggest to go with that.

IStorable

public interface IStorable
{
    public string Id { get; }
    string Name { get; }
}

Weird Naming [MINOR]

When going over the project I had hard time understanding the naming. I'm not gonna go over the IStorable whatever that means. Even some folders have unrelated or misleading names. In the Files.Core.Storage project inside the Storables directory, you have a directory named ExtendableStroage What does that even mean? The interfaces inside it are called IFileExtended. There is a difference between and extendable file and an extended interface for a file. I still don't know which is which.

Gains

Better code readability
Better code clarity
Better practices

Requirements

Going over the code and manually changing stuff. To be honest, might be too hard.

Comments

No response

0x5bfa commented 4 months ago

Thank you for the feedback! I'm only the person who work on this implementation as of now so let me respond your concerns.

The current implementation has some problems including what you mentioned here. My latest research and idea is in #8974, which is also still under heavy construction and there's still a lot of room to improve but is much better.

Interfaces with property inside them. Code Repetition

Every storable item has its unique name to identify. This is not limited just to files and folders but also properties and any other storable objects on file systems. Regarding ID, I think we don't need it and I'd remove in the future.

Regarding the code repetition, IStorable is the greatest farther of any storable objects. And file system classes NativeStorable (natively supported file systems such as NTFS and FAT32), FtpStorable, SftpStorable and ArchiveStorable (example of virtual storage which doesn't exists in file system) implement it. There aren't so many. My preference of using abstract is when I put actual value in abstracted properties from methods in abstract class. I think your concern also can be said to abstract methods and abstract properties that forces descendants to implement.

Regarding storage properties (IStorageProperty/IStorageProperties) such as bitrate and git commit message text, which is still not even implemented. I completely agreed. There're so many properties that I can't force descendants to implement. Therefore, my latest opinion is to just have a method to retrieve either a group of properties or single properties (IStorageProperties.GetProperty, IStorageProperties.GetProperties).

Access Modifier Inconsistencies

I recognize this issue. We should make all public objects internal in Files.App, and public be public in other projects. All objects have to have explicit modifier and it shouldn't exist in interfaces. Regarding this, everyone can open a PR and we'll review.

Weird Naming

I realize your concerns. I'll think of better names.

Merging with #4180 and #8974. Our implementation bases on OwlCore.Storage project which can be found on GitHub.

AniAdamPashut commented 4 months ago

Thank you for your fast reply. I just want to clarify regarding the abstract properties. When I mentioned abstract classes I was talking about an abstract class containing a field, not an abstract property. This would not force the child to re-implement and will define a standard.

abstract class Parent 
{
    public string MyProp { get; protected set; }
}

class Child : Parent
{
    // doesn't need to re-declare MyProp
}

interface IBehavior
{
    string MyProp { get; }
}

class BehaviorParticipant : IBehavior
{
    public string MyProp { get; private set; } // Has to re-declare the property AND declare the setter
}

Just hit me up if it's intentional or not.