Skip code without using state variable, using goto envisaged

Question

I have a code which has parts that mustn't be executed if there was an error before in the code. I actually use a bool variable called EndProg that, if set to true, will instruct the program to avoid executing some parts of code.

My problem is that I don't want to use this method and I'd prefer to use goto instead because it will make the program jump to the cleanup part and avoid checking EndProg value multiple times.

The other problem is that I've read on many pages on StackOverflow and other websites that using goto is considered a bad practice and that it can make a code more difficult to read or create errors.

My code is simple enough and I will need to use just one label so I doubt that this will create problems; but I would like to know if there are other ways to do what I want without creating functions to do cleanup tasks or using return (because, for example, I will need to write the cleanup code several times) and I also don't want to write the same big cleanup code in multiple places and then use return or do something else.

I don't want to increase the number of lines of code nor use return nor use a lot of if nor check the value of a state variable. What would you recommend ?

Here's a piece of code :

bool EndProg=false;
/*
    Lot of code that can set EndProg to true
*/
ClassType ClassName;
if(!EndProg && LoadConf(&ConfFilePath,&ClassName)==0)
{
    int fildes=-1;
    if(ClassName.abc) // bool
    {
        if(ClassName.FilePath==0) // char *
        {
            ClassName.FilePath=new(std::nothrow) char[9]();
            if(ClassName.FilePath!=0)strcpy(ClassName.FilePath,"file.ext");
            else EndProg=true;
        }
        if(!EndProg && mkfifo(ClassName.FilePath,S_IRUSR | S_IWUSR)==-1)
        {
            if(errno==EEXIST)
            {
                /* EEXIST is returned if the file already exists
                We continue, later we will try to open this file */
            }
            else EndProg=true;
        }
        if(!EndProg && (fildes=open(ClassName.FilePath,O_RDWR))==-1)EndProg=true;
    }
    /*
    Lot of code that will check if EndProg == true
    */
}
delete[] ClassName.FilePath;
delete[] ConfFilePath;

What I would like to do is :

bool EndProg=false;
/*
    Lot of code that can set EndProg to true
*/
ClassType ClassName;
if(LoadConf(&ConfFilePath,&ClassName)==0)
{
    int fildes=-1;
    if(ClassName.abc) // bool
    {
        if(ClassName.FilePath==0) // char *
        {
            ClassName.FilePath=new(std::nothrow) char[9]();
            if(ClassName.FilePath==0)goto cleanup;
            strcpy(ClassName.FilePath,"file.ext");
        }
        if(mkfifo(ClassName.FilePath,S_IRUSR | S_IWUSR)==-1)
        {
            if(errno==EEXIST)
            {
                /* EEXIST is returned if the file already exists
                We continue, later we will try to open this file */
            }
            else goto cleanup;
        }
        if((fildes=open(ClassName.FilePath,O_RDWR))==-1)goto cleanup;
    }
    /*
    Lot of code that will check if EndProg == true
    */
}
cleanup:
delete[] ClassName.FilePath;
delete[] ConfFilePath;

As you can see it isn't difficult to understand and even if searching the label can be a problem for someone, it isn't for me; and I don't plan to make the code public.

Update :

I decided to using exceptions and it works for some parts of my original code. But I doubt this will be easy to implement in more complex parts. Thanks for your answers.

*"I don't plan to make the code public"* In general, try to avoid this mentality. Don't use bad programming practice just because this particular code won't be shared. Get in the habit of using good programming from the start. — Cory Kramer, May 23 '14 at 20:25
Why is `ClassName` not taking care of cleaning up its own resources in its destructor? [RAII](http://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization). — Benjamin Lindley, May 23 '14 at 20:36
@BenjaminLindley `ClassName` destroys `FilePath` but other functions can modify it like destroying, allocating, writing, etc. — Echren, May 23 '14 at 20:45
Well, that's terrible. What is `FilePath`, a `char*`? Why is it not a `std::string`? — Benjamin Lindley, May 23 '14 at 20:51
@BenjaminLindley I use `char` because that's what I learned first when I started programming. As I said elsewhere, I learned programming by myself and there are a lot of things I don't know yet (like using `std::string`). I'm comfortable with `strcpy()`, `strcat()`, `strcmp()`, `strlen()` and others and the compiler doesn't tell me : "No you can't do this with std::string, no you can't do that, no this... no that...". — Echren, May 23 '14 at 21:10

Scis · Accepted Answer · 2014-05-23T21:54:40.053

6

Since you've tagged this question c++ as well I'd go with using exceptions and a try catch block.

You can find a lot of useful information about the subject on SO and on other websites:

Here is a very basic tutorial.

And here is a nice and basic FAQ that might help you as well.

Basically there's nothing to fear, exceptions are not cryptic and in fact make more sense when you get the hang of it. Because basically this concept enables you to achieve exactly what you want:

Several pitfalls that can be handled by the same error handling code.

Edit: For example if I'd move the mkfifo etc. into a function (in general creating a function for each well defined logical block is clearer and more readable) and have something like

This is just a sketch to give you a general idea:

#include <exception>


functionThatDoesMakeFifo(...){
   // check which ever conditions you want to check after mkfifo
   // if one of them goes wrong just do:
   throw std::exception();

}

// this is inside your function:
   ClassType ClassName;       
   try{
      ClassName.FilePath = new char[9](); // even though I'd use a string...
      .
      .
      . // rest of the code
   } catch(std::exception &e){    
       delete [] ClassName.FilePath;
       delete [] ConfFilePath;
       ClassName.FilePath = NULL; // not mandatory just my habit
       ConfFilePath = NULL;
   }

edited May 23 '14 at 21:54

answered May 23 '14 at 20:23

Scis

2,934
3
23
37

I learned programming by myself and I still don't know what exceptions are. They're somewhat cryptic to me and I have difficulties understanding what to write in the `try` part. I prefer to avoid them for the moment. I'll learn about them when I'll feel ready. – Echren May 23 '14 at 20:30
I would suggest to learn them then. They are designed to do EXACTLY what your problem is: collecting errors from arbitrary complex program, reporting them and doing cleanup. – CygnusX1 May 23 '14 at 20:34
exceptions can get messy when the code becomes full of try and catch blocks, and like any code just requires a bit of care and used sensibly, but can remove a lot of conditional error checking. I like to think of exceptions as used to deal with exceptional situations... – May 23 '14 at 20:35
If someone tells me the code should get messy, then they have not thought about solving the problem in a cleaner way. I could point out many ways of improving this example, but that was not the question. – May 23 '14 at 20:45
@Curg It was a typo I meant **should not**, and the not got whacked somehow in the process of editing sorry for the confusing reply :) What I originaly wanted to say is that it should not get messy if he neatly packs some of this long code into functions which throw exceptions on error and because he has a single way of recovering from those errors there should not be a lot of catch clauses which might make it messy – Scis May 23 '14 at 20:53
Ah yes this I agree with, although the use of exceptions is debatable for the example given. – May 23 '14 at 20:56
I have difficulties to see how to use try-catch in this code. Do I have to place each `if` block in a `try` block like "try { if(ClassName...){...} } try { if(mkfifo...){...} }" etc ? How do I place `catch`. And, I think the try-catch solution will add a lot of lines. – Echren May 23 '14 at 21:37
@ChkLst I've tried to sketch up a general idea of how should this look + I've added a link to a tutorial (and the internet is filled with plenty more, just google "c++ exceptions" maybe with "tutorial"). This should not add a lot of lines. And even if a few more lines make your code easier to understand and to debug it's worth it :) – Scis May 23 '14 at 21:58

