Middle

public bool BookTour(string userId, DateTime start, DateTime end, int hotelId, string roomType, string flightClass, int airCompanyId, int flightnumber) { try { //flightClass also not desired to parse enum var flightInfo = new FlightInfo(Enum.Parse<FlightClass>(flightClass), airCompanyId, flightnumber); var resultBookFlight = _airWaysService.Book(GetUser(userId), flightInfo); //roomType also not desired to parse enum var rentInfo = new RentInfo(start, end, hotelId, Enum.Parse<RoomType>(roomType)); //GetUserId in a separate variable var resultBookHotel = _hotelService.Book(GetUser(userId), rentInfo); //Exceptions will go to inner, not caught Task.WhenAll(resultBookFlight, resultBookHotel); //Why static reservationIdAir = resultBookFlight.Result; _reservationIdHotel = resultBookHotel.Result; } catch (BookFlightException) { _hotelService.CancelBook(_reservationIdAir); return false; } catch (BookHotelException) { _airWaysService.CancelBook(_reservationIdHotel); return false; } catch (Exception ex) { throw new ApplicationException(ex.Message); } finally { //fields are 100% uninitialized //Why do we try to save something after exception //GetUser already extracted unitOfWork.Execute(async () => { await _tourRepository.Save(_reservationIdAir, _reservationIdHotel, GetUser(userId)); }).RunSynchronously(); //Interlock.Increment() _countBooked++; } return true; }

sobes.tech AI

Answer from AI

The provided code has several issues and areas for improvement:

Parsing enum from strings — Enum.Parse<FlightClass>(flightClass) and Enum.Parse<RoomType>(roomType) can throw exceptions if the string is invalid. It's better to use Enum.TryParse with error handling or consider a safer way to pass these values (e.g., directly as enum or with a validator).
Duplicate call to GetUser(userId) — it's better to call it once and store the result in a variable.
Asynchronous calls and Task.WhenAll — calling Task.WhenAll(resultBookFlight, resultBookHotel); without await does not wait for completion. This means errors might not be caught, and results may not be available in time.
Exception handling and booking cancellation — in catch blocks, cancellation is called, but parameters are mixed up (_hotelService.CancelBook(_reservationIdAir); and vice versa). This is an error.
Use of static fields _reservationIdAir and _reservationIdHotel — if the method is called concurrently, this can lead to race conditions. It's better to use local variables.
Calling asynchronous code with RunSynchronously() in finally block — this is bad practice and can lead to deadlocks. It's better to make the method asynchronous and use await.
Saving data and incrementing the booking counter even on exceptions — it's logical to do this only on successful booking.

An example of a corrected (simplified) version:

public async Task<bool> BookTourAsync(string userId, DateTime start, DateTime end, int hotelId, string roomType, string flightClass, int airCompanyId, int flightNumber)
{
    var user = GetUser(userId);

    if (!Enum.TryParse<FlightClass>(flightClass, out var parsedFlightClass))
        return false; // or throw exception
    if (!Enum.TryParse<RoomType>(roomType, out var parsedRoomType))
        return false;

    var flightInfo = new FlightInfo(parsedFlightClass, airCompanyId, flightNumber);
    var rentInfo = new RentInfo(start, end, hotelId, parsedRoomType);

    string reservationIdAir = null;
    string reservationIdHotel = null;

    try
    {
        var bookFlightTask = _airWaysService.Book(user, flightInfo);
        var bookHotelTask = _hotelService.Book(user, rentInfo);

        await Task.WhenAll(bookFlightTask, bookHotelTask);

        reservationIdAir = bookFlightTask.Result;
        reservationIdHotel = bookHotelTask.Result;

        await unitOfWork.Execute(async () =>
        {
            await _tourRepository.Save(reservationIdAir, reservationIdHotel, user);
        });

        Interlocked.Increment(ref _countBooked);

        return true;
    }
    catch (BookFlightException)
    {
        if (reservationIdHotel != null)
            await _hotelService.CancelBook(reservationIdHotel);
        return false;
    }
    catch (BookHotelException)
    {
        if (reservationIdAir != null)
            await _airWaysService.CancelBook(reservationIdAir);
        return false;
    }
    catch (Exception ex)
    {
        throw new ApplicationException(ex.Message, ex);
    }
}

This approach improves readability, safety, and correctness in handling asynchronous operations and exceptions.

Previous Next