Clean Code

Кирилл Корняков, Андрей Морозов

Сентябрь 2018

Зачем?

Наши клиенты не смотрят на исходный код.
Почему мы должны держать его в чистоте?

Генеральный директор компании по разработке ПО,
Нижний Новгород, Август 2010

Преимущества чистого кода

Содержание

Что такое качество кода?

Как измерить качество кода?

Признаки качества кода

Хорошо

Плохо

  • Простой, логичный, понятный
  • Чистый, структурированный
  • Краткий, лаконичный
  • Единообразный (стилистически единый)
  • Непонятный, заумный, сложный
  • Запутанный, шумный, визуально нагруженный
  • Длинный, повторяющийся
  • Беспорядочный, пестрый

Ключевые понятия

  1. Избыточность 1. Дублирование (Duplication) 1. Шум (Noise)
  2. Напрасная сложность 1. Принцип единой ответственности (Single Responsibility Principle — SRP) 1. Ортогональность (Orthogonality) 1. Уровни абстракции (Levels of abstraction)
  3. Единообразие (Uniformity)

Откуда берется плохой код?


Кто виноват?

Как результат...

Ключевые принципы (фольклор)

Эксперты о чистоте кода

Say what you mean, simply and directly
Write clearly — don't be too clever
Write clearly — don't sacrifice clarity for "efficiency"

The Elements of Programming Style (1978)

You know you're brilliant, but maybe you'd like to understand what you did 2 weeks from now. If you need more than 3 levels of indentation, you're screwed anyway, and should fix your program.

https://en.wikiquote.org/wiki/Linus_Torvalds

API — самое важное

Matrix createMatrix(int a1, int a2); // Bad
Matrix createMatrix(int width, int height); // OK
List<PECustomerDetailsData> retrieveValidateAndConvertCustomerSpecificDataIntoPresentationEntities(); // Bad
customer.customerName = "Bill";
  public void do(); // move up

  private int i1;   // move down

Содержание

Осмысленные имена

// Bad
public List<int[]> getThem() {
  List<int[]> list1 = new ArrayList<int[]>();
  for (int[] x: theList) {
    if (x[0] == 4)
      list1.add(x);
  }
  return list1;
}
// OK
public List<Cell> getFlaggedCells() {
  List<Cell> flaggedCells = new ArrayList<Cell>();

  for (Cell cell: gameBoard) {
    if (cell.isFlagged()) {
      flaggedCells.add(cell);
    }
  }

  return flaggedCells;
}

Magic numbers

    int dailyPay = hourlyRate * 8;
    double milesWalked = feetWalked / 5280;
    int step = width * 4;

Не используйте их!

Вместо этого используйте:

Шум!

public class Part {
  private String m_dsc;
  void setName(String name) {
    m_dsc = name;
  }
}




public class Part {
  private String description;
  void setDescription(String description) {
    this.description = description;
  }
}

Одно слово для каждой концепции

Имена классов

Имена классов и объектов должны представлять собой
существительные и их комбинации.

Хорошо

Плохо

Имена методов

Имена методов представляют собой
глаголы или глагольные словосочетания.

String name = employee.getName();
customer.setName("Mike");
if (paycheck.isPosted()) ...

Именование переменных и членов класса

Хорошо

public class IncompleteOrder {}
public int currentPosition = -1;
const int WORK_HOURS_PER_DAY = 8;
private bool isBlocked // can, is, has


Плохо

public class incompleteOrder {}
private bool flag;

public const int NUMBEROFCONTEXTS = 10;
private int collectionsize;
private string m_strName;
private byte _array;

Итого. Именование

Советы

Содержание

Функции

Какова нормальная длина функции?

