Mengolah fungsi yang mengembalikan kode integer yang mewakili banyak status berbeda

10

Saya telah mewarisi beberapa kode buruk yang telah saya sertakan contoh singkat di bawah ini.

  • Apakah ada nama untuk pola khusus ini?
  • Apa sajakah rekomendasi untuk refactoring ini?

    // 0=Need to log in / present username and password
    // 2=Already logged in
    // 3=Inactive User found
    // 4=Valid User found-establish their session
    // 5=Valid User found with password change needed-establish their session
    // 6=Invalid User based on app login
    // 7=Invalid User based on network login
    // 8=User is from an non-approved remote address
    // 9=User account is locked
    // 10=Next failed login, the user account will be locked
    
    public int processLogin(HttpServletRequest request, HttpServletResponse response, 
                            int pwChangeDays, ServletContext ServContext) { 
    }
    
A_B
sumber
2
Apa itu "mendirikan-bangun" dan "membangun-dibutuhkan" ?
Tulains Córdova
4
Itu seharusnya merupakan dasbor em , baca seperti "ditemukan pengguna yang valid: buat sesi mereka".
BJ Myers
2
@ A_B Manakah dari nilai-nilai kembali yang berhasil login yang gagal login. Tidak semua terbukti dengan sendirinya.
Tulains Córdova
@ A_B Apakah "menetapkan sesi mereka" berarti "sesi ditetapkan" atau "perlu membuat sesi"?
Tulains Córdova
@ TulainsCórdova: "Membangun" berarti sebanyak "menciptakan" (setidaknya dalam konteks ini) - jadi "menetapkan sesi mereka" kira-kira sama dengan "membuat sesi mereka"
hoffmale

Jawaban:

22

Kode buruk bukan hanya karena angka ajaib , tetapi karena menyatukan beberapa makna dalam kode kembali, menyembunyikan di dalamnya artinya kesalahan, peringatan, izin untuk membuat sesi atau kombinasi dari ketiganya, yang membuatnya menjadi input buruk untuk pengambilan keputusan.

Saya akan menyarankan refactoring berikut: mengembalikan enum dengan hasil yang mungkin (seperti yang disarankan dalam jawaban lain), tetapi menambahkan ke enum atribut yang menunjukkan apakah itu penolakan, pengabaian (saya akan membiarkan Anda melewati ini terakhir kali) atau jika tidak masalah (LULUS):

public LoginResult processLogin(HttpServletRequest request, HttpServletResponse response, 
                            int pwChangeDays, ServletContext ServContext) { 
    }

==> LoginResult.java <==

public enum LoginResult {
    NOT_LOGGED_IN(Severity.DENIAL),
    ALREADY_LOGGED_IN(Severity.PASS),
    INACTIVE_USER(Severity.DENIAL),
    VALID_USER(Severity.PASS),
    NEEDS_PASSWORD_CHANGE(Severity.WAIVER),
    INVALID_APP_USER(Severity.DENIAL),
    INVALID_NETWORK_USER(Severity.DENIAL),
    NON_APPROVED_ADDRESS(Severity.DENIAL),
    ACCOUNT_LOCKED(Severity.DENIAL),
    ACCOUNT_WILL_BE_LOCKED(Severity.WAIVER);

    private Severity severity;

    private LoginResult(Severity severity) {
        this.severity = severity;
    }

    public Severity getSeverity() {
        return this.severity;
    }
}

==> Severity.java <==

public enum Severity {
    PASS,
    WAIVER,
    DENIAL;
}

==> Test.java <==

public class Test {

    public static void main(String[] args) {
        for (LoginResult r: LoginResult.values()){
            System.out.println(r + " " +r.getSeverity());           
        }
    }
}

Output untuk Test.java menunjukkan tingkat keparahan untuk setiap LoginResult:

NOT_LOGGED_IN : DENIAL
ALREADY_LOGGED_IN : PASS
INACTIVE_USER : DENIAL
VALID_USER : PASS
NEEDS_PASSWORD_CHANGE : WAIVER
INVALID_APP_USER : DENIAL
INVALID_NETWORK_USER : DENIAL
NON_APPROVED_ADDRESS : DENIAL
ACCOUNT_LOCKED : DENIAL
ACCOUNT_WILL_BE_LOCKED : WAIVER

Berdasarkan nilai enum dan tingkat keparahannya, Anda dapat memutuskan apakah pembuatan sesi dilanjutkan atau tidak.

EDIT:

Sebagai tanggapan terhadap komentar @ T.Sar, saya mengubah nilai yang mungkin dari keparahan ke PASS, WAIVER dan DENIAL alih-alih (OK, PERINGATAN dan KESALAHAN). Dengan cara itu jelas bahwa DENIAL (sebelumnya ERROR) bukan merupakan kesalahan per se dan tidak seharusnya diterjemahkan ke dalam melempar pengecualian. Penelepon memeriksa objek dan memutuskan apakah akan melempar pengecualian, tetapi DENIAL adalah status hasil yang valid yang dihasilkan dari panggilan processLogin(...).

  • LULUS: silakan, buat sesi jika belum ada
  • WAIVER: lanjutkan kali ini, tetapi kali berikutnya pengguna Anda mungkin tidak diizinkan untuk lulus
  • DENIAL: maaf, pengguna tidak dapat lulus, jangan membuat sesi
