A Question of Style

Discussion in 'C Programming' started by Geoff, Sep 21, 2013.

  1. Geoff

    Jorgen Grahn Guest

    No; it was how I immediately interpreted it too. Who has anything
    against Italy these days?

    /Jorgen
     
    Jorgen Grahn, Sep 28, 2013
    #21
    1. Advertisements

  2. Geoff

    Ian Collins Guest


    Just using C++ where possible would save you a lot of pain :)
     
    Ian Collins, Sep 28, 2013
    #22
    1. Advertisements

  3. Geoff

    Rui Maciel Guest

    I looked at your alternative and saw the same thing: atrocious code in need of re-writing.
    You've declared single-purpose functions which only evaluate a single logical expression,
    write to a text stream and return a value, for something which only takes a couple of lines on
    a function and is executed once. In the process, motivated by a need to show off, you've
    turned a simple function which is straight forward and written in easily readable and
    maintainable code into a cryptic function which obscures the control flow for no good reason.

    You see, you're arguing style instead of substance, and your code only reflects your personal
    preferences and does absolutely nothing with regards to substance. Accusing someone else's
    code of being atrocious just because it doesn't fit your questionable personal preferences,
    particularly when they happen to worsen the quality of the code, is just bad form and does
    nothing to improve anything. At all.


    You see? It "seems much better" to you. You've made a baseless assertion regarding your own
    personal taste which reflects no technical aspect, with the only purpose of criticising
    someone else's code without actually doing anything to improve it, and in the process arguably
    worsening it.


    Style and substance are two entirely different things. One reflects personal tastes, whims
    and even technical limitations, the other tackles problems and provices appropriate ways to
    solve them. It doesn't make sense and helps no one to refer to personal opinions regarding
    style to criticise other people's work.


    Rui Maciel
     
    Rui Maciel, Sep 28, 2013
    #23
  4. (snip)
    Amanda Knox.
     
    glen herrmannsfeldt, Sep 28, 2013
    #24
  5. Geoff

    Rui Maciel Guest


    I'm starting to believe this goto phobia is a sign of incompetence, and reflects a need to
    blindly and mindlessly adopt beliefs which are believed to be held by a group in an effort to
    try to fit in, while showing a complete lack of critical thinking and insight on the issue.

    The code in question is perfectly fine. The goto statements are a classical example of how to
    properly use goto statements: they don't mangle the control flow, their use significantly
    enhance the readability of the code, and they provide an easy, simple and better yet localised
    way to perform all cleanup tasks required by that function. They are used to simply implement
    a destructor-like behavior. Anyone will be hard pressed to implement the exact same function
    without resorting to code duplication, complicating things with extra loops and conditionals,
    or declaring useless functions.

    Plus, your comment regarding "faster code" is complete nonsense. If you'd read the code you'd
    noticed that the goto statements are used by error handling functions for cleanup purposes, in
    cases such as malloc and fread failures. I don't believe anyone worries about speed when
    throwing a potentially and assuredly fatal error regarding memory allocation.

    There is plenty of software development best practices beyond the "go to considered harmful"
    paper, which is clearly a classical paper in the sense that everyone is aware if its existence
    and refers to it extensively, but never took the time to actually read it. Holding on to ill-
    conceived notions to criticise someone else's code is completely innapropriate and rude, and
    entirely unnecessary.


    Rui Maciel
     
    Rui Maciel, Sep 28, 2013
    #25
  6. Geoff

    Rosario1903 Guest

    for me you could be racist against italians etc

    not all italians are the same, but the 90% is *not* ok for me
    the way they follow is wrong for me
     
    Rosario1903, Sep 28, 2013
    #26
  7. Geoff

    Rosario1903 Guest

    what does it mean above?
    ....
    i not say i'm ok
     
    Rosario1903, Sep 28, 2013
    #27
  8. When you start declaring function pointer members of structures
    to allow for run-time resolution of bindings, it's time to consider
    whether you should move to C++. But the constructor / destructor
    paradigm works perfectly well in C.
    In modern C++ you can use vectors rather than arrays for variable-
    number members, which simplifies the destructor. In C everything
    has to be destroyed explicitly, which is a bit of a nuisance. But
    it's not too bad if you always have matching create / kill calls,
    all the complexity is in two places (the constructor and destructor).
     
    Malcolm McLean, Sep 28, 2013
    #28
  9. I agree.

    Goto is fine for handling error conditions. One of the problems in C is
    that there is no way of distinguishing error handling from normal control
    flow, which can make code hard to read because error handling has a way
    of dominating the paths. A goto helps to alleviate that problem.
     
    Malcolm McLean, Sep 28, 2013
    #29
  10. Geoff

    Ian Collins Guest

    Only in a manual and therefore error prone way. The big gain from C++
    is automatic object destruction which removes a whole class of
    programmer errors (and the need to ever use goto!). This is also just
    about the only trick in C++'s box that C can't replicate.
     
    Ian Collins, Sep 28, 2013
    #30
  11. Geoff

    Geoff Guest

    Actually, it was intended as a pun about loops, and those who would
    execute them.

    Ethnicity and nationality is not race.

    I would be crass to disparage Italians, after all I am ensconced in a
    continent discovered by an Italian with the help of Spaniards. I am
    descended from English and German, you may now begin disparaging my
    ethnic heritage.
     
    Geoff, Sep 29, 2013
    #31
  12. Geoff

    Ian Collins Guest

    So?

    Which is easier to test, spaghetti or single-purpose functions?

    Which is easier to understand and maintain, spaghetti or single-purpose
    functions?
     
    Ian Collins, Sep 29, 2013
    #32
  13. Geoff

    Geoff Guest

    Francesco Schettino didn't like sitting in his chair driving a
    predetermined course. He chose to play captain and ordered a deviation
    in a wide loop and hit a rock. He further turned back to Giglio in a
    U-turn and crashed into another rock in an attempt to save the ship. A
    captain's primary responsibility is the SAFETY of his ship and of the
    souls on board, it's not about salutes and parties or impressing
    mistresses.

    The salvage operation was in the news during this thread. It was
    merely on my mind while reading about the suggestion to write a loop.

    Rosario, it was not about incompetent Italians in general or even
    about you or your competence. It was about the word loop and the fact
    I find your coding style to be quirky and strange, synonyms for loopy.

    OT Aside: There was no way for Schettino to have known the contour of
    the sea floor approaching that harbor, there was no way for him to
    study the charts in sufficient detail to have decided to ground the
    ship where it ended up. He was trying to drive it into a grounding
    point within the harbor and actually ran it aground where it lays.
     
    Geoff, Sep 29, 2013
    #33
  14. Geoff

    Rui Maciel Guest

    I fail to see where how your comparison applies to this case, as the code presented by Geoff
    can't possibly be described as spaghetti code.


    Rui Maciel
     
    Rui Maciel, Sep 29, 2013
    #34
  15. Geoff

    Rosario1903 Guest

    than afther i have seen what is ok in the C++ language [class ini
    function and end function],
    why i prefer the same assembly?
     
    Rosario1903, Sep 29, 2013
    #35
  16. Geoff

    Geoff Guest

    All,
    In the end I took advantage of some internal knowledge of the Q2 engine and the fact it
    uses its own allocator and the game DLLs are supposed to be using that allocator instead
    of malloc/free. Q2's memory management posts diagnostic messages and terminates the
    program in case of errors and this makes it unnecessary to actually check and report
    errors when using it.

    Especially helpful in putting my thoughts on the right track were Tim's use of __func__,
    and Ben's note that guarding fclose(f) was a good idea and I quite agree with James Kuyper
    on the value of single-return being desirable even without being dogmatic about it.

    This is very old code and by necessity I am dealing with Microsoft's compiler and their
    lack of support for newer C standards but I managed to incorporate some of them.

    To that end I attempt the following rewrite, apologizing for the line length:

    void TeamplaySpawnEntities (char *mapname, char *entities, char *spawnpoint)
    {
    FILE *f;
    char filename[MAX_QPATH];
    int nEntSize, nRead;
    char *pszCustomEnt;

    // Create pathname to the entity file and try to open it.
    Com_sprintf (filename, MAX_QPATH, "%s/ent/%s.ent", gamedir->string, mapname);
    f = fopen (filename, "rb");

    // if we don't have a custom file or we're not
    // configured to load one, just spawn the defaults
    if (!f || (!custom_ents->value && !ctf->value))
    {
    SpawnEntities (mapname, entities, spawnpoint);
    return;
    }

    // Get the size of the file.
    if (fseek (f, 0, SEEK_END) != 0)
    gi.dprintf ("ERROR: %s: fseek %s\n", __func__, filename);
    else if ((nEntSize = ftell (f)) < 0)
    gi.dprintf ("ERROR: %s: ftell %s (%d)\n", __func__, filename, nEntSize);
    else if (fseek (f, 0, SEEK_SET) != 0)
    gi.dprintf ("ERROR: %s: fseek %s\n", __func__, filename);
    else
    // allocate space for the entities we are about to read
    // if gi.TagMalloc fails, it reports error and terminates the program
    pszCustomEnt = gi.TagMalloc (nEntSize + 1, TAG_LEVEL);

    if ((nRead = fread (pszCustomEnt, 1, nEntSize, f)) != nEntSize)
    {
    gi.dprintf ("ERROR: %s: fread %s (%d/%d) not all entities were loaded\n",
    __func__, filename, nRead, nEntSize);
    gi.TagFree (pszCustomEnt);
    }

    // Null-terminate the string.
    pszCustomEnt[nEntSize] = '\0';

    // Now spawn *these* entities!
    SpawnEntities (mapname, pszCustomEnt, spawnpoint);

    if (f) fclose (f);
    }

    I believe the guarding of fclose is unnecessary given the refactoring and may even be a
    little annoying since there is only one fopen/fclose path.

    I am open to more suggestions about how to improve this code. I know some of you will
    probably have something to say about bracketing the statements for clarity or
    maintainability. I have tested it but have not tested all the paths yet.

    If you can throw in a few quips about my Teutonic and Saxon origins so I can call you
    racist I would very much appreciate it. I know Europeans would be the last people on Earth
    to cast aspersions on someone because of their ethnicity or religion. Especially in the
    21st century. :)
     
    Geoff, Sep 29, 2013
    #36
  17. Geoff

    Rosario1903 Guest

    if(hereCallAnotherfunction()==-1)
    goto errFree;

    how you can see the path that follow routine is as usual

    ||
    ||
    ||
    ||
    \/

    instead the error path goes in reverse

    /\
    ||
    ||
    ||
    ||
     
    Rosario1903, Sep 29, 2013
    #37
  18. Two minor things. I suggested it as a way to permit the flow of control
    to pass, always, to the lexical end of the function. You don't do that
    below, which seems to have introduced a bug.

    In fact I'd almost always write multiple functions to do this kind of
    thing. I only didn't do so here because I wanted to make a point about
    the particular linear structure you had.

    There seems to be opposition to single-use functions, but I use them
    quite a lot. They encapsulate the parts of a more complex task in a
    particularly easy to understand way. Function that must succeed or fail
    depending on various conditions can be written very clearly in terms of
    function calls:

    return do_this(...) && then_this(...) && and_finally();

    or maybe

    return do_something_with(a_resource(...), ...) || fallback_here(...);

    and so on.

    Unfortunately C makes it harder than it should be to do this. You can't
    define one function inside another, and because you have to declare a
    function before calling it, you either end up writing the small parts
    before the big picture or you have to write declarations as well as
    definitions.
    You can get here with f != NULL and so never close the file. The whole
    point I was making was to use an if ... else chain so you always got to
    the end. Hence the use of if (f) fclose(f).
    Quite. Either remove it or fix the problem above in which case it
    becomes useful again.
    Testing for files left open is hard. There are lots of problems that
    don't show up in simple tests, which is why I put so much store on
    clarity so that the problem can be avoided as early as possible.
    I am more tempted to make a quip about suitable times to stop digging...
     
    Ben Bacarisse, Sep 29, 2013
    #38
  19. Geoff

    Geoff Guest

    You are quite right. There are two.
    I understand. In this last cut I was hoping to preserve the linear
    structure but I can see the point of breaking it up but not to the extent
    that Tim did.
    I once worked on a project in Z80 assembler where there was heavy use of
    macros. The macro calls were interspersed with the appropriate register
    initializations and it was like working with a high level language. Beautiful.

    Re-written yet again.

    static size_t getEntityFileSize(const char *filename, FILE *f)
    {
    size_t size;

    // Get the size of the file.
    if (fseek (f, 0, SEEK_END) != 0) {
    gi.dprintf ("ERROR: %s: fseek %s\n", __func__, filename);
    return 0;
    }
    else if ((size = ftell (f)) < 0) {
    gi.dprintf ("ERROR: %s: ftell %s (%d)\n", __func__, filename, size);
    return 0;
    }
    else if (fseek (f, 0, SEEK_SET) != 0) {
    gi.dprintf ("ERROR: %s: fseek %s\n", __func__, filename);
    return 0;
    }
    else
    return size;
    }

    void TeamplaySpawnEntities (char *mapname, char *entities, char *spawnpoint)
    {
    FILE *f;
    char filename[MAX_QPATH];
    size_t nEntSize;
    size_t nRead;
    char *pszCustomEnt;
    int err;

    if (!custom_ents->value && !ctf->value)
    {
    SpawnEntities (mapname, entities, spawnpoint);
    return;
    }

    /* Create path to the entity file and try to open it. */
    Com_sprintf (filename, MAX_QPATH, "%s/ent/%s.ent", gamedir->string, mapname);
    if ((f = fopen (filename, "rb")) != NULL)
    {
    nEntSize = getEntityFileSize(filename, f);
    if (nEntSize != 0)
    /* if gi.TagMalloc fails it logs the error, aborts process */
    pszCustomEnt = gi.TagMalloc (nEntSize + 1, TAG_LEVEL);

    nRead = fread (pszCustomEnt, 1, nEntSize, f);
    if (nRead != nEntSize)
    {
    gi.dprintf ("ERROR: %s: fread %s (%d/%d) not all entities were loaded\n",
    __func__, filename, nRead, nEntSize);
    gi.TagFree (pszCustomEnt);
    err = true;
    }

    if (!err)
    {
    pszCustomEnt[nEntSize] = '\0';
    SpawnEntities (mapname, pszCustomEnt, spawnpoint);
    }
    fclose (f);
    }
    }
     
    Geoff, Sep 29, 2013
    #39
  20. Geoff

    Jorgen Grahn Guest

    Odd. I think I'm seeing the opposite in recent years: "Wow, I can
    fit in *and* break taboos at the same time, by using goto for error
    handling! The Linux kernel does it so I can claim superiority by
    association!"

    I don't mind this when goto /is/ clearly the best approach. Or when
    there wasn't time to do it better. I /do/ mind when the author didn't
    find the better design (probably breaking the function down) simply
    because he wasn't looking.

    /Jorgen
     
    Jorgen Grahn, Sep 30, 2013
    #40
    1. Advertisements

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments (here). After that, you can post your question and our members will help you out.