Poczatkujacy, email-application.

0

Aplikacja do tworzenia adresu e-mail. Określania działu itp. Pytanie czy to jest odpowiednio napisane? Getoza i setoza zachowana.
*

package emailapp;
import java.util.Scanner;

public class EmailApp {

    public static void main(String[] args) {
        Scanner s = new Scanner(System.in);
        Email em1 = new Email("John", "Smith");
        int choice = -1;
        do {
            System.out.println("\nENTER CHOICE\n1.Show Info\n2.Change Password"
                    + "\n3.Change Mailbox Capacity\n4.Set Alternate Email\n5.Exit");
            choice = s.nextInt();
            switch(choice) {
                case 1:
                    em1.getInfo();
                    break;
                case 2:
                    em1.set_password();
                    break;
                case 3:
                    em1.set_mailCap();
                    break;
                case 4:
                    em1.alternate_email();
                    break;
                case 5:
                    System.out.println("\nTHANKS!!!");
                    break;
                default:
                    System.out.println("INVALID CHOICE! ENTER AGAIN!");
            }
        }while(choice!=5);
    }
}

package emailapp;

import java.util.Scanner;
import java.util.Random;

public class Email {

    public Scanner s = new Scanner(System.in);

    private String firstName;
    private String lastName;
    private String department;
    private String email;
    private String password;
    private int mailCapacity = 500;
    private String altEmail;

    public Email(String firstName, String lastName) {
        this.firstName = firstName;
        this.lastName = lastName;
        System.out.println("New worker: " + this.firstName + " " + this.lastName);

        this.department = this.setDept();

        this.password = this.generate_password(8);

        this.email = this.generate_email();
    }

    private String generate_email() {
        return this.firstName.toLowerCase() + "." + this.lastName.toLowerCase() + "@" + this.department.toLowerCase()
                + ".4programmers.net";
    }

    private String setDept() {
        System.out.println(
                "Department code\n1 Sales\n2 Development\n3 Accounting\n0 None");
        boolean flag = false;
        do {
            System.out.print("Enter Department Code: ");
            int choice = s.nextInt();
            switch (choice) {
                case 1:
                    return "Sales";
                case 2:
                    return "Development";
                case 3:
                    return "Accounting";
                case 0:
                    return "None";
                default:
                    System.out.println("INVALID CHOICE");
            }
        } while (!flag);
        return null;
    }

    private String generate_password(int length) {
        Random r = new Random();

        String Capital_chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
        String Small_chars = "abcdefghijklmnopqrstuvwxyz";
        String numbers = "0123456789";
        String symbols = "!@#$%&?";
        String values = Capital_chars + Small_chars + numbers + symbols;

        String password = "";
        for (int i = 0; i < length; i++) {
            password = password + values.charAt(r.nextInt(values.length()));
        }
        return password;
    }

    public void set_password() {
        boolean flag = false;
        do {
            System.out.print("Change password? (Y/N) : ");
            char choice = s.next().charAt(0);
            if (choice == 'Y' || choice == 'y') {
                flag = true;
                System.out.print("Enter current password: ");
                String temp = s.next();
                if (temp.equals(this.password)) {
                    System.out.println("Enter new password: ");
                    this.password = s.next();
                    System.out.println("PASSWORD CHANGED");
                } else {
                    System.out.println("Incorrect Password!");
                }
            } else if (choice == 'N' || choice == 'n') {
                flag = true;
                System.out.println("PASSWORD CHANGE CANCELED!");
            } else {
                System.out.println("ENTER A VALID CHOICE");
            }
        } while (!flag);
    }

    public void set_mailCap() {
        System.out.println("Current capacity = " + this.mailCapacity + "mb");
        System.out.print("Enter new capacity: ");
        this.mailCapacity = s.nextInt();
        System.out.println("MAILBOX CAPACITY CHANGED SUCCESSFULLY!");

    }
    public void alternate_email() {
        System.out.print("Enter new alternate email: ");
        this.altEmail = s.next();
        System.out.println("ALTERNATE EMAIL SET SUCCESSFULLY!");
    }

    public void getInfo() {
        System.out.println("Name: " + this.firstName + " " + this.lastName);
        System.out.println("Department: " + this.department);
        System.out.println("Email: " + this.email);
        System.out.println("Password: " + this.password);
        System.out.println("Mailbox Capacity: " + this.mailCapacity + "mb");
        System.out.println("Alter Email: " + this.altEmail);
    }
}
2
p_agon napisał(a):

Getoza i setoza zachowana.

Brzmi jakbyś chciał pochwały ...

  1. w Javie na nazwy jest konwencja nie set_password a setPassword
    Po drugie pod taką nazwą oczekiwany jest setter, a nie algorytm wchodzący w interakcje z otoczeniem
  2. podobnie
   private String setDept()

