Code Review Asked by Neslihan Bozer on December 22, 2021
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;
}
}
@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...
Answered by forsvarir on December 22, 2021
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
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)
...
for(int i=0;i<50;i++)...
- why 50?enum
for subcriptionType
- that would make turn "Classic"
into it's proper value)String creditCardInfo
- a class would make sense here (especially because you have to treat these information with special care)"Started reading"
, "Finished reading at "
main()
testAnswered by Martin Frank on December 22, 2021
Get help from others!
Recent Answers
Recent Questions
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP