Skip to Content

From my experience when companies talk about testing their ABAP developments they are talking about functional black box testing: someone, not the developer, will test the development to make sure it complies with the requirements. During this phase the tester will not, of course, look at the code.

I think code reviews / inspections, be they more or less formal, might have been more common in the past but have now fallen out of fashion. To me, that is a pity. I do not know if SAP AG, for example, has reviewers for its own ABAP code, but judging from some examples I would bet it doesn’t (at least not for all the code). A good book publisher would not publish a book without having it reviewed, as a good magazine would not live without copy editors, so why should software companies publish code that was never seen by anyone else other than the original programmer?

So why out of fashion? Well, for several reasons: agile methodologies focus on very short cycles; test driven development basically means that if it doesn’t show up on tests then it is not important; and I suppose programmer egos have always been against having someone else reading their code and daring to suggest improvements.

I believe that functional tests do not replace code reviews. The different techniques are complimentary, and some errors will never be caught by a functional tester (which is, though, what most companies seem to rely on). But let me give an example.

We recently had a problem with code that was meant to delete obsolete entries from a customer table:

      LOOP AT it_seq INTO l_seq.
        so-seq-sign   = ‘I’
        so-seq-option = ‘EQ’.
        so_seq-low    = l_seq-seq.
        APPEND so_seq.
      ENDLOOP.
      DELETE FROM ztable WHERE seq IN so_seq.      

As experienced abapers know, the problem is that if the range has too many entries this will dump. This original code probably wouldn’t have survived a code review (though it would pass most tests), but this is still not the point I am trying to make. Since this was causing dumps, a correction was made; this correction passed all tests and we caught it just before being installed in production. This was the correction (ok, there could be other solutions, but still that is not the point here):


      CLEAR l_index.
      LOOP AT it_seq INTO l_seq.
        so_seq-sign   = 'I'.
        so_seq-option = 'EQ'.
        so_seq-low    = l_seq-seq.
        APPEND so_seq.
        l_index = l_index + 1.
        CHECK l_index = 1000.
        DELETE FROM ztable WHERE seq IN so_seq.
        REFRESH so_seq. CLEAR l_index.
      ENDLOOP.
      DELETE FROM ztable WHERe seq IN so_seq.

Instead of looping all the entries and only deleting in the end, it deletes every 1000 records and then after the loop deletes the remaining records. The logic seems simple, right?

Well, wrong. This code will work unless it_seq has a multiple of 1000 records; if it does, then the last DELETE will be executed with so_seq empty, and will in fact delete the entire contents of the table.

Would this ever be detected by a functional test? I doubt it: only by a great coincidence the test would be done with a multiple of 1000 records.

Would this cause a problem in production? Well, of course: as everybody knows, the very first execution in production would be with 2,000 records and the entire table contents would be gone.

To report this post you need to login first.

8 Comments

You must be Logged on to comment or reply to a post.

  1. Steve Oldner
    We have 2 reviews, a technical and functional, for each program/report I code.  Tech review depends on what was changed.  Sometimes it is only the code that was changed and a cursory review of know issues like “for all entries.”

    I agree that SAP might want to be a better leader in this area.

    (0) 
    1. Rui Pedro Dantas Post author
      Hi Steven,
      Thank you for the feedback.

      I am curious how this works in other places. Where you work, who does this job? An expert or just another programmer? One person or more than one? Is there a team dedicated for this?

      (0) 
      1. Steve Oldner
        For tech reviews, we have tech ABAP contractors/consultants that review.  They both have been here over 5 years and 2 implementations of ours so are really knowledgeable about us. Supposedly there are more independent, but they both have worked for other companies and have a broader ABAP experience than me.  For functional testing, it is our functional (employee)SMEs that test.

        So 2 expert programmers, both dedicated to coding reviews.  And they are worth it.  I’ve seen my coding vastly improve over the years due to the tech reviews.   

        (0) 
  2. Vijay Vijayasankar
    Code reviews are a vital part of the life cycle – in SAP and non-SAP projects.

    Even if all the errors get caught in a black box type functional test, it might still be the case that code is not maintainable. Development cost is only one part of TCO – maintenance is a long term pain if code was poorly written.

    (0) 
  3. Manas Dua
    Hi,

    For Agile methodolgy in each sprint code review is a high priority task to be finished by scrum team.

    Being a part of development team earlier, I can say with my experience that all the important sections of code undergo code review thoroughly.

    At SAP, code review is done rigourously and mandatorily and we have mature process for it. 

    Each code review session invloves presence of Expert, Developer, Code Reader and Moderator. All the comments\ bugs founded are recorded and monitored till completion.

    Many a times small errors are found in code reviews itself.

    Things like usage of Constants, Progarmming Model, Readability and Modularity of Code, Code Commenting, Checking for translation related literals, simple flow logic, exception handling are things which are checked with expert eyes.

    (0) 
    1. Rui Pedro Dantas Post author
      Hi Manas,

      I am certainly not an agile expert, but in some presentations I had strong supporters of Agile admit that architectural issues are sometimes ignored because you often also assume that you are building code to be quickly replaced in the future to respond to changing requirements, and not something built to last.

      Glad to hear, though, that code review tasks are included in each sprint.

      (0) 
  4. Harald Boeing
    Hi Rui,
    A proper code review requires a qualified developer and reviewing a sufficiently complex application will take quite some time. Somebody has to be willing to pay for that time and in my experience that rarely happens (let’s see if others contradict). So it seems to me that most often the easy way out is to reduce code reviews to static code analysis. And even though this is a good first step, this is obviously not enough to catch all problems.

    I’m personally not sure, if thorough code reviews with the goal of detecting bugs are really worth it. Are enough severe problems discovered to justify the additional time? However, if one sees this as an opportunity for passing on knowledge and sharing it, then I think we’re getting closer to some real value.

    Developers should explain their solution to others and walk them through the implementation. This process often allows the developer to discover problems before even the reviewer spots it. Furthermore the exchange about style, possible different or better approaches, etc. usually increases the skill set throughout the team. And of course ongoing application support is much easier, because now there’s more people who have a good background about the application (thus turn-around time for support incidents probably decreases, because you don’t have that one-person bottleneck).

    Cheers, harald

    (0) 
    1. Rui Pedro Dantas Post author
      Hi Harald,

      Thank you for your inputs. From what I’ve seen I also think that having reviews is not that normal, but from the first comments it seemed it was. Let’s see if there are more opinions.

      My example actually shows something that would not be caught by testing, and something that could be quite severe (a table would be completely deleted). I don’t catch those kind of things everyday, of course, but they do happen.

      I agree that code reviews are great for knowledge sharing, but I am not sure if over the shoulder reviews, with the original developer walking through his own code, is the best thing. I think that by hearing his explanation you tend to see things by his eyes, so the “fresh eyes” advantage is gone.

      Regards, Rui

      (0) 

Leave a Reply