Кодирование Стандартов / Кодирующий Лучшие практики в C++

I've used similar to both in different circumstances but you've overcomplicated the first IMO:

bool MyApplication::ReportGenerator::GenerateReport(){
    bool retval = false;
    if (!isAdmin()){
    }
    else if (!isConditionOne()){
    }
    else if (!isConditionTwo()){
    }
    else if (!isConditionThree()){
    }
    else
        retval = generateReport();
    return retval;
}

Лично я предпочитаю мои для , , в то время как и делают ... while циклы, чтобы быть реальными циклами. В первом примере кода это не так. Так что я бы выбрал пример 2. Или, как уже говорили другие, разбить пример 2 на ряд операторов if ... return .

Code Complete - это обычно рекомендуемая книга, в которой подробно рассматриваются стилистические проблемы подобного рода. Также подумайте о том, чтобы взглянуть на некоторые из опубликованных руководств по стилю организации, чтобы узнать, есть ли у них какое-либо мнение по этому вопросу; Руководство по стилю Google , например.

Что касается того, что я предпочитаю, второй пример, на мой взгляд, намного лучше. Первый - это, по сути, злоупотребление конструкцией do {} while с целью избежать использования goto, догматическое соблюдение буквы «избегать goto любой ценой», но при этом отсутствует его дух «код для ясности, не используйте неочевидные языковые уловки» .

Фактически, единственная причина вообще использовать goto - это догматическая приверженность подходу «только один оператор возврата на функцию», когда вы могли бы уйти с простым, readable

if (!isAdmin()){
    return false;
}
else if (!isConditionOne()){
    return false;    }
else if (!isConditionTwo()){
    return false;    }
else if (!isConditionThree()){
    return false;    }
else
    return generateReport();

Другие мысли?

Не называйте локальные переменные, которые используются для хранения вычисленного состояния успеха, «returnValue» или аналогичным. Очевидно все, что вы возвращаете, является возвращаемым значением, любой, кто может читать C, может увидеть, что возвращается. Скажите , какое вычисление он выполняет. Имя returnValue не дает мне никакой информации о том, что оно означает, когда оно истинно или ложно. В этом примере «couldGenerateReports» или подобное было бы гораздо предпочтительнее.

https://computing.llnl.gov/linux/slurm/coding_style.pdf

A switch would be a better solution to your problem I think. You would need to overload the method to take in an int param and I don't know if that's something you would want to do or not.

Bool MyApplication::ReportGenerator::GenerateReport(int i)
{
  switch(i)
  {
    case 1:
       // do something
       break;
    case 2:
       // do something
       break;
   // etc
 }
 return GeneratReport()
}

Not really sure what your plan is since you're calling the method recursively and as some point you will want to leave the method.

Лично я предпочитаю образец 2. Он группирует те элементы, которые не приводят к созданию отчета вместе.

в качестве общих рекомендаций по кодированию книга Code Complete ( Amazon) является хорошим справочником по вопросам стиля кодирования.

Относительно примера 1: Если вы хотите goto, почему бы вам не написать goto ??? Это намного яснее вашего предложения. И учитывая, что goto следует использовать редко, я голосую за №2.

Code 1 is, IMO, worst as it does not immediately convey the intendend meaning which is to generate the repoort only in certain circumstances.

Using:

   if (condition_1) return false;
   if (condition_2) return false;
   ...

would be better.

Also, what I dislike in code 1 is the fact that it try to mask gotos using a while and breaks (which are gotos). I would then prefer to use directly a goto, at least it would have been easier to see where the landing point is.

Code 2 might be formatted to look nicely, I guess:

 bool MyApplication::ReportGenerator::GenerateReport(){
     if (!isAdmin()        || !isConditionOne() ||
         !isConditionTwo() || !isConditionThree()) {
        return false;
     }
     return generateReport();
 }

Or something similar.

Какой из них, по вашему мнению, лучше всего выражает то, что пытается сказать код. Какой из них вам нужно больше всего потрудиться, чтобы понять?

Я бы сделал это:

bool MyApplication::ReportGenerator::GenerateReport(){
    if (isAdmin()  && isConditionOne() && isConditionTwo() && isConditionThree()){
         return generateReport();
    } else {
        return false;
    }

}

Потому что:

a). Предпочитаю говорить то, что хочу, а не то, чего не хочу б). предпочитаю симметрию, если и еще. Ясно, что все случаи покрыты.

I prefer a modification of sample 2:

bool MyApplication::ReportGenerator::GenerateReport()
{
    bool returnValue = false;
    if (isAdmin() &&
        isConditionOne() &&
        isConditionTwo() &&
        isConditionThree())
    { 
        returnValue = generateReport();
    } 
    return returnValue;
}

It has the benefit of having a single exit point for the function, which is recommended for easier maintenance. I find stacking conditions vertically instead of horizontally easier to read quickly, and it's a lot easier to comment individual conditions if you need to.

Это действительно зависит от будущих ожиданий кода. Код 1 выше подразумевает, что может быть добавлена ​​дополнительная логика для каждого из условий; Код 2 выше подразумевает скорее, что существует рациональная группировка условных выражений. Code1 может быть более актуальным, если вы планируете добавить логику для условий позже; если вы этого не сделаете, Code2, вероятно, более разумен из-за краткости и подразумеваемой группировки.

bool MyApplication::ReportGenerator::GenerateReport(){
    if ( ! isAdmin         () ) return false ;
    if ( ! isConditionOne  () ) return false ;
    if ( ! isConditionTwo  () ) return false ;
    if ( ! isConditionThree() ) return false ;
    return generateReport() ;
}

bool MyApplication::ReportGenerator::GenerateReport()
{
   return isAdmin()  
      && isConditionOne() 
      && isConditionTwo()
      && isConditionThree()
      && generateReport();    // Everything's ok.
}

Я лично предпочел бы вариант вашего второго сегмента кода. Короткое замыкание будет работать так же, но условное выражение менее подробное.

bool MyApplication::ReportGenerator::GenerateReport()
{
    if(isAdmin() && isConditionOne() && isConditionTwo() && isConditionThree())
    {
        return generateReport();
    }

    return false;
}

В нем все сказано в одном красивом чистом месте. «Если все эти условия соблюдены, сделайте это. В противном случае не делайте этого»

Я чувствую, что ваш первый сегмент кода усложняет понимание этой логики, распределяя условие по 12 строкам. Кроме того, замкнутый цикл может заставить кого-то сделать двойной дубль.

Мне нравятся ответы, являющиеся вариацией версии 2, но просто чтобы дать альтернативу: если эти условия логически связаны друг с другом, есть вероятность, что вам нужно будет снова проверить их в других местах. Если это правда, то, возможно, вспомогательная функция сработает лучше. Примерно так:

bool isReportable(anyParametersNeeded){
    //stuffYouWantToCheck
}

bool MyApplication::ReportGenerator::GenerateReport(){
    if (isReportable(anyParametersNeeded)){
        return generateReport();
    }
    return false;
}

Поскольку эта функция небольшая, возможно, вы даже сможете встроить ее (или позволить компилятору решать;)). Еще один бонус заключается в том, что если вы хотите включить дополнительные проверки в будущем, вам нужно будет изменить только эту функцию, а не каждое место, где она используется.