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

Refactoring Document.php #428

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

Conversation

agungsantoso
Copy link

I refactored Document.php.
Changes already commented on the file.

@agungsantoso
Copy link
Author

agungsantoso commented Sep 3, 2016

Change in Document.php:

  • Separating Document.php into two files, so each class has it's own file
    • Why? Because by separating each class to its own file, make it easier to find and also reducing the file size of each class
  • Adding comment to file, class and method
    • Why? Because by adding the comment, make it easier for others to read and understand the file, class and method.
  • Adding new class properties $database and Initialize database at class constructor
    • Why? Because by doing this, we can use database throughout the class.
  • Adding constructor parameter $database
    • Why? I avoid using static access because static access causes unexchangeable dependencies to other classes and leads to hard to test code. So instead of calling Database::getInstance() every time needed, I make it as a constructor parameter.
  • Moving query to different line
    • Why? Because it makes the line too long.
  • Changing getAllDocuments method to not static
    • Why? I avoid using static access because static access causes unexchangeable dependencies to other classes and leads to hard to test code.

Change in User.php:

  • Adding new class properties $document and Initialize document at class constructor
    • Why? Because by doing this, we can use document throughout the class.
  • Using document constructor instead of init method
    • Why? Because init is not needed since we have a constructor.

@agungsantoso
Copy link
Author

Change in Parser.java:

  • Change file and class name from Parser to FileContent
  • Adding package declaration
    • Why? Because it's needed
  • Adding Javadoc to class and method
    • Why? Because by adding the comment, make it easier for others to read and understand the file, class and method.
  • Making class final
    • Why? Because class has only private constructors
  • Adding private constructor
    • Why? Because I'm not providing any instance members, so we don't need them to create instances
  • Make all method static except constructor
    • Why? Because non-static method must contain at least one reference to this
  • Make all variable that not need to be changed later to final
    • Why? Because I want the compiler to prevent a variable from being re-assigned to a different object and ensure a variable always points to the same object
  • Rename some variables that are too short
    • Why? Because variable name should have pattern '^(id|[a-z]{3,12})$'
  • Assigning magic number 0x80 to datalimit variable
    • Why? Because 0x80 is a numeric literal that should be defined as a constant

@agungsantoso
Copy link
Author

If the API is not fixed, I'd like to change:

  • in Parser.java
    • Rename class name from Parser to ParsedFile. That's because class name should be an abstraction of a real life entity.
    • Instead of using the setter, I remove method setFile and create a class constructor that take the file as an argument and set file class properties at the constructor. Then I rename getter getFile to fileOf. Because it is conceptually incorrect to have any methods starting with set or get in an object.
    • Create content method with an argument to return ASCII or Unicode content, then overload content method without argument to return ASCII content and create unicodeContent method to return Unicode content. This will remove code duplication, so we get cleaner code.
    • Rename method saveContent to save. Because the name will be shorter and explain what it does.
  • In Document.php
    • Rename method getTitle to title, getContent to content and getAllDocuments to all. The reason is same as above, to avoid any methods starting with set or get.
    • Rename class name from User to UserDocument, which is a better name to represent the class, when we look at the whole class.
    • Rename method makeNewDocument to create and getMyDocuments to listOf, that's because If a method returns something, then its name should explain what it returns.

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

Successfully merging this pull request may close these issues.

1 participant