I'm making note where you can add items that you buy or sell, any suggestions?
public class Main {
private static Scanner in = new Scanner(System.in);
private static String commands = "[-1]Quit, [0]Commands, [1]Available items, [2] Sold items, [3]Add, [4]Delete,"
+ " [5]Edit, [6]Sell. [7]Bilans [8]Details";
public static void main(String[] args) {
Storage storage = new Storage(5);
storage.addItem();
System.out.println("| RESELL NOTE |");
System.out.println(commands);
boolean flag = true;
while (flag) {
System.out.println("Choose option: (0 - print list)");
int answer = in.nextInt();
switch (answer) {
default:
System.out.println("Wrong command");
break;
case -1:
System.out.println("QUIT");
flag = false;
break;
case 0:
System.out.println(commands);
break;
case 1:
storage.availableItems();
break;
case 2:
storage.soldItems();
break;
case 3:
storage.addItem();
break;
case 4:
storage.removeItem();
break;
case 5:
System.out.println("modify item");
break;
case 6:
storage.sellItem();
break;
case 7:
storage.bilans();
break;
case 8:
storage.details();
}
}
}
}
public class Storage {
private int maxCapacity;
public Storage(int maxCapacity) {
this.maxCapacity = maxCapacity;
}
Scanner in = new Scanner(System.in);
private Map<Integer, Item> items = new TreeMap<>();
public void availableItems() {
System.out.println("Available items:");
for (Map.Entry<Integer, Item> entry : items.entrySet()) {
if (!entry.getValue().sold) {
System.out.println(entry.getKey() + ". " + entry.getValue().getName());
}
}
}
public void soldItems() {
System.out.println("Sold items:");
for (Map.Entry<Integer, Item> entry : items.entrySet()) {
if (entry.getValue().sold) {
System.out.println(entry.getKey() + "." + entry.getValue().getName()
+ " - (" + (entry.getValue().soldPrice - entry.getValue().price + "PLN profit)"));
}
}
}
public void addItem() {
if (items.size() >= maxCapacity) {
System.out.println("You cant add more items, storage full! (" + items.size() + "/" + maxCapacity + ")");
} else {
items.put(Item.assignId(), new Shoes("Piraty", 1500, 7, "red", 11));
items.put(Item.assignId(), new Shoes("Belugi", 1500, 7, "red", 11));
items.put(Item.assignId(), new Shoes("Zebry", 1500, 7, "red", 11));
items.put(Item.assignId(), new Shoes("Creamy", 1500, 7, "red", 11));
items.put(Item.assignId(), new Shoes("Sezame", 1500, 7, "red", 11));
System.out.println("Item added");
}
}
public void modifyItem() { // work in progress
printInLine();
System.out.println("\nPick item to modify: ");
int id = in.nextInt();
if (items.containsKey(id)) {
System.out.println("Enter new name for " + items.get(id).getName());
in.nextLine();
String newName = in.nextLine();
items.get(id).setName(newName);
} else {
System.out.println("Item not found");
}
}
public void sellItem() {
printInLine();
System.out.println("\nChoose item to mark as sold: ");
int id = in.nextInt();
if (items.containsKey(id)) {
items.get(id).setSold(true);
System.out.println("How much did you get for " + items.get(id).getName() + "?: ");
items.get(id).setSoldPrice(in.nextInt());
System.out.println("You marked " + items.get(id).getName() + " as sold");
System.out.println("Your profit is " + (items.get(id).soldPrice - items.get(id).price) + " PLN");
} else {
System.out.println("Item not found");
}
}
public void removeItem() {
printInLine();
System.out.println("\nChoose item to remove: ");
int id = in.nextInt();
if (items.containsKey(id)) {
System.out.println(items.get(id).getName() + " removed");
items.remove(id);
} else {
System.out.println("Item not found");
}
}
public void bilans() {
int spendMoney = 0;
int earnedMoney = 0;
int profit = 0;
for (Map.Entry<Integer, Item> entry : items.entrySet()) {
if (!entry.getValue().sold) {
spendMoney += entry.getValue().getPrice();
} else {
earnedMoney += entry.getValue().getSoldPrice();
profit += (entry.getValue().getSoldPrice() - entry.getValue().getPrice());
}
}
System.out.println("You have already spended: " + spendMoney + " PLN");
System.out.println("You sold items for: " + earnedMoney + " PLN");
System.out.println("Current profit: " + profit + " PLN");
if (earnedMoney > spendMoney) {
System.out.println("Wow, you are on +");
} else {
System.out.println("Keep trying");
}
}
public void details() {
printInLine();
System.out.print("\nPick item: ");
int pickItem = in.nextInt();
if (items.containsKey(pickItem)) {
System.out.println("\n| Details |");
System.out.println("Name: " + items.get(pickItem).getName() +
"\nPrice: " + items.get(pickItem).getPrice() +
"\nCondition: " + items.get(pickItem).getCondition() + "/10" +
"\nColor: ");
} else {
System.out.println("Item not found");
}
}
private void printInLine() {
for (Map.Entry<Integer, Item> entry : items.entrySet()) {
if (!entry.getValue().isSold()) {
System.out.print(" - " + entry.getKey() + "." + entry.getValue().getName());
}
}
}
}
2 Answers 2
Responsibilities
A class should have only one responsibility. Robert C. Martin describes it with
"There should never be more than one reason for a class to change".
When we have a look into Storage we can find multiple responsibilities:
- read inputs from the console
public void sellItem() { // .. int id = in.nextInt(); // .. } - provide the user with information
public void bilans() { // .. System.out.println("You have already spended: " + spendMoney + " PLN"); System.out.println("You sold items for: " + earnedMoney + " PLN"); System.out.println("Current profit: " + profit + " PLN"); // .. } - manage
items
Focus on one Responsibility
The class Storage should focus only on the logic to manage items.
Advantage Over Multiple Responsibilities
Multiple companies bought your tool. Now imagine the following two scenarios:
5 Years later Company A and B want different customized information. You can't find a class like InformationFormate. But after some search you found it in Storage, you go into it and change it. You built an if-statement to check for A or B and the default case to print some information.
Now company C calls you and sad that you should build them a graphical user interface. And again you go into the Storage after you didn't found a class like UserInterface and build a new if-statement to check for C and have build your gui logic into it and for all other the text user interface.
You can see, that with the number of responsibilities the number of possible changes grows for one class and it gets bigger and bigger. Additional when you search for these task you will not think at first that displaying information would be in a class named Storage.
Replace Conditional With Polymorphism
switch (answer) { default: System.out.println("Wrong command"); break; case -1: System.out.println("QUIT"); flag = false; break; case 0: System.out.println(commands); break; case 1: // ... case 8: storage.details(); }
What this huge switch tries to express is
Find a command by a number and execute the associated task.
We could simplyfy the switch with
Command command = commandFactory.getBy(answer);
boolean shouldContinueProgram = command.execute();
Factory Method Pattern
In the Unit above you found
Command command = commandFactory.provide(answer); command.execute();
This is using the factory method pattern.
The commandFactory is an object which gives you the correct Instance of type Command, which is an interface and could be one of your 8 given commands to choose.
interface Command {
boolean execute();
}
class WrongInput implements Command {
@Override
public boolean execute() {
System.out.println("Wrong command");
return true;
}
}
class Quite implements Command {
@Override
public boolean execute() {
System.out.println("Wrong command");
return false;
}
}
interface CommandFactory<T> {
Command provide(T t);
}
class NumberCommandFactory implements CommandFactory<Integer> {
private Map<Integer, Command> numberByCommand;
public NumberCommandFactory() {
numberByCommand.put(-1, new Quite());
// ...
}
@Override
public Command provide(Integer numberOfCommand) {
return numberByCommand.getOrDefault(numberOfCommand, new WrongInput());
}
}
boolean flag = true; while (flag) { System.out.println("Choose option: (0 - print list)"); int answer = in.nextInt(); switch (answer) { default: System.out.println("Wrong command"); break; case -1: System.out.println("QUIT"); flag = false; break; case 0: System.out.println(commands); break; case 1: storage.availableItems(); break; case 2: storage.soldItems(); break; case 3: storage.addItem(); break; case 4: storage.removeItem(); break; case 5: System.out.println("modify item"); break; case 6: storage.sellItem(); break; case 7: storage.bilans(); break; case 8: storage.details(); } }
Consider
public static void loop() {
for (;;) {
int answer = in.nextInt();
switch (answer) {
case -1:
System.out.println("QUIT");
return;
case 0:
System.out.println(commands);
break;
case 1:
storage.availableItems();
break;
case 2:
storage.soldItems();
break;
case 3:
storage.addItem();
break;
case 4:
storage.removeItem();
break;
case 5:
System.out.println("modify item");
break;
case 6:
storage.sellItem();
break;
case 7:
storage.bilans();
break;
case 8:
storage.details();
break;
default:
System.out.println("Wrong command");
}
}
}
Now we don't need the flag variable. We simply loop forever until the user enters -1 and we return from the method.
This gives us the option of moving the method to a separate class, simplifying the Main class and allowing the code to be used in more than one program.
I find it easier to read with the default case at the end and with it being the only case without a break. That's not to say that that the other form is not perfectly functional. But unless you are making use of fallthrough, I just find it simpler this way.
Consider displaying the list of commands any time an invalid command is given. Because sometimes a person can't remember that 0 will print the list of commands. And saying "print list" might not suggest to them that it will print a list of commands rather than items.
Separate logic and display
You have
public void availableItems() { System.out.println("Available items:"); for (Map.Entry<Integer, Item> entry : items.entrySet()) { if (!entry.getValue().sold) { System.out.println(entry.getKey() + ". " + entry.getValue().getName()); } } }
and
private void printInLine() { for (Map.Entry<Integer, Item> entry : items.entrySet()) { if (!entry.getValue().isSold()) { System.out.print(" - " + entry.getKey() + "." + entry.getValue().getName()); } } }
What if we instead had
public static void printItemsOnSeparateLines(Iterable<Item> items) {
for (Item item : items) {
System.out.println(item.getID() + ". " + item.getName());
}
}
public static void printItemsOnSameLine(Iterable<Item> items) {
for (Item item : items) {
System.out.print(" - " + item.getID() + "." + item.getName());
}
}
public List<Item> findAvailableItems() {
return items.stream.filter(item -> !item.isSold()).collect(Collectors.toList);
}
public List<Item> findSoldItems() {
return items.stream.filter(item -> item.isSold()).collect(Collectors.toList);
}
Now we use them like
System.out.println("Available items:");
printItemsOnSeparateLines(findAvailableItems());
And our methods are each individually simpler.
I also changed from a property access to isSold. This reads better to me and is better encapsulated.
Now if we come up with new criteria, we can add a new simple method and just reuse our existing display method. Or we can add a new display and reuse our business logic.
This would require Item to include its identifier, which may just be in the Map keys.
Refactor common code into new methods
public void sellItem() { printInLine(); System.out.println("\nChoose item to mark as sold: "); int id = in.nextInt(); if (items.containsKey(id)) { items.get(id).setSold(true); System.out.println("How much did you get for " + items.get(id).getName() + "?: "); items.get(id).setSoldPrice(in.nextInt()); System.out.println("You marked " + items.get(id).getName() + " as sold"); System.out.println("Your profit is " + (items.get(id).soldPrice - items.get(id).price) + " PLN"); } else { System.out.println("Item not found"); } }
This could be
public void sellItem() {
printItemsOnSameLine(findAvailableItems());
Item item = selectItem("\nChoose item to mark as sold: ");
if (item == null) {
return;
}
if (item.isSold()) {
System.out.println("That item already sold.");
return;
}
item.setSold(true);
System.out.println("How much did you get for " + item.getName() + "?: ");
item.setSoldPrice(in.nextInt());
System.out.println("You marked " + item.getName() + " as sold");
System.out.println("Your profit is " + (item.soldPrice - item.price) + " PLN");
}
with
public Item selectItem(String query) {
System.out.println(query);
Item item = items.get(in.nextInt();
if (item == null) {
System.out.println("Item not found.");
}
return item;
}
Now we don't have to continually rewrite the same code (four times). If we want to select an item, we can just call that method.
I also handled a condition that the original code did not. What if someone entered the ID of an already sold item?
Rather than repeatedly writing items.get(id), this code just uses item.
We don't have to call containsKey, as we can get the same behavior by simply calling get, which we have to call anyway. And now we don't have to carry around the ID.
Logic mistake?
for (Map.Entry<Integer, Item> entry : items.entrySet()) { if (!entry.getValue().sold) { spendMoney += entry.getValue().getPrice(); } else { earnedMoney += entry.getValue().getSoldPrice(); profit += (entry.getValue().getSoldPrice() - entry.getValue().getPrice()); } }
Later you compare spendMoney and earnedMoney. But the two aren't comparable. They are on different items. Consider
for (Item item : values()) {
spentMoney += item.getPrice();
if (item.isSold()) {
earnedMoney += item.getSoldPrice();
profit += item.getSoldPrice() - item.getPrice();
}
}
Now we have the amount of money spent to procure all the items compared to the amount of money earned on the current sold items. We know if we are currently ahead. Or we can look at profit to see if we are ahead on the items already sold.
I also changed from entrySet to just values because you were never using the key. So we can just say item rather than entry.getValue().
I changed spendMoney to spentMoney to be consistent with earnedMoney.