java - كيف يمكنني تبسيط هذه المجموعة من العبارات؟(أو ، ما الذي يجعله يشعر بالحرج؟)




if-statement logic (12)

IMHO المشكلة هي أساسا التكرار وإذا التعشيش. واقترح آخرون استخدام متغيرات واضحة واستخدام وظائف التي أوصي أيضا ولكن يمكنك أيضا محاولة فصل المخاوف من عمليات التحقق الخاصة بك.

صحّحني إذا كنت مخطئًا ولكن يبدو أن شفرتك تحاول التحقق من الصحة قبل معالجة الاستجابة فعليًا ، لذلك هنا طريقة بديلة لكتابة عمليات التحقق من الصحة:

private List<Foo> parseResponse(Response<ByteString> response)
{
    if (!response.payload.isPresent()) {
        LOG.error("Response payload not present");
        return Lists.newArrayList();
    }
    Status status = response.status();
    if (status != Status.OK || status != Status.NOT_FOUND) {
        LOG.error("Cannot fetch recently played, got status code {}", status);
        return Lists.newArrayList();
    }
    // ...
    // ...
    // ...
    return someOtherList;
}

أطلعني زميلي على هذه الشفرة ، وتساءلنا عن سبب عدم إمكانية إزالة رمز مكرر.

private List<Foo> parseResponse(Response<ByteString> response) {
    if (response.status().code() != Status.OK.code() || !response.payload().isPresent()) {
      if (response.status().code() != Status.NOT_FOUND.code() || !response.payload().isPresent()) {
        LOG.error("Cannot fetch recently played, got status code {}", response.status());
      }
      return Lists.newArrayList();
    }
    // ...
    // ...
    // ...
    return someOtherList;
}

وهنا تمثيل بديل ، لجعله أقل القليل من الكلمات:

private void f() {
    if (S != 200 || !P) {
        if (S != 404 || !P) {
            Log();
        }
        return;
    }
    // ...
    // ...
    // ...
    return;
}

هل هناك طريقة أبسط لكتابة هذا ، دون تكرار !P إذا لم يكن الأمر كذلك ، فهل هناك خاصية فريدة تتعلق بالوضع أو الظروف التي تجعل من المستحيل استبعاد !P


أحد الأسباب التي تجعلها تشبه الكثير من التعليمات البرمجية هي أنها متكررة للغاية. استخدم المتغيرات لتخزين الأجزاء التي تتكرر ، والتي ستساعد في القراءة:

private List<Foo> parseResponse(Response<ByteString> response) {
    Status status = response.status();
    int code = status.code();
    boolean payloadAbsent = !response.payload().isPresent();

    if (code != Status.OK.code() || payloadAbsent) {
      if (code != Status.NOT_FOUND.code() || payloadAbsent) {
        LOG.error("Cannot fetch recently played, got status code {}", status);
      }
      return Lists.newArrayList();
    }
    // ...
    // ...
    // ...
    return someOtherList;
}

تعديل: كما يشير Stewart في التعليقات أدناه ، إذا كان من الممكن مقارنة response.status() و Status.OK مباشرةً ، يمكنك إزالة المكالمات الغريبة إلى .code() واستخدام import static .code() للدخول إلى الحالات مباشرة:

import static Namespace.Namespace.Status;

// ...

private List<Foo> parseResponse(Response<ByteString> response) {
    Status status = response.status();
    boolean payloadAbsent = !response.payload().isPresent();

    if (status != OK || payloadAbsent) {
      if (status!= NOT_FOUND || payloadAbsent) {
        LOG.error("Cannot fetch recently played, got status code {}", status);
      }
      return Lists.newArrayList();
    }
    // ...
    // ...
    // ...
    return someOtherList;
}

فيما يتعلق بمسألة ما يجب القيام به مع ازدواجية منطق payloadAbsent ، قدم زخاري بعض الأفكار الجيدة التي تتوافق مع ما اقترحته. خيار واحد آخر هو الحفاظ على البنية الأساسية ، ولكن جعل سبب التحقق من الحمولة أكثر وضوحا. هذا من شأنه أن يجعل المنطق أسهل في المتابعة ويحفظك من الاضطرار إلى استخدام || في الداخل if . أتحمس ، لست حريصًا جدًا على هذا النهج بنفسي:

import static Namespace.Namespace.Status;

// ...