kłamie co do przeznaczenia

  1. nazwa klasy Email pozornie coś mówi, a w rzeczywistości wszytko / nic.
    zwłaszcza, ze kod klasy wymyka się jasnemu opisowi.
  2. public void getInfo() kłamie, bo niczego nie bierze, ale drukuje

Klasa to nie jest "zbiornik na wszytko co wiem", tylko klasa ma** zakres odpowiedzialności**. jest albo daną, albo przetwarza itd...
Tu masz groch z kapustą.

PS. z geterów i seterów dwója.

0

Getoza i setoza zachowana.

Brzmi jakbyś chciał pochwały ...

Brzmisz jak Polak ;)

  1. Nazwy zmienione.
  2. setDept() zmieniony na choiceDept()
  3. Nazwa klasy Email została zmieniona na CreateEmailForNewEmployee, nie wiem czy powinienem wyciągnąć operacje na hasłach do nowej klasy.
  4. public void getInfo() zmieniłem na printInfo nie lepiej by bylo getAndPrintInfo?

Z getrów i seterów dwója to i tak lepiej niż jedynka. Na studiach poprawka. Załączałam nowy kod i proszę o zaliczenie :)

package emailapp;

import java.util.Scanner;
import java.util.Random;

public class CreateEmailForNewEmployee {

    public Scanner s = new Scanner(System.in);

    private String firstName;
    private String lastName;
    private String department;
    private String email;
    private String password;
    private int mailCapacity = 500;
    private String altEmail;

    public CreateEmailForNewEmployee(String firstName, String lastName) {
        this.firstName = firstName;
        this.lastName = lastName;
        System.out.println("New worker: " + this.firstName + " " + this.lastName);

        this.department = this.choiceDept();

        this.password = this.generatePassword(8);

        this.email = this.generateEmail();
    }

    private String generateEmail() {
        return this.firstName.toLowerCase() + "." + this.lastName.toLowerCase() + "@" + this.department.toLowerCase()
                + ".4programmers.net";
    }

    private String choiceDept() {
        System.out.println(
                "Department code\n1 Sales\n2 Development\n3 Accounting\n0 None");
        boolean flag = false;
        do {
            System.out.print("Enter Department Code: ");
            int choice = s.nextInt();
            switch (choice) {
                case 1:
                    return "Sales";
                case 2:
                    return "Development";
                case 3:
                    return "Accounting";
                case 0:
                    return "None";
                default:
                    System.out.println("INVALID CHOICE");
            }
        } while (!flag);
        return null;
    }

    public void setupMailCap() {
        System.out.println("Current capacity = " + this.mailCapacity + "mb");
        System.out.print("Enter new capacity: ");
        this.mailCapacity = s.nextInt();
        System.out.println("MAILBOX CAPACITY CHANGED SUCCESSFULLY!");

    }
    public void alternateEmail() {
        System.out.print("Enter new alternate email: ");
        this.altEmail = s.next();
        System.out.println("ALTERNATE EMAIL SET SUCCESSFULLY!");
    }

    public void printInfo() {
        System.out.println("Name: " + this.firstName + " " + this.lastName);
        System.out.println("Department: " + this.department);
        System.out.println("CreateEmailForNewEmployee: " + this.email);
        System.out.println("Password: " + this.password);
        System.out.println("Mailbox Capacity: " + this.mailCapacity + "mb");
        System.out.println("Alter CreateEmailForNewEmployee: " + this.altEmail);
    }
    private String generatePassword(int length) {
        Random r = new Random();

        String Capital_chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
        String Small_chars = "abcdefghijklmnopqrstuvwxyz";
        String numbers = "0123456789";
        String symbols = "!@#$%&?";
        String values = Capital_chars + Small_chars + numbers + symbols;

        String password = "";
        for (int i = 0; i < length; i++) {
            password = password + values.charAt(r.nextInt(values.length()));
        }
        return password;
    }

    public void changePassword() {
        boolean flag = false;
        do {
            System.out.print("Change password? (Y/N) : ");
            char choice = s.next().charAt(0);
            if (choice == 'Y' || choice == 'y') {
                flag = true;
                System.out.print("Enter current password: ");
                String temp = s.next();
                if (temp.equals(this.password)) {
                    System.out.println("Enter new password: ");
                    this.password = s.next();
                    System.out.println("PASSWORD CHANGED");
                } else {
                    System.out.println("Incorrect Password!");
                }
            } else if (choice == 'N' || choice == 'n') {
                flag = true;
                System.out.println("PASSWORD CHANGE CANCELED!");
            } else {
                System.out.println("ENTER A VALID CHOICE");
            }
        } while (!flag);
    }
}
package emailapp;
import java.util.Scanner;