Tulains Córdova
sumber
Anda juga dapat membangun enum "kompleks" (enum dengan atribut) untuk menyematkan tingkat kesalahan dalam Enum. Namun hati-hati karena jika Anda menggunakan alat serialisasi somme, itu mungkin tidak lulus dengan baik.
Walfrat
Melontarkan pengecualian dalam kasus kesalahan dan menyimpan enum hanya untuk kesuksesan juga merupakan pilihan.
T. Sar
@ T.Sar Yah, seperti yang saya pahami itu bukan kesalahan semata tetapi penolakan untuk membuat sesi untuk beberapa alasan. Saya akan mengedit jawabannya.
Tulains Córdova
@ T.Sar Saya mengubah nilai menjadi PASS, WAIVER dan DENIAL untuk memperjelas apa yang sebelumnya saya sebut ERROR adalah status yang valid. Mungkin sekarang saya harus membuat nama yang lebih baik untukSeverity
Tulains Córdova
Saya sedang memikirkan sesuatu yang lain dengan saran saya, tetapi saya sangat menyukai saran Anda! Saya muntah +1, pasti!
T. Sar
15

Ini adalah contoh Obsesi Primitif - menggunakan tipe primitif untuk tugas "sederhana" yang akhirnya menjadi tidak begitu sederhana.

Ini mungkin sudah dimulai sebagai kode yang mengembalikan a booluntuk menunjukkan keberhasilan atau kegagalan, kemudian berubah menjadi intketika ada keadaan ketiga, dan akhirnya menjadi seluruh daftar kondisi kesalahan yang hampir tidak berdokumen.

Refactoring khas untuk masalah ini adalah membuat kelas baru / struct / enum / objek / apa pun yang dapat lebih mewakili nilai yang dimaksud. Dalam hal ini, Anda dapat mempertimbangkan beralih ke enumyang berisi kondisi hasil, atau bahkan kelas yang bisa berisi booluntuk keberhasilan atau kegagalan, pesan kesalahan, informasi tambahan, dll.

Untuk pola refactoring lebih lanjut yang mungkin berguna, lihat di Lembar Logika Bau Industri untuk Refactorings .

BJ Myers
sumber
7

Saya menyebutnya "angka ajaib" - angka yang spesial dan tidak memiliki arti yang jelas dengan sendirinya.

Refactoring yang akan saya terapkan di sini adalah untuk merestrukturisasi tipe kembali ke enum, karena merangkum masalah domain dalam suatu jenis. Berurusan dengan kesalahan kompilasi yang jatuh dari itu harus dimungkinkan sedikit demi sedikit, karena java enum dapat dipesan dan diberi nomor. Bahkan jika tidak, seharusnya tidak sulit untuk berurusan dengan mereka secara langsung alih-alih kembali ke int.

Daenyth
sumber
Bukan itu yang biasanya dimaksud dengan 'angka ajaib'.
D Drmmr
2
Ini akan muncul sebagai angka ajaib di situs panggilan, seperti diif (processLogin(..) == 3)
Daenyth
@Drmmr - Inilah yang dimaksud dengan bau kode 'angka ajaib'. Tanda tangan fungsi ini hampir pasti menyiratkan bahwa processLogin () berisi baris seperti "return 8;" dalam implementasinya, dan itu memaksa kode menggunakan processLogin () agar terlihat seperti ini "if (resultFromProcessLogin == 7) {".
Stephen C. Steel
3
@Stephen Nilai sebenarnya dari angka-angka itu tidak relevan di sini. Itu hanya identitas. Istilah angka ajaib biasanya digunakan untuk nilai-nilai dalam kode yang memiliki arti, tetapi siapa artinya tidak berdokumen (misalnya dalam nama variabel). Mengganti nilai-nilai di sini dengan variabel integer bernama tidak akan memperbaiki masalah.
D Drmmr
2

Ini sedikit kode yang tidak menyenangkan. Antipattern dikenal sebagai "kode pengembalian ajaib". Anda dapat menemukan diskusi di sini .

Banyak dari nilai yang dikembalikan menunjukkan status kesalahan. Ada perdebatan yang valid tentang apakah akan menggunakan penanganan kesalahan untuk kontrol aliran, tetapi dalam kasus Anda, saya pikir ada 3 kasus: sukses (kode 4), sukses tetapi perlu mengubah kata sandi (kode 5), dan "tidak diizinkan". Jadi, jika Anda tidak peduli tentang menggunakan pengecualian untuk kontrol aliran, Anda bisa menggunakan pengecualian untuk menunjukkan status tersebut.

Pendekatan lain adalah untuk memperbaiki desain sehingga Anda mengembalikan objek "pengguna", dengan atribut "profil" dan "sesi" untuk login yang sukses, atribut "must_change_password" jika perlu, dan banyak atribut untuk menunjukkan mengapa log -dalam gagal jika itu alirannya.

Neville Kuyt
sumber