9

Long methods are generally considered bad, however in my code I have some hard-to-understand long methods (more than 50 lines). I have trouble making those methods easier to read because a single statement inside is already more than 50 lines long, and that hard-to-read single statement is to build a database query using an ORM to do some specific job where the job done is clearly indicated on the method name. The reason that the statement is so long because it joins on multiple columns, applies multiple wheres and selects multiple distinct columns to make a required documented output format.

Is such hard-to-read code considered bad code? Similarly, if I write code for a complicated algorithm resulting in hard-to-read code wrapped in a clearly-named method, is that code bad code?

  • Isn't there a way for you to parametrize the query in some way? I'm guessing that this query varies depending on what's going on inside the method that creates. Maybe you can break it into smaller pieces and construct in several steps making easier to read. – Zalomon Jun 9 '17 at 8:11
  • Does your ORM support views? You can extract a (group of) join into a view and then select the view. Even if it the view isn't used elsewhere, that can help break up a big SQL statement – Caleth Jun 9 '17 at 9:07
  • Does your ORM support a SQL-like query language? If yes than you could move the query to its own file and enable IDE syntax highlighting for it. In your application load the query from the file. If your IDE doesn't exactly support that specific query language you may get along with SQL formatting even though that might not be perfect. However readability is largely increased by a good formatting. This also has the benefit that its easy to copy the query to a scratchpad and do modifications there. – SpaceTrucker Jun 12 '17 at 16:21
17

You wrote

Is such hard-to-read code considered bad code

so you definitely agree it is hard-to-read code, and if it is hard to read, it is hard to maintain and evolve - so I guess you consider the code as beeing "bad" by your own measures. However, sometimes it is not obvious how to improve something like a 50 line SQL statement. The easy "extract method" refactorings don't work, and you probably do not have a clue where to start in making the code more readable. For these cases, you can still try one or all of the following

  • show the code someone else who is more experienced than you in cleaning up code. If you do not have someone in your organization you can ask, try codereview.stackexchange

  • try to google for the specific problem. For your example, things like "clean up long sql statement" might be a good start. You will be astonished how many articles you find about that topic, and even if you cannot find a braindead recipe for your case, you might get some fresh ideas

  • instead of asking for a justification for the things you cannot do, focus on the things you can do to clean up the code at least a little bit, like adding proper line breaks, proper indentation, adding some explaining comments, giving some variables a better name. It is not unlikely, during this process, forcing yourself to reread the details of the code, you find a way to refactor the code into smaller units

  • practice, practice, practice. "Clean coding" is not something you learn in one day, it get easier with more experience. Maybe you do not find a solution for your problem today, but when you come back to the code in a few months, it will look differently to you.

  • I somewhat disagree with the comment part, if there is one complex part of code that is complex because it cannot be otherwise AND didn't found way to simplify it, comments are unlikely to give the big picture of what is being done. Document it with some diagram would be way better. And a comment that refer to that external documentation should be left. Of course those situation have to be exceptional because we al know that maintening external documentation is rarely done, so the fewer, the better. For the rest, a good answer as always. – Walfrat Jun 9 '17 at 7:58
  • @Walfrat: my intention was to provide a general guideline about the process, not exclusively for "50 lines of SQL code", and not as a "out-of-the box" solution with all potential approaches. – Doc Brown Jun 9 '17 at 11:45
  • I know, I just wanted to suggest that if something after being reviewed many times couldn't be simplified enough (whatever it is), comments will very likely not help about what make this thing complex so a minimal external documentation will likely be required. In the specific case of database querying this is even more easy by showing a diagram that shows how each part of the query correlate with each other. – Walfrat Jun 9 '17 at 12:23
4

Hard to read isn't bad - unnecessarily hard to read is bad.

Some things just are difficult. In that case, you need to completely understand the problem, write the code, and comment it as good as you can so the next developer has a chance.

But some things are only difficult because you made them difficult. If the problem can be simplified and made easier, simplify it. If it is hard to understand but can be reasonably split into subproblems, then split it into subproblems to simplify it.

  • Exactly. Try your best to make the code self-documenting, then use comments to fill in the gaps. (edited: I realized after posting my comment that the OP referred to ORM database queries, not SQL.) – Kyle A Jun 10 '17 at 13:57
1

ORM to create a report? Seriously? Learn some SQL, man. Procedural languages are terrible at this sort of thing.

SQL is a language much better designed to handle complicated joins and selects. And even if you can't get the SQL to look beautiful, there are all kinds of visualization tools available where you can drag and drop database objects on a diagram and the SQL will get generated for you. Not to mention you will be able to tune and optimize the query, view its query plan, get the platform to suggest additional indexing options, etc. It's just way more flexible.

I'm sure some folks on here will disagree with me, but ORM is not a good fit for complicated reporting purposes. If at all possible I would consider moving away from that and moving toward Structured Query Language.

  • 2
    Honestlly, the fact you do not like ORMs is completely irrelevant for the question. – Doc Brown Jun 9 '17 at 22:20
  • I like ORMs just fine. I am stating that they are not a good tool when the code "joins on multiple columns, applies multiple wheres and selects multiple distinct columns to make a required documented output format," which is the topic of this thread. – John Wu Jun 9 '17 at 22:30
0

In general hard to read code is a bad idea anywhere - even if you are the only maintainer - I have had several occurrences of returning to some code years or even weeks later and finding it hard to get my head around.

If you need to do a lot in a single query try splitting it across lines with embedded comments:

query(join(map(condition1, condition2), blah, blah, something(bah, blah, blah))) 

Becomes:

// Why are we doing such an awful single query rather than a sequence of selects?
query( // Description of query
  join( // Why are you doing a join here
    map( // Why a map
      condition1, // What is this
      condition2  // And this
   ), // End Map
   blah, // What, Why?
   blah, // What, Why?
   something( // What does this do?
      bah, // What, Why?
      blah, // What, Why?
      blah // What, Why?
      ) // End Something
   ) // End Join
) // End Query
  • I am ambiguous about your example. comments should explain why the code is like it is. The what should be expressed by the identifiers... – Timothy Truckle Jun 9 '17 at 8:06
  • @TimothyTruckle I agree that identifiers should clearly identify what they are but all to often they are unclear in normal code - in the case of record field names there is often a lack of clarity due to historic constraints I have come across cases were the field names were limited to 5 characters which all had to be upper case ASCII letters & they could not be changed due to compatibility requirements, e.g. with the MDs favourite reporting tool. – Steve Barnes Jun 9 '17 at 17:08
0

In addition to @DocBrown's excellent answer, I think it's worth recognising that nobody writes "good" code all the time. Coding is making trade-offs, and often it's better to accept that you've written something a little bit less clean than you'd like and come back to it later. Refactoring is the process of improving code over time - and in my experience, that's what makes a good code base, not "get it right first time".

And you assess code at the level of the application, not the level of individual methods/lines of code. So if you have a complex method, but it's clearly named, I don't think you have "bad" code as long as the method is cohesive.

Naming is the biggest weapon you have in making code intelligible - give your method a name which allows people reading your code to skip the body if they need to. Name your variables etc. in a way that means readers can see what they represent without having to read their assignment statements.

The next thing is to make sure your method really does only one thing - side effects make code hard to understand. So if your method returns data for an output format, it shouldn't also update the status of your database to "printed", or whatever.

Layering/componentization is the next thing you might do - if you have a bunch of complex methods which generate ORM results, bundle them together so readers of your code can assume that they all behave in the same way, have no side effects etc.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.