4
\$\begingroup\$

I have designed and Online Book Reader System as considering the previous reviews on this platform. You can find the classes below. I would be appreciated for the valuable reviews.

Assumptions: "Online Book Reader System" is a system that includes online books and serves its customers to read books online. Users should be login to reach their accounts. The system can include many users and many books. Multiple users can access the same book. Users can add any books to their reading list. After users log in to the system they can start to read any book they want and also can proceed with the latest page they have resumed.

Book.java

package oopdesign.onlinebookreader;

public class Book {

    private long bookId;
    private String name;
    private String category;
    private String author;
    private int pageCount;

    public Book(String name, String category, String author, int pageCount){
        this.bookId = name.hashCode();
        this.name = name;
        this.category = category;
        this.author = author;
        this.pageCount = pageCount;
    }

}

BookProgress.java

package oopdesign.onlinebookreader;

public class BookProgress  {

    User user;
    Book book;
    int  resumedPage;

    public BookProgress(Book book, User user) {
        this.book = book;
        this.user = user;
        this.resumedPage = 0;
    }

    public void setResumedPage(int resumedPage) {
        this.resumedPage = resumedPage;
    }

    public int getResumedPage() { return resumedPage;  }

    public void pageForward(){
        resumedPage++;
        setResumedPage(resumedPage);
    }

    public void pageBackward(){
         resumedPage--;
        setResumedPage(resumedPage);
    }

    public int startReading() {

        int resumedPage =  this.resumedPage;

        for(int i=0;i<50;i++){
            pageForward();
        }

        System.out.println("Started reading");
        return resumedPage;
    }

    public void  finishReading(){
        System.out.println("Finished reading at "+ resumedPage);
    }

}

Library.java

package oopdesign.onlinebookreader;

import java.util.ArrayList;
import java.util.List;

public class Library {

    List<Book> library;

    public Library(){
        library = new ArrayList<>();
    }

    public void addBook(Book book){
        library.add(book);
    }

    public List<Book> getBookList(){
        return library;
    }

}

OnlineReaderSystem.java

package oopdesign.onlinebookreader;

import java.util.List;

public class OnlineReaderSystem {

    private Library library;
    private UserManager userConsole;
    private BookProgress progress;

    public OnlineReaderSystem() {
        userConsole = new UserManager();
        library = new Library();
    }

    public static void main(String[] args) {

        OnlineReaderSystem onlineReaderSystem = new OnlineReaderSystem();

        // Create user
        User userNes = new User("Nesly", "Nesly","Password");

        onlineReaderSystem.userConsole.addUser(userNes);

        List<User> userAllList = onlineReaderSystem.userConsole.getAllUsers();

        for(User u: userAllList){
            System.out.println(u.getName());
        }

        // Create book
        Book bookFiction = new Book("Fiction Book", "Fiction", "James",320);

        onlineReaderSystem.library.addBook(bookFiction);

        // User login
        userNes.login("Nesly","password");

        // Start reading book

        onlineReaderSystem.progress = new BookProgress(bookFiction, userNes);

        onlineReaderSystem.progress.startReading();

        onlineReaderSystem.progress.finishReading();

        int page = onlineReaderSystem.progress.getResumedPage();

        System.out.println(page);
    }

}

User.java

package oopdesign.onlinebookreader;

import java.time.LocalDateTime;
import java.time.format.DateTimeFormatter;
import java.util.Date;

public class User {

    private long userId;

    public String getName() {
        return name;
    }

    public String getSubcriptionType() {
        return subcriptionType;
    }

    public Date getSubsciptionDate() {
        return subsciptionDate;
    }

    private String name;

    private String subcriptionType;
    private Date   subsciptionDate;

    private String loginUserId;
    private String loginPassword;
    private String lastLoginDate;

    private String creditCardInfo;

    public User(String name, String loginUserId, String loginPassword) {
        this.userId = name.hashCode();
        this.name = name;

        this.subcriptionType = "Classic";
        this.loginUserId = loginUserId;
        this.loginPassword = loginPassword;
    }


    public void login(String loginUser, String login_Password){

        if(this.loginUserId.equals(loginUserId) && this.loginPassword.equals(login_Password)) {
            System.out.println("Welcome " + name);
            DateTimeFormatter dtf = DateTimeFormatter.ofPattern("yyyy/MM/dd HH:mm:ss");
            LocalDateTime now = LocalDateTime.now();
            lastLoginDate = dtf.format(now);
        }else {
            System.out.println("Unsuccessful login  " + name);
        }

    }

}

UserManager.java

package oopdesign.onlinebookreader;

import java.util.ArrayList;
import java.util.List;

public class UserManager {

    List<User> users;

    public UserManager(){
        users = new ArrayList<>();
    }

    public void addUser(User user){
        users.add(user);
    }

    public List<User> getAllUsers(){
        return users;
    }

}
\$\endgroup\$
1
  • \$\begingroup\$ There's a convention for Java package names, aiming to avoid collisions. Packages should begin with a reversed domain name that you own or something similar, e.g. com.stackexchange.neslihanbozer.onlinebookreader. \$\endgroup\$ Jul 24 '20 at 7:56
3
\$\begingroup\$

bloater pt 1

on my first glance i saw that two classes are not required - the name for that anti-pattern is called Middle Man:

  • Library
  • UserManager

you can easily refer directly to these List<?> (Note: if you provide more function to these classes, maybe like a filter, then these classes would make totally sense - it also would have a name then: first-class collection. Since it is not (yet) needed it also violates YAGNI

bloater pt 2

the User class is bloated - it has too much responsibility

extract these responsibilites into the proper classes

//TODO class Subscription
private String subcriptionType;
private Date   subsciptionDate;
...
public String getSubcriptionType() 
public Date getSubsciptionDate()
...
//TODO class LogIn
private String loginUserId;
private String loginPassword;
private String lastLoginDate;
...
public void login(String loginUser, String login_Password)
...

some other minor issues

  • magic numbers for(int i=0;i<50;i++)... - why 50?
  • primitve Obsession (why not use an enum for subcriptionType - that would make turn "Classic" into it's proper value)
  • primitve Obsession - String creditCardInfo - a class would make sense here (especially because you have to treat these information with special care)
  • hardcoded Strings - "Started reading", "Finished reading at "
  • use a test class instead of a main() test
\$\endgroup\$
1
\$\begingroup\$

@Martin Frank has given you a few things to think about, so I'm only going to add a couple of points. Firstly, use consistent formatting it makes a difference for readability. Most IDE's will do this for you automatically. As it stands, you have some method signatures that have spaces between ) { and some that don't ){.

Secondly, your Book class has a bunch of private fields, that don't have getters, or any methods that operate on them other than the constructor that set's them. Consider not adding fields until they're actually needed, it'll save you time in the long run.

Finally, consider these methods:

public void pageForward(){
    resumedPage++;
    setResumedPage(resumedPage);
}
public void setResumedPage(int resumedPage) {
    this.resumedPage = resumedPage;
}

Your pageForward increments the resumedPage field then calls the properties setter to set the field to the value that the field already has. You don't need to call setResumedPage in this scenario, since resumedPage already has the same value...

\$\endgroup\$
1
  • \$\begingroup\$ I didn't see that, you are a good observer! +1 \$\endgroup\$ Aug 1 '20 at 13:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.