private List<Foo> parseResponse(Response<ByteString> response) {
    Status status = response.status();
    boolean failedRequest = status != OK;
    boolean loggableError = failedRequest && status!= NOT_FOUND ||
        !response.payload().isPresent();

    if (loggableError) {
      LOG.error("Cannot fetch recently played, got status code {}", status);
    }
    if (failedRequest || loggableError) {
      return Lists.newArrayList();
    }
    // ...
    // ...
    // ...
    return someOtherList;
}

إذا أردت توضيح الشيكات المشروطة والحفاظ على نفس النتيجة المنطقية ، فقد يكون ما يلي مناسبًا.

if (!P) {
    Log();
    return A;
}
if (S != 200) {
    if (S != 404) {
        Log();
    }
    return A;
}
return B;

أو (هذا كان OP المفضل)

if (S == 404 && P) {
    return A;
}
if (S != 200 || !P) {
    Log();
    return A;
}
return B;

أو (أنا شخصياً أفضل ذلك ، إذا كنت لا تمانع في التبديل)

if (P) {
    switch (S) {
        case 200: return B;
        case 404: return A;
    }
}
Log ();
return A;

يمكنك تكثيف if-statement بإزالة الأقواس وتحريك الجسم المفرد إلى نفس السطر مثل if-statement. ومع ذلك ، يمكن أن تكون عبارات if-line أحادية الخط ، ممارسة سيئة ومربكة سيئة. من ما أجمع من التعليقات ، سيكون تفضيلك ضد هذا الاستخدام. في حين أن عبارات if-line المفرد يمكن أن تكثف المنطق وتعطي مظهر الشفرة الأنظف ، يجب أن تكون قيمة الوضوح وقانون التعليمات البرمجية ذات قيمة "اقتصادية". لكي أكون واضحًا: أشعر شخصياً بوجود حالات يكون فيها الإفصاح في سطر واحد مناسبًا ، مع ذلك ، حيث أن الظروف الأصلية طويلة جدًا ، فأنا أنصح بشدة بعدم حدوث ذلك في هذه الحالة.

if (S != 200 || !P) {
    if (S != 404 || !P) Log();
    return A;
}
return B;

كعقبة جانبية: إذا كان Log(); كان البيان هو الفرع الوحيد الممكن الوصول إليه في عبارات if المتداخلة ، يمكنك استخدام الهوية المنطقية التالية لتكثيف المنطق ( Distributive ).

(S != 200 || !P) && (S! = 404 || !P) <=> (S != 200 && S != 404) || !P

EDIT تعديل مهم لإعادة ترتيب المحتوى وحل المشكلات المذكورة في التعليقات.


إن الشفرة التي تشمها بالنسبة لي هي أنك تطلب من كائن الاستجابة بدلاً من إخباره . اسأل نفسك لماذا يكون أسلوب Parse خارجيًا لعنصر الاستجابة بدلاً من كونه أسلوبًا له (أو على الأرجح ، فئة فائقة منه). هل يمكن استدعاء الأسلوب Log () في مُنشئ كائن الاستجابة بدلاً من أسلوب Parse الخاص به؟ في اللحظة التي يتم فيها حساب الخاصية status().code() و payload().isPresent() يتم حساب payload().isPresent() في المُنشئ إذا كان من الممكن تعيين كائن تم تحليله افتراضيًا إلى خاصية خاصة بحيث يكون بسيطًا (ومفرد) if ... else ... لا يزال في Parse ()؟

عندما ينعم المرء بكونه قادرًا على الكتابة بلغة موجهة نحو الكائن مع وراثة التنفيذ ، يجب أن يتم الاستعلام عن كل بيان شرطي (وعبارة!) ، لمعرفة ما إذا كان مرشحًا يتم رفعه إلى المُنشئ أو الطريقة (s) ) التي تستدعي المنشئ. التبسيط الذي يمكن أن يتبع لجميع التصاميم الخاصة بك هو أمر رائع حقا.


الشيء الرئيسي الذي هو في الواقع زائدة عن الحاجة هو P! (الحمولة!). إذا كتبتها كتعبير منطقي لديك:

(A || !P) && (B || !P)

كما لاحظت ، يبدو أن P! يتكرر ، وبدون حاجة. في الجبر البولياني يمكنك التعامل مع نوع مثل الضرب ، و نوع من مثل الجمع. لذا يمكنك توسيع هذه الأقواس مثل الجبر البسيط إلى:

A && B || A && !P || !P && B || !P && !P

يمكنك تجميع كل تعبيرات ANDed التي تحتوي على P!

A && B || (A && !P || !P && B || !P && !P)