In a completely uncommented 2000 line method:

    {
      {
        while (..) {
          if (..){
          }

          ... (just putting in the control flow here, imagine another few hundred ifs)
          if(..) {
                if(..) {
                    if(..) {
                    ...
                    (another few hundred brackets)
                    }
                }
          } //endif
          ...

Оптимальное количество аргументов

    int OverlayFlatVideos(int numberOfFlatVideos,
                          int currentFrameIdx,
                          OverlayAllVideosParams^ previewParams,
                          std::vector<bool>& StreamProcessed,
                          std::vector<acvCapture*>& flatVideoReaders,
                          std::vector<double>& fpsFlatVideos,
                          bool sharedReflection,
                          std::vector<CvMat*>& reflectionsFlatToDome,
                          CvSize& fullDomeSize,
                          std::vector<IplImage*>& masks,
                          std::vector<IplImage*>& borderSmoothImage,
                          CvSize& maskSize,
                          IplImage*& imageFullDome,
                          CvMat*& tempRef,
                          double fps,
                          int& numberOfVideoReaders,
                          IplImage*& imageReflected,
                          IplImage*& imageFullDomeCopy,
                          InterpolationMethod inMethod) // 19

https://en.wikipedia.org/wiki/Design_smell

Выходные параметры функции

    static void GetSupportDocFilePath(out string supportDocFilePath) {
        supportDocFilePath = new ConfigurationHelper().SupportFilePath;
    }

    GetSupportDocFilePath(path);

Или лучше так?

    static string GetSupportDocFilePath() {
        return new ConfigurationHelper().SupportFilePath;
    }

    path = GetSupportDocFilePath();

Убийственная сложность

float _______ ( float number )
{
  long i;
  float x2, y;
  const float threehalfs = 1.5F;

  x2 = number * 0.5F;
  y  = number;
  i  = * ( long * ) &y;                       // evil floating point bit level hacking
  i  = 0x5f3759df - ( i >> 1 );               // what the f*ck?
  y  = * ( float * ) &i;
  y  = y * ( threehalfs - ( x2 * y * y ) );   // 1st iteration
//      y  = y * ( threehalfs - ( x2 * y * y ) );   // 2nd iteration, this can be removed
  return y;
}

Условия

    if (splitParameters->projectorVideos == nullptr ||
        System::String::IsNullOrEmpty(splitParameters->splitSettings) ||
        splitParameters->projectorWidth <= 0 ||
        splitParameters->projectorHeight <= 0)

    if (timer.HasExpired() && !timer.IsRecurrent()) // Bad
    if (ShouldBeDeleted(timer)) // OK

    if(isValid == false) // Bad
    if(!canEditPrice) // OK

Предпочитайте исключения кодам ошибок

    public void SendShutDown() {
        var handle = GetHandle(device);
        if (handle != DeviceHandle.INVALID) {
            var err = OpenDevice(handle);
            if (err == NULL) {
              //
            }
            else {
              Logger.Log("Can't open device. Error: " + err.ToSting());
            }
        }
        else {
            Logger.Log("Invalid handle for: " +
            device.ToSting());
        }
    }

С использованием исключений

    public void TrySendShutDown() {
        try {
          SendShutDown();
        }
        catch() {
        // ...
      }
    }

    public void SendShutDown() {
      var handle = GetHandle(device);
      OpenDevice(handle);
      //...
    }

Итого: Функции

Содержание

Комментарии

"Don’t comment bad code — rewrite it!"

B. Kernighan, P. Plauger "The Elements of Programming Style"

"...the only software documentation that actually seems to satisfy the criteria of an engineering design is the source code listings".

Jack Reeves

# Комментарии — это плохо!

Комментарии

    // When I wrote this, only God and I understood what I was doing
    // Now, God only knows

    ...

    // Magic. Do not touch.

    ...

    // sometimes I believe compiler ignores all my comments

    ...

    // I'm sorry.

    ...

    // I am not sure if we need this, but too scared to delete.
catch (Exception e) {
    //who cares?
}

Комментарии

Они полезны?

Большие заголовки

    /*---------------------------------------------------------------
    -----------------------
    Created by: NANDA
    Created Date: 01-AUG-2009
    Modified by:
    Procedure Description: Fetches menu items based on the given
    user permission
    Input Parameters: LoginEntry user
    Output Parameters: none
    ----------------------------------------------------------------
    --------------------*/
    public BEMenuList FetchMenuItems(LoginEntity user) {
    ...
    }

Устаревший комментарий

    ...
    // Gets the login user id
    // Gets the CRM details
    FetchCrmDetails();
    ...

Избыточный комментарий

    // If the server variable is empty, throw the error message
    if (loginUserId == null)
    {
        throw new Exception("No User Id");
    }

Запутывающий комментарий

    public void LoadProperties() {
        try
        {
            var propertiesPath = propertiesLocation +
            "/" + PROPERTIES_FILE;
            var propertiesStream = File.Open(propertiesPath);
            loadedProperties.Load(propertiesStream);
        }
        catch (IOException ex) {
            //If file with properties doesn’t exist,
            //default settings are loaded
        }
    }

Закоментированный код

//#region ShowHyperLink
///// <summary>
///// Show the hyperlink controls
///// </summary>
//private void ShowHyperLink()
//{
//  asphypCreateAccounts.Visible = true;
//  asphypCreateAccounts.NavigateUrl = "CreateAccount.aspx";
//  asphypCreateAccounts.Text = WebConstants.CreateAccountHyperLink;
//  asphypCreateExtUser.Visible = true;
//  asphypCreateExtUser.NavigateUrl = "ManageExternalUsers.aspx";
//  asphypCreateExtUser.Text = WebConstants.ExternalUserHyperLink;
//}
//#endregion

Еще хуже

asplblAcceptDeclineDate.Text = contractHistoryList[0].AcceptedDate.ToString();
if (contractHistoryList[0].IsMultiple)
{
    asplblMultiplePublish.Text = "Yes";
}
else
{
    asplblMultiplePublish.Text = "No";
}
//if (contractHistoryList[0].IsLegalApprovalRequierd == true)
//{
//  asplblLegalApproval.Text = "Yes";
//}
//else
//{
//  asplblLegalApproval.Text = "No";
//}
asphdnFileGuid.Value = contractHistoryList[0].FileGuid.ToString();
//int contractRefNo = Convert.ToInt32(asphdnFileMasterld.Value.ToString());
//Session(WebConstants.CONTRACT_REF_NO_SESSION_KEY] = contractRefNo;
asptxtReasonForRejection.Text = string.Empty;

Дезинформация

/**
 * Always returns true.
 */
public boolean isAvailable()
{
    return false;
}

Позволительные комментарии

// format matched kk:mm:ss EEE, MMM dd, yyyy
Pattern timeMatcher = Pattern.Compile("\\d*:\\d*:\\d* \\w*, \\w* \\d*, \\d*");
    //TODO: ...
    //FIXME: ...
    //HACK, NOTE, WARNING

Содержание

Форматирование

http://www.ioccc.org/2011/akari/hint.html

Горизонтальное выравнивание

Плохо

customer.CalculateCredit ( fromDate );
customer.CalculateCredit(fromDate , toDate);
if ( IsValid ) i ++

Хорошо

if(customer.IsValid && customer.Credit == 0.0)
position = new Location(position.x + 10, position.y);
return IsValid ? cdd : DateTime.MaxDate;

Вертикальное выравнивание

using MVCS.Diff4Big.Domain.ImageEntities;
using MVCS.Diff4Big.Domain.Specifications;
namespace MVCS.Diff4Big.Domain.Comparison.FT {
    public class ByteByByte : ITileComparator {
        private readonly LengthBased lengthBased = new LengthBased();
        public IDeltaContainer Compare(IContentTile tile1, IContentTile tile2, ChangeType changeType) {
            var bytesRangeEqualsSpecification = new BytesRangeEquals(tile1.Content);
            var baseResult = lengthBased.Compare(tile1, tile2, changeType);
            if (baseResult != null) return baseResult;
            if (bytesRangeEqualsSpecification.IsSatisfiedBy(tile2.Content)) return null;
            if (changeType == ChangeType.ImageThematicalTile) {
                return new ThematicalTileContainer((ThematicalBig1Tile) tile2.Clone());}
            return new TileContainer((IContentTile) tile2.Clone(), changeType);
        }
    }
}

Вертикальное выравнивание

using MVCS.Diff4Big.Domain.ImageEntities;
using MVCS.Diff4Big.Domain.Specifications;

namespace MVCS.Diff4Big.Domain.Comparison.FT {

    public class ByteByByte : ITileComparator {
        private readonly LengthBased lengthBased = new LengthBased();

        public IDeltaContainer Compare(IContentTile tile1, IContentTile tile2, ChangeType changeType) {
            var bytesRangeEqualsSpecification = new BytesRangeEquals(tile1.Content);

            var baseResult = lengthBased.Compare(tile1, tile2, changeType);
            if (baseResult != null) return baseResult;

            if (bytesRangeEqualsSpecification.IsSatisfiedBy(tile2.Content)) return null;

            if (changeType == ChangeType.ImageThematicalTile) {
                return new ThematicalTileContainer((ThematicalBig1Tile) tile2.Clone());
            }
            return new TileContainer((IContentTile) tile2.Clone(), changeType);
        }
    }
}

Рекомендация

  if (a == 0);
    a++;

  while(a++ != magic_number);
    a = a << 2;



  if (a == 0) {
    a++;
  }

  while(a++ != magic_number) {}
    a = a << 2;

Содержание

Ключевые моменты

Заключение: Правило бойскаута

Всегда оставляй лагерь чище, чем ты его нашел.

Книги

Контрольные вопросы

  1. Признаки хорошего и плохого кода
  2. Ключевые понятия при разговоре о качестве кода, их использование.
  3. Рекомендации по оформлению функций
  4. Рекомендации по именованию
  5. Комментарии и чистый код. Примеры плохих комментариев, почему они считаются code smell.
  6. Ключевые принципы (фольклер)
  7. Почему важно поддерживать код в чистоте

Спасибо за внимание!

Вопросы?

WinApi C++

        hThreadArray[i] = CreateThread(
            NULL,                   // default security attributes
            0,                      // use default stack size
            MyThreadFunction,       // thread function name
            pDataArray[i],          // argument to thread function
            0,                      // use default creation flags
            &dwThreadIdArray[i]);   // returns the thread identifier




    wnd.CreateWnd(hInstance, wcname, NULL, WS_VISIBLE|WS_OVERLAPPEDWINDOW,
        NWin::SRect(NWin::SPoint(CW_USEDEFAULT,CW_USEDEFAULT),
        NWin::SSize(600,400)), NULL,
        LoadMenu(hInstance, resWapp), NULL);