На вопрос «Мне сказали, что методы должны быть максимально короткими, и один метод должен делать только одну вещь».
На самом деле это два разных утверждения:
- методы должны быть как можно короче
- метод должен делать только одну вещь.
Для первого утверждения:
В общем, я бы отклонил первое утверждение. Самый короткий способ написания метода - это нечитаемый мусор запутанного кода.
Однако упрощение понимания кода часто приводит к уменьшению количества строк кода. Кроме того, если вы последуете второму утверждению, вы получите меньше строк.
Поэтому, хотя я думаю, что попытка создать наименьшее количество строк кода не приносит никакой пользы, многие строки указывают на то, чтобы просмотреть код, если он может быть улучшен (но сам обзор не должен фокусироваться на строках кода. по качеству кода).
Для второго утверждения:
Это утверждение верно! Метод должен концентрироваться на одной задаче и делать это правильно. У него должно быть как можно меньше побочных эффектов.
Вы призываете это сделать что-то. Если он делает что-то дополнительное, вы, скорее всего, рано или поздно столкнетесь с проблемой, когда вызовете ее для первоначальной цели. Или, наоборот, вы не можете назвать это в другой ситуации, поскольку это делает то, что вы не хотите делать.
Также для ясности метод, который имеет ясную цель, легко понять и проверить: делает ли он то, что должен? Или это что-то делает не так? Вы можете легко создать тест, который проверяет метод, если вы хотите быть уверены.
Теперь для вас код:
Прежде всего, в качестве оправдания, ваш пример «как есть» не совсем идеален, чтобы показать, как к нему приблизиться. Также отсутствуют детали, которые могут или не могут дать место для дальнейших улучшений. Так что не будьте догматичными, а скорее сконцентрируйтесь на основной идее разбить ваш код на несколько логических единиц (как вы и просили).
а) У вас есть идентификация пользователя:
System.out.println("Enter the name of the user>");
String userName = input.nextLine().toLowerCase();
User u = getUser(userName);
if (userName == null || !userRegister.contains(u)) {
System.out.println("error. no such user");
return;
}
Эта идентификация представляется логической единицей, которую можно поместить в свой собственный метод. Идентифицирует пользователя.
b) Вы получаете некоторую ставку bidAuction из контейнера, определенного где-то еще:
for(Auction a: auctionRegister ) {
if(auctionRegister.equals(name.toLowerCase())) {
bidAuction = a;
break;
}
}
Этот мир может быть отдельным методом, который выбирает элемент по его имени: "getBidAuction (String name)".
в) Следующая ставка:
System.out.print("Amount to bid: min " + (getAuctionedDog(d).getTopBid()+ 1 )+ "> ");
int amount = input.nextInt();
while(amount <= a.getTopBid()) {
System.out.println("error. too low bid");
System.out.println("Amount to bid: min " + (getAuctionedDog(d).getTopBid() + 1));
amount = input.nextInt();
}
Bid b = new Bid(u, amount);
Это снова напоминает код, который можно вставить в метод, который будет возвращать ставку: "Bid getBid (пользователь, аукцион аукцион) {".
С этим вы в конечном итоге получите это - которое все еще не оптимально (некоторые намеки на логику и на то, что делают другие методы, отсутствует), но немного разбито :
public class NewClass {
User identifyUser() {
System.out.println("Enter the name of the user>");
String userName = input.nextLine().toLowerCase();
User user = getUser(userName);
if (userName == null || !userRegister.contains(user)) {
System.out.println("error. no such user");
return null;
}
return user;
}
Auction findAuction(String name) {
Auction bidAuction = null;
for (Auction a : auctionRegister) {
if (auctionRegister.equals(name.toLowerCase())) {
return a;
}
}
return null;
}
Bid getBid(User user, Auction auction) {
System.out.print("Amount to bid: min " + (auction.getTopBid() + 1) + "> ");
int amount = input.nextInt();
while (amount <= auction.getTopBid()) {
System.out.println("error. too low bid");
System.out.println("Amount to bid: min " + (auction.getTopBid() + 1));
amount = input.nextInt();
}
return new Bid(user, amount);
}
void makeNewBid() {
User user = identifyUser();
if (null == user) {
return; //use an exception instead.
}
String name = dogLowerCaseTrim();
Auction bidAuction = findAuction(name);
//is this realy what you intended?!
if (bidAuction != null) {
System.out.println("error. this dog is not up for auction.");
return; //use an exception instead?
}
Dog dog = getDog(name);
//might this be the same as bidAuction?!
Auction auction = getAuctionedDog(dog);
Bid bid = getBid(user, auction);
auction.addBid(bid);
System.out.println("Bid made");
input.nextLine();
}
}
Следующий вопрос, который возникает, почему пользователь снова регистрируется на каждую ставку? Разве это не должно быть сделано где-то еще? Может ли даже случиться, что незарегистрированный пользователь сделает ставку? Скорее всего, это следует проверить вне метода.
То же самое может быть в случае с "аукционом". В зависимости.
Что делает getAuctionedDog ()? Это лишнее? Это похоже на findAuction? Так является ли bidAuction тем же объектом, что и аукцион? - не могу сказать из кода. Если нет: возможно, так и должно быть! Если так: используйте тот, который у вас есть, и удалите второй (и вместе с ним в этом случае лишний объект собаки).
Является ли сравнение "if (bidAuction! = Null) {" логически тем, что вы хотите? - Я не могу сказать из вашего кода, но, скорее всего, вы хотите его наоборот "if (bidAuction == null) {".