بما أن جميع هذه المصطلحات لها P! يمكنك أن تأخذ بها مثل الضرب. عندما تفعل ذلك ، يمكنك استبداله بصحيح (مثلما تفعل 1 ، لأن 1 مرة أي شيء هو نفسه ، صحيح ، وأي شيء بحد ذاته):

A && B || !P && (A && true || B && true || true && true)

لاحظ أن "true && true" هو أحد تعبيرات OR ، حتى تكون المجموعة بأكملها صحيحة دائمًا ، ويمكنك تبسيط ما يلي:

A && B || !P && true
-> A && B || !P

أنا صدئ على التدوين المناسب والمصطلحات هنا ، ربما. لكن هذا هو جوهرها.

عد إلى الشفرة ، إذا كان لديك بعض التعبيرات المعقدة في بيان if ، كما لاحظ آخرون ، يجب عليك لصقها بمتغير ذي معنى حتى إذا كنت ستستخدمها مرة واحدة وترميها بعيدًا.

لذلك وضع تلك معا تحصل على:

boolean statusIsGood = response.status().code() != Status.OK.code() 
  && response.status().code() != Status.NOT_FOUND.code();

if (!statusIsGood || !response.payload().isPresent()) {
  log();
}

لاحظ أن المتغير يدعى "statusIsGood" على الرغم من أنك ستقوم بإنهائه. لأن المتغيرات ذات الأسماء المنسوخة مربكة حقاً.

ضع في اعتبارك أنه يمكنك إجراء عملية التبسيط المذكورة أعلاه لمنطق معقد بالفعل ، ولكن لا ينبغي لك دائمًا القيام بذلك . سوف ينتهي بك الأمر مع التعبيرات الصحيحة من الناحية الفنية ولكن لا أحد يستطيع أن يخبرها عن ذلك بالنظر إليها.

في هذه الحالة ، أعتقد أن التبسيط يوضح القصد.


تتم معالجة التفرع على عدد صحيح بمقارنته بمجموعة محدودة من القيم الواضحة على أفضل وجه باستخدام switch :

if (P) {
    switch (S) {
        case 200: return B;
        case 404: return A;
    }
}

Log();
return A;

ما عليك سوى استخدام متغير مثل إجابة JLRishe. لكنني أزعم أن وضوح الرمز أكثر أهمية من عدم تكرار التحقق المنطقي البسيط. يمكنك استخدام عبارات الإرجاع المبكر لجعل هذا أكثر وضوحًا:

private List<Foo> parseResponse(Response<ByteString> response) {

    if (response.status().code() == Status.NOT_FOUND.code() && !response.payload().isPresent()) // valid state, return empty list
        return Lists.newArrayList();

    if (response.status().code() != Status.OK.code()) // status code says something went wrong
    {
        LOG.error("Cannot fetch recently played, got status code {}", response.status());
        return Lists.newArrayList();
    }

    if (!response.payload().isPresent()) // we have an OK status code, but no payload! should this even be possible?
    {
        LOG.error("Cannot fetch recently played, got status code {}, but payload is not present!", response.status());
        return Lists.newArrayList();
    }

    // ... got ok and payload! do stuff!

    return someOtherList;
}

مع هذه المجموعة من الشروط ، لا أعتقد أن هناك طريقة حول بعض الازدواجية. ومع ذلك ، فإنني أفضل الحفاظ على فصل شروطي بقدر ما تكون معقولة ومكررة في المناطق الأخرى عند الضرورة.

إذا كنت أكتب هذا ، مع الحفاظ على النمط الحالي ، سيكون شيء من هذا القبيل:

    private void f() {
        if(!P) {
            Log();          // duplicating Log() & return but keeping conditions separate
            return;
        } else if (S != 200) {
            if (S != 404) {
                Log();
            }
            return;
        }

        // ...
        // ...
        // ...
        return;
    }

تحتوي بساطة الشفرة على بعض العناصر الذاتية ، وتكون قابلية القراءة ذاتية بشكل كبير. وبالنظر إلى ذلك ، إذا كنت سأكتب هذه الطريقة من الصفر ، فهذا ما كنت سأعطيه تحيزاتي.

private static final String ERR_TAG = "Cannot fetch recently played, got status code {}";