score 1 · Answer 2 · edited May 23 '17 at 11:45

I would try with something like Scope Guards or BOOST_SCOPE_EXIT (C++) or its C++11 analogue:

template<class F>
struct ScopeExit
{
  ScopeExit(F f) : f(f) {}
  ~ScopeExit() { f(); }
  F f;
};

template<class F>
ScopeExit<F> MakeScopeExit(F f) { return ScopeExit<F>(f); }

#define STRING_JOIN2(arg1, arg2) DO_STRING_JOIN2(arg1, arg2)
#define DO_STRING_JOIN2(arg1, arg2) arg1 ## arg2
#define SCOPE_EXIT(code) \
  auto STRING_JOIN2(scope_exit_, __LINE__) = MakeScopeExit([=](){code;})


bool myfunct()
{
  ClassType ClassName;

  ClassName.FilePath = 0;
  ConfFilePath = 0;
  SCOPE_EXIT(delete [] ClassName.FilePath; delete [] ConfFilePath; );

  if (LoadConf(&ConfFilePath,&ClassName) == 0)
  {
    int fildes=-1;
    if(ClassName.abc) // bool
    {
      if(ClassName.FilePath==0) // char *
      {
        ClassName.FilePath=new(std::nothrow) char[9]();
        if(ClassName.FilePath==0) return false;
        strcpy(ClassName.FilePath,"file.ext");
      }
      if(mkfifo(ClassName.FilePath,S_IRUSR | S_IWUSR)==-1)
      {
        if (errno==EEXIST)
        {
          /* EEXIST is returned if the file already exists
          We continue, later we will try to open this file */
        }
        else return false;
      }
      if((fildes=open(ClassName.FilePath,O_RDWR))==-1) return false;
    }
    /*
    Lot of code that will check if EndProg == true
    */
  }

  return true;
}

I'm using return but the cleanup code is just in one place.

Anyway ClassName should take care of cleaning up its own resources in the destructor.

score 0 · Answer 3 · answered May 23 '14 at 20:33

There is a little trick I have seen before that might help you solve this, although I am personally not a fan of tricks, it might be appropriate for what you require.

while (true)
{
    if(ClassName.FilePath==0) // char *
    {
        ClassName.FilePath=new(std::nothrow) char[9]();
        if(ClassName.FilePath==0) break;
        strcpy(ClassName.FilePath,"file.ext");
    }
    if(mkfifo(ClassName.FilePath,S_IRUSR | S_IWUSR)==-1)
    {
        if(errno==EEXIST)
        {
            /* EEXIST is returned if the file already exists
            We continue, later we will try to open this file */
        }
        else break;
    }
    if((fildes=open(ClassName.FilePath,O_RDWR))==-1) break;
    /*
    Lot of code that will check if EndProg == true
    */
    break;
}

delete[] ClassName.FilePath;
delete[] ConfFilePath;

But again I am not condoning this as a graceful solution, I personally would re-write your code and break it down into something more readable. But then again I don't write functions containing hundreds of lines either.

score 0 · Answer 4 · answered May 24 '14 at 00:35

I may get downvoted for this, but I think that limited use of goto in C is not evil. Particularly, what you are talking about is quite acceptable: branching forward to clean up code on errors. I'd suggest that you limit this to a single target label per routine.

What people hate (justifiably) is the old fastion spaghetti code with goto's jumping all over the place.

Skip code without using state variable, using goto envisaged

4 Answers4