public class EmailApp {

    public static void main(String[] args) {
        Scanner s = new Scanner(System.in);
        CreateEmailForNewEmployee em1 = new CreateEmailForNewEmployee("John", "Smith");
        int choice = -1;
        do {
            System.out.println("\nENTER CHOICE\n1.Show Info\n2.Change Password"
                    + "\n3.Change Mailbox Capacity\n4.Set Alternate CreateEmailForNewEmployee\n5.Exit");
            choice = s.nextInt();
            switch(choice) {
                case 1:
                    em1.printInfo();
                    break;
                case 2:
                    em1.changePassword();
                    break;
                case 3:
                    em1.setupMailCap();
                    break;
                case 4:
                    em1.alternateEmail();
                    break;
                case 5:
                    System.out.println("\nTHANKS!");
                    break;
                default:
                    System.out.println("INVALID CHOICE! ENTER AGAIN!");
            }
        }while(choice!=5);
    }
}

Nie spodziewalem sie takiej dobrej konstruktywnej krytyki. Dziekuje!

1

CreateEmailForNewEmployee jak nazwa wskazuje powinna być odpowiedzialna za tworzenie konta email a widze, że jeszcze tam hasło zmienia, generuje je... Z konstruktora wywaliłbym te metody proszące o konkretne dane i do klasy CreateEmailForNewEmployee przychodził już z gotowymi danymi w postaci jakiegoś dto.

2

Podstawowym problemem tego kodu jest zmiksowanie ze sobą wielu warstw i odpowiedzialności. Gdybym chciał zmienić hasło przez np. Rest API to musiałbym wszystko wyrzucić do śmieci i napisać od nowa. Tak samo, gdybym chciał logować nie na konsole ale do pliku. Brzydki kod, nierozszerzalny, zero enkapsulacji. Nie mówię już o testowalności.

2

To jest po prostu kod napisany proceduralnie, tylko po wierzchu udający programowanie obiektowe. Jeśli klasa nazywa się CreateEmailForNewEmployee, coś poszło nie tak. "choiceDept" też nie podnosi jakości kodu. Sporo tu również - jak na tak krótki program - przykładów zwykłej niechlujności.

Na dobry początek radziłbym oddzielić działania na "czystych" danych (np. generowanie hasła czy adresu mailowego) od kodu odpowiedzialnego za komunikację z użytkownikiem (czyli wszystkiego, co wymaga użycia Scanner, print itd.). Jedno z drugim nie powinno współistnieć w tej samej klasie.

Nie jest także normalne, aby generator adresów email znał i pamiętał dane pracownika. Generator powinien być od generowania, a nie od przechowywania efektów swojej pracy. To już jest osobna odpowiedzialność.

Moim zdaniem najprostsza sensowna architektura mogłaby wyglądać z grubsza tak:

  • Klasa Employee, zawierająca dane pracownika. To tutaj znajdowałyby się takie pola jak department czy email.
  • Klasa EmailGenerator, która na żądanie generuje adresy email czy hasła.
  • Klasa App, która odpowiada za komunikację z użytkownikiem, i reaguje na jego decyzje - edytując dane w obiekcie Employee, wyświetlając efekty tych edycji itd.
    I wyłącznie w tej klasie używamy Scanner czy System.out.*.
    Dzięki temu kod klas Employee i EmailGenerator byłby teoretycznie gotów do użycia w jakimś innym kontekście - np. aplikacji webowej. Inaczej mówiąc, nie byłby już związany z warstwą prezentacji danych. Nie ma żadnego powodu, by taki kod wiedział, albo musiał się liczyć z faktem, że aplikacja ma akurat formę tekstową. Kluczowe pojęcie: https://pl.wikipedia.org/wiki/Zasada_jednej_odpowiedzialności
  • Kwestią do rozważenia jest, gdzie Employee powinien "spinać się" z EmailGenerator. Kod, w którym to nastąpi, reprezentowałby naszą tzw. logikę biznesową.
    Dla uproszczenia i na początek można zostawić to w EmailApp. W prawdziwym projekcie zapewne wylądowałoby to w jeszcze osobnej klasie.
    Chociażby dlatego, aby dało się napisać testy jednostkowe (automatyczne) dla tego kodu. Aby było to w ogóle możliwe, testowany kod nie może wymagać, że ktoś będzie mu wpisywał dane z klawiatury...

PS. Ciekawe realia panują w tej fikcyjnej firmie - domyślna pojemność skrzynki: 500 maili? ;) My już przed wojną mielim więcej, panie

1 użytkowników online, w tym zalogowanych: 0, gości: 1