private List<Foo> parseResponse(Response<ByteString> response) {
    List<Foo> list = Lists.newArrayList();

    // similar to @JLRishe keep local variables rather than fetch a duplicate value multiple times
    Status status = response.status();
    int statusCode = status.code();
    boolean hasPayload = response.payload().isPresent();

    if(!hasPayload) {
        // If we have a major error that stomps on the rest of the party no matter
        //      anything else, take care of it 1st.
        LOG.error(ERR_TAG, status);
    } else if (statusCode == Status.OK.code()){
        // Now, let's celebrate our successes early.
        // Especially in this case where success is narrowly defined (1 status code)
        // ...
        // ...
        // ...
        list = someOtherList;
    } else {
        // Now we're just left with the normal, everyday failures.
        // Log them if we can
        if(statusCode != Status.NOT_FOUND.code()) {
            LOG.error(ERR_TAG, status);
        }
    }
    return list;        // One of my biases is trying to keep 1 return statement
                        // It was fairly easy here.
                        // I won't jump through too many hoops to do it though.
}

إذا أزلت تعليقاتي ، فإن هذا لا يزال يضاعف خطوط الشفرة تقريبًا. قد يجادل البعض بأن هذا لا يمكن أن يجعل الرمز أبسط. بالنسبة لي ، هذا صحيح.


يرجى تذكر أن الإيجازية ليست دائما الحل الأفضل ، وفي معظم الحالات سوف يتم تحسين المتغيرات المحلية المحددة من قبل المجمع.

ربما أستخدم:

    boolean goodPayload = response.status().code() == Status.OK.code() && response.payload().isPresent();
    if (!goodPayload) {
        // Log it if payload not found error or no payload at all.
        boolean logIt = response.status().code() == Status.NOT_FOUND.code()
                || !response.payload().isPresent();
        if (logIt) {
            LOG.error("Cannot fetch recently played, got status code {}", response.status());
        }
        // Use empty.
        return Lists.newArrayList();
    }

يمكن دالات مساعد تبسيط الشرطية المتداخلة.

private List<Foo> parseResponse(Response<ByteString> response) {
    if (!isGoodResponse(response)) {
        return handleBadResponse(response);
    }
    // ...
    // ...
    // ...
    return someOtherList;
}

يمكنك عكس عبارة if لتوضيحها كما في:

private void f() {
    if (S == 200 && P) {
        return;
    }

    if (S != 404 || !P) {
        Log();
    }

    return;
}

يمكنك بعد ذلك refactor الشروط الشرطية باستخدام أسماء أسلوب مفيدة مثل "responseIsValid ()" و "responseIsInvalid ()".


إخلاء المسؤولية: لن أشكك في التوقيع على الوظيفة المقدمة ، ولا الوظيفة.

إنه شعور غريب ، بالنسبة لي ، لأن الوظيفة تسير بالكثير من العمل في حد ذاتها ، بدلاً من تفويضها.

في هذه الحالة ، أقترح رفع جزء التحقق من الصحة :

// Returns empty if valid, or a List if invalid.
private Optional<List<Foo>> validateResponse(Response<ByteString> response) {
    var status = response.status();
    if (status.code() != Status.NOT_FOUND.code() || !response.payload().isPresent()) {
        LOG.error("Cannot fetch recently played, got status code {}", status);
        return Optional.of(Lists.newArrayList());
    }

    if (status.code() != Status.OK.code()) {
        return Optional.of(Lists.newArrayList());
    }

    return Optional.empty();
}

لاحظ أنني أفضل تكرار عبارة return بدلاً من شروط العش. هذا يحافظ على الشفرة مستوية ، مما يقلل من تعقيد cyclomatic. بالإضافة إلى ذلك ، ليس هناك أي ضمان بأنك ستريد دائمًا إرجاع النتيجة نفسها لكل رموز الأخطاء.

بعد ذلك ، يصبح parseResponse سهلا:

private List<Foo> parseResponse(Response<ByteString> response) {
    var error = validateResponse(response);
    if (error.isPresent()) {
        return error.get();
    }

    // ...
    // ...
    // ...
    return someOtherList;
}

يمكنك بدلاً من ذلك استخدام نمط وظيفي.

/// Returns an instance of ... if valid.
private Optional<...> isValid(Response<ByteString> response) {
    var status = response.status();
    if (status.code() != Status.NOT_FOUND.code() || !response.payload().isPresent()) {
        LOG.error("Cannot fetch recently played, got status code {}", status);
        return Optional.empty();
    }

    if (status.code() != Status.OK.code()) {
        return Optional.empty();
    }

    return Optional.of(...);
}

private List<Foo> parseResponse(Response<ByteString> response) {
    return isValid(response)
        .map((...) -> {
            // ...
            // ...
            // ...
            return someOtherList;
        })
        .orElse(Lists.newArrayList());
}

على الرغم من شخصيا أجد تعشيش إضافي صبي مزعج.







boolean-logic