Это явное использование goto? - PullRequest
7 голосов
/ 19 ноября 2009

Просто интересно, считается ли это очевидным использованием goto в C #:

IDatabase database = null;

LoadDatabase:
try
{
    database = databaseLoader.LoadDatabase();
}
catch(DatabaseLoaderException e)
{
    var connector = _userInteractor.GetDatabaseConnector();
    if(connector == null)
        throw new ConfigException("Could not load the database specified in your config file.");
    databaseLoader = DatabaseLoaderFacade.GetDatabaseLoader(connector);
    goto LoadDatabase;
}

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

Редактировать: Это было быстро. Чтобы ответить на несколько вопросов и немного прояснить ситуацию - это часть процесса, который по сути превращается из проекта другого типа. Вызов _userInteractor.GetDatabaseConnector () - это часть, которая определяет, хочет ли пользователь повторить попытку (возможно, с базой данных, отличной от той, что в конфигурации, из которой они загружаются). Если он возвращает null, то не было указано новое соединение с базой данных, и операция должна завершиться неудачей полностью.

Понятия не имею, почему я не подумал об использовании цикла while. Должно быть, слишком близко к 5 вечера.

Редактировать 2: Я посмотрел на метод LoadDatabase (), и он выдаст DatabaseLoaderException в случае сбоя. Я обновил код выше, чтобы перехватывать это исключение вместо исключения.

Редактировать 3: Общее мнение, как представляется, заключается в том, что

  • Использование goto здесь не обязательно - цикл while будет работать нормально.
  • Использование таких исключений не очень хорошая идея - хотя я не уверен, что заменить его.

Ответы [ 7 ]

15 голосов
/ 19 ноября 2009

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

Да, в коде вызова. Пусть вызывающий этот метод решит, нужно ли ему повторить логику или нет.

UPDATE:

Чтобы уточнить, вы должны ловить исключения, только если вы действительно можете их обработать. Ваш код в основном говорит:

«Я понятия не имею, что случилось, но что бы я ни делал, все это вызывало взорвать ... так что давайте сделаем это снова. "

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

ОБНОВЛЕНИЕ 2:

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

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

//The main thread might look something like this

try{
    var database = LoadDatabaseFromUserInput();

    //Do other stuff with database
}
catch(Exception ex){
    //Since this is probably the highest layer,
    // then we have no clue what just happened
    Logger.Critical(ex);
    DisplayTheIHaveNoIdeaWhatJustHappenedAndAmGoingToCrashNowMessageToTheUser(ex);
}

//And here is the implementation

public IDatabase LoadDatabaseFromUserInput(){

    IDatabase database = null;
    userHasGivenUpAndQuit = false;

    //Do looping close to the control (in this case the user)
    do{
        try{
            //Wait for user input
            GetUserInput();

            //Check user input for validity
            CheckConfigFile();
            CheckDatabaseConnection();

            //This line shouldn't fail, but if it does we are
            // going to let it bubble up to the next layer because
            // we don't know what just happened
            database = LoadDatabaseFromSettings();
        }
        catch(ConfigFileException ex){
            Logger.Warning(ex);
            DisplayUserFriendlyMessage(ex);
        }
        catch(CouldNotConnectToDatabaseException ex){
            Logger.Warning(ex);
            DisplayUserFriendlyMessage(ex);
        }
        finally{
            //Clean up any resources here
        }
    }while(database != null); 
}

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

Ура, Джош

7 голосов
/ 19 ноября 2009

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

IDatabase database = null;

while(database == null){
   try
   {
        database = databaseLoader.LoadDatabase();
   }
   catch(Exception e)
   {
        var connector = _userInteractor.GetDatabaseConnector();
        if(connector == null)
                throw new ConfigException("Could not load the database specified in your config file.");
        databaseLoader = DatabaseLoaderFacade.GetDatabaseLoader(connector);
        //just in case??
        database = null;
   }
 }

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

4 голосов
/ 19 ноября 2009

Лично у меня это было бы в отдельном методе, который возвращает код состояния успеха или неудачи. Затем в коде, который будет вызывать этот метод, у меня может быть какое-то магическое число попыток повторять это до тех пор, пока код состояния не станет «Успех». Мне просто не нравится использовать try / catch для управления потоком.

2 голосов
/ 19 ноября 2009

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

IDatabase loadedDatabase = null;

// first try
try
{
    loadedDatabase = databaseLoader.LoadDatabase();
}
catch(Exception e) { }  // THIS IS BAD DON'T DO THIS

// second try
if(loadedDatabase == null) 
{
    var connector = _userInteractor.GetDatabaseConnector();
    if(connector == null)
        throw new ConfigException("Could not load the database specified in your config file.");
    databaseLoader = DatabaseLoaderFacade.GetDatabaseLoader(connector);
    loadedDatabase = databaseLoader.LoadDatabase()
}

Это более четко иллюстрирует, что вы на самом деле делаете. В качестве дополнительного бонуса, другие программисты не будут выбивать вам глаза. :)

ПРИМЕЧАНИЕ: вы почти наверняка не хотите поймать Exception. Вероятно, есть более конкретное исключение, которое вы бы предпочли поймать. Это также перехватывает исключение TheComputerIsOnFireException, после чего повторять его не стоит.

1 голос
/ 19 ноября 2009

Обязательный XKCD

1 голос
/ 19 ноября 2009

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

Технически нет ничего плохого в вашей структуре goto, но для меня я бы выбрал вместо этого цикл while. Что-то вроде:

IDatabase database = null;

bool bSuccess = false;
int iTries = 0
while (!bSuccess) // or while (database == null)
{
    try
    {
        iTries++;
        database = databaseLoader.LoadDatabase();
        bSuccess = true;
    }
    catch(DatabaseLoaderException e)
    {
        //Avoid an endless loop
        if (iTries > 10)
             throw e;

        var connector = _userInteractor.GetDatabaseConnector();
        if(connector == null)
             throw new ConfigException("Could not load the database specified in your config file.");
        databaseLoader = DatabaseLoaderFacade.GetDatabaseLoader(connector);
    }
}
1 голос
/ 19 ноября 2009

Нет, все не в порядке: http://xkcd.com/292/

Добро пожаловать на сайт PullRequest, где вы можете задавать вопросы и получать ответы от других членов сообщества.
...