Что плохого в использовании заголовков кнопок в качестве переменных в VB6?

Question

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

Я чувствую, что это заслуживает отдельного обсуждения, поэтому здесь та же идея была немного очищена и изменена, чтобы сделать ее "правильной" (что в основном означает определение строк в одном месте, чтобы ваш код не начинал сбоить, потому что вы просто изменил текст командной кнопки). Я знаю, что мои правила именования переменных и элементов управления плохие (ОК, не существует), поэтому заранее извиняюсь. Но я бы хотел сосредоточиться на заголовке как на обсуждении переменных состояния.

Итак, поехали:

' Global variables for this form
Dim DoTheThingCaption(1) As String
Dim UserCancel, FunctionCompleted As Boolean

Private Sub Form_Initialize()
  ' Define the possible captions (is there a #define equivalent for strings?)
  DoTheThingCaption(0) = "Click to Start Doing the Thing"
  DoTheThingCaption(1) = "Click to Stop Doing the Thing"

  ' Set the caption state when form initializes
  DoTheThing.Caption = DoTheThingCaption(0)
End Sub

Private Sub DoTheThing_Click() ' Command Button

If DoTheThing.Caption = DoTheThingCaption(0) Then
  UserCancel = False ' this is the first time we've entered this sub
Else ' We've re-entered this routine (user clicked on button again
     ' while this routine was already running), so we want to abort
  UserCancel = True ' Set this so we'll see it when we exit this re-entry
  DoTheThing.Enabled = False 'Prevent additional clicks
  Exit Sub
End If

' Indicate that we're now Doing the Thing and how to cancel
DoTheThing.Caption = DoTheThingCaption(1)

For i = 0 To ReallyBigNumber
  Call DoSomethingSomewhatTimeConsuming
  If UserCancel = True Then Exit For ' Exit For Loop if requested
  DoEvents ' Allows program to see GUI events
Next

' We've either finished or been canceled, either way
' we want to change caption back
DoTheThing.Caption = DoTheThingCaption(0)

If UserCancel = True Then GoTo Cleanup

'If we get to here we've finished successfully
FunctionCompleted = True
Exit Sub '******* We exit sub here if we didn't get canceled ******* 

Cleanup:
'We can only get to here if user canceled before function completed

FunctionCompleted = False
UserCancel = False ' clear this so we can reenter later
DoTheThing.Enabled = True 'Prevent additional clicks

End Sub  '******* We exit sub here if we did get canceled ******* 

Итак, вот оно. Есть ли что-нибудь действительно такое, что плохо делает это таким образом? Это просто вопрос стиля? Есть ли что-то еще, что дало бы мне эти четыре вещи более желательным или поддерживаемым способом?

1. Мгновенная обратная связь с графическим интерфейсом пользователя о том, что нажатие кнопки пользователя привело к действию
2. Мгновенная обратная связь с графическим интерфейсом пользователя в том месте, где пользователь уже видит, как ОТМЕНИТЬ, если действие не требуется
3. Одноклавишный способ для пользователей запустить / отменить операцию (уменьшая количество беспорядка в GUI)
4. Простое немедленное отключение командной кнопки для предотвращения нескольких запросов закрытия

Я вижу, что одной проблемой может быть тесная связь (несколькими способами) между кодом и графическим интерфейсом, поэтому я мог видеть, как это может стать большой проблемой для больших проектов (или, по крайней мере, для больших графических интерфейсов). Это, случается, меньший проект, где есть только 2 или 3 кнопки, которые получили бы этот вид "обработки".

Answer 1

Самая большая проблема с этим методом заключается в том, что он использует строку в качестве логического значения. По определению, логическая переменная может иметь только два состояния, в то время как строка может иметь любое количество состояний.

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

• Логика не совсем ясна в отношении текущих и доступных состояний (на самом деле для формы существует четыре возможных состояния: не начато, начато, завершено, начато, но отменено) - обслуживание потребует тщательного наблюдения за потенциальными взаимодействиями между текстом кнопки и состояниями логической переменной, чтобы определить, каким должно быть текущее состояние. Одно перечисление сделало бы эти состояния явными, что облегчило бы чтение и понимание кода, тем самым упрощая обслуживание.
• Вы полагаетесь на поведение свойства элемента управления (текст кнопки), чтобы оставаться согласованным с типом значения открытого свойства (строка). Именно такое предположение делает миграцию старого кода VB6 на новые языки / платформы абсолютный ад .
• Сравнение строк намного, намного медленнее, чем простой тест логической переменной. В этом случае это не имеет значения. В общем, его так же легко избежать.
• Вы используете DoEvents для симуляции многопоточности (не имеет прямого отношения к вопросу ... но, тьфу).

Answer 2

Игнорирование общих проблем архитектуры / связи, потому что вы знаете об этих проблемах, одна проблема с вашим подходом состоит в том, что элементы управления VB6 делают магические вещи, когда вы устанавливаете свойства. Вы можете думать, что просто устанавливаете свойство, но во многих случаях вы также вызываете события. Установка значения флажка в true запускает событие click. Установка tabindex на элементе управления tab вызывает событие click. Есть много случаев.

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

Я видел слишком много ужасного кода VB6, который использует управляющие свойства в качестве хранилища. Если вы когда-нибудь найдете такой код, вы узнаете его, потому что он разбросан избыточными вызовами методов Refresh, DoEvents, и вы часто будете видеть, как пользовательский интерфейс зависает. Это часто вызывается бесконечными циклами, в которых устанавливается свойство, вызывается событие, а затем устанавливается другое свойство, и в конце концов кто-то пишет строку кода, которая снова обновляет первое свойство.

Если эти проблемы вас не пугают, подумайте об этом. Некоторые из нас просто не такие умные. Я кодирую на VB6 более 10 лет и лично написал около 750K LOC, и я продолжаю смотреть на ваш пример выше, и мне очень трудно понять, что происходит. Предположим, что все люди, которые будут нуждаться в чтении вашего кода в будущем, будут очень глупыми и сделают нас счастливыми, написав действительно простой на вид код.

Answer 3

Самая большая проблема, с которой я столкнулся при работе с (очень старым) кодом, подобным этому [заголовки кнопок в качестве переменных], заключается в том, что глобализация - это кошмар .... Мне пришлось переместить старое приложение vb6, чтобы использовать английский и немецкий языки. ... это заняло недели, если не месяцы.

Вы также используете goto ..... требуется немного рефакторинга, возможно, чтобы сделать код читабельным ??

** Редактировать в ответ на комментарии Я использовал бы только goto в vb6 в начале каждого процесса; в случае ошибки перейдите к myErrorHandler.

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

Answer 4

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

Answer 5

Помимо удаления GOTos, как это сделал DJ в своем ответе , в вашем подходе нет ничего плохого. Заголовок кнопки может иметь только два состояния, и вы используете эти два состояния для определения потока в вашем коде.

Однако у меня есть две причины, по которым я бы сделал это по-другому:

1. Ваш метод создает проблемы, когда вы хотите перевести вашу программу на другой язык (по моему опыту вы должны всегда планировать это), потому что заголовки изменятся на другом языке
2. Это противоречит принципу отделения пользовательского интерфейса от потока программ. Это может быть не важно для вас, но когда программа становится больше и сложнее, четкое отделение пользовательского интерфейса от логики делает вещи намного проще.

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

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

Я знаю, что сделал.

Answer 6

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

Private Const Caption_Start As String = "Click to Start Doing the Thing"
Private Const Caption_Stop As String = "Click to Stop Doing the Thing"

Private Enum eStates
  State_Initialized
  State_Running
  State_Canceled
  State_Completed
End Enum

Private Current_State As eStates

Private Sub Form_Initialize()

  DoTheThing.Caption = Caption_Start
  Current_State = State_Initialized

End Sub

Private Sub DoTheThing_Click()

  If Current_State = State_Running Then

    'currently running - so set state to canceled, reset caption'
    'and disable button until loop can respond to the cancel'
    Current_State = State_Canceled
    DoTheThing.Caption = Caption_Start
    DoTheThing.Enabled = False

  Else

    'not running - so set state and caption'
    Current_State = State_Running
    DoTheThing.Caption = Caption_Stop

    'do the work'
    For i = 0 To ReallyBigNumber
      Call DoSomethingSomewhatTimeConsuming

      'at intervals check the state for cancel'
      If Current_State = State_Canceled Then
        're-enable button and bail out of the loop'
        DoTheThing.Enabled = True
        Exit For
      End If

      DoEvents

    Next

    'did we make it to the end without being canceled?'
    If Current_State <> State_Canceled Then
      Current_State = State_Completed
      DoTheThing.Caption = Caption_Start
    End If

  End If

End Sub

Answer 7

Если кому-то по какой-либо причине понадобится работать над вашим кодом, он не найдет практики и соглашения, с которыми он знаком и удобен, поэтому границ функциональности не будет. Другими словами, вы движетесь в неверном направлении на пути сцепления / сцепления. Функциональная интеграция государственного управления с пользовательским интерфейсом является классическим плакатом для этого вопроса.

Ты вообще понимаешь ООП? (Не критика, а законный вопрос. Если бы вы это сделали, это было бы намного понятнее для вас. Даже если это всего лишь VB6 ООП.)

Answer 8

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

Вы должны были бы добавить дополнительные константы, охватывающие и эти языки ... чистый ад. Данные GUI должны оставаться такими, какие они есть, данными GUI.

Метод OP, описанный здесь, напомнил мне, что сказал Генри Форд: «Любой покупатель может иметь машину, окрашенную в любой цвет, который ему нужен, если он черный».