Have you turned down work because of quality of source code

[snarkysparky]

I'm in a position where I may get the opportunity to go full time to work on a software project but the code base I will have to work with
gives me nightmares.   Haven't had to change it yet because we don't have a compile system but I have had to spend significant time trying
to reverse engineer what the code is doing.   That said some comments about this code.

It's a flat structure with many thousands of random functions just seeming to magically work together somehow.  No hierarchy of control.
Full to the brim with magic numbers in the source code.
Using VxWorks RTOS but nothing in interface is documented.
Virtually no comments.  Authors were Japanese and English comments were attempted but still not usable.
Variable and function names like Phtack2zu .... etc are the rule.  Very few names provide any context.
Vast amounts of abandoned code but not defined out or commented out ,  just not called.

source is C
Size is about 350 files.

I am tempted to reject the job cause I just don't think I can ever make sense of this mess.

Have you plowed through something like this or have you ever declined to work on it.

[snarkysparky]

for example

Code: [Select]


SLONG kysChgAscdt( bind, strp, strlen )
ULONG bind;
SBYTE *strp;
SLONG strlen;
{
UBYTE ascd;
SLONG i;

kysPadSet( strp,strlen,' ' );
strp = strp + (strlen-1);
for( i=0 ; i<strlen ; i++ )
{
if( bind == 0 )
{
if( i==0 )
{
*strp = '0';
i=1;
}
break;
}
ascd = bind % 10;
ascd |= '0';
*strp = ascd;
--strp;
bind = bind / 10;
}
return( i );
}





[SiliconWizard]

No comments, but the identifiers for functions and variables don't even help either!

[jfiresto]

I am tempted to reject the job cause I just don't think I can ever make sense of this mess.

Have you plowed through something like this or have you ever declined to work on it.

That could still work if it is an indefinite position that expects you to clean up the code and make it sensible. A friend took a job like that and spent a few years knocking some terribly ugly Windows and VB6 code into good shape. He subsequent migrated it to .NET and now has a job for however long he wants it. He made himself indispensable, by both now knowing the problem domain and being one of the very few that can address it (as demonstrated by the code he started with.)

[SiliconWizard]

I don't know how interesting the job otherwise is, so the decision is of course entirely up to you.

But having to deal with a large code base you don't feel comfortable with can be a real PITA, and if you know this is going to last for a long time (and not something just temporary), you'll have to really think hard about it.

And yeah, it looks likely that the company opening this position is expecting whoever takes it to save their ass with this hard-to-maintain piece of code, so it might well be your main responsibility, which would make the job even more uncomfortable.

Good luck deciding!

[capt bullshot]

for example

Code: [Select]

...


This routine formats a number into a string, beginning at the end of the string (least significant digit). Does look a bit strange, but not incomprehensible. Does something similar (but not exactly) to a sprintf(strp, "0%6d", bind) ...

One can even guess the meanings of the variables:
bind -> binary digit
ascd -> ascii digit
strp -> string pointer

[NivagSwerdna]

Sounds fun to me.

A consultancy I once worked for was employed once (not me personally) to 're-write' a codebase where the outgoing developer had written it with the variable names all being expletives or such phrases... Apparently some were quite involved... of course it made their purpose somewhat tricky to derive.   

Understanding large code bases of undocumented code is a skill... make sure you charge for it. 

"No hierarchy of control."  ---> in very stack constrained environments this was not unusual.

[SiliconWizard]

for example

Code: [Select]

...


This routine formats a number into a string, beginning at the end of the string (least significant digit). Does look a bit strange, but not incomprehensible. Does something similar (but not exactly) to a sprintf(strp, "0%6d", bind) ...

One can even guess the meanings of the variables:
bind -> binary digit
ascd -> ascii digit
strp -> string pointer

Good job deciphering this piece of code.
Yeah, it might be understandable once you get to know the author's conventions. It's not necessarily that badly written. But one thing that would definitely put me off though, is that judging from the above function, it's not even ANSI C. I would not touch that with a stick. But that's just me, someone has got to do it. Again, good luck!

[ataradov]

This specific piece of code is not all that bad apart from non ANSI C part. But that depends on the target device. May be the only available compiler is that old. Although VxWorks suggests otherwise.

Whether I would work on something like that depends on a single (technical) factor - am I allowed to make code better as I go? If they insist that it must remain this way, you just need to add new things, then hell no. If this is just a legacy code base and they are looking to modernize it, then many times yes. I personally enjoy this type of work.

This implies long term full time employment. If this is a contract job, then I probably would not do that.

[NivagSwerdna]

No takers to re-implement the code example?

It takes an integer value (bind) and a string buffer (pointed to by strp) with length (strlen)

First it fills the string with spaces
Then it converts bind to an ASCII representation which is right justified in the buffer
and returns the number of digits

e.g. 123 becomes "         123"   returns 3

So can you do better?

[Psi]

i would definitively make it clear in the contract that the current code is undocumented and you will spend significant time just understanding how it all works and rewriting sections.
Give them a price and time-frame that takes this into account.

Consider that you may not be the first person they have offered this job too.
It may have been turned down multiple times before for the same reasons you outlined,
so they may be willing to pay top dollar to get someone to do it,

It really depends how your brain works, some programmers enjoy figuring out and fixing crappy code as long as they are being paid good money. While other programmers hate it

[Mechatrommer]

for example

Code: [Select]

...


This routine formats a number into a string, beginning at the end of the string (least significant digit). Does look a bit strange, but not incomprehensible. Does something similar (but not exactly) to a sprintf(strp, "0%6d", bind) ...

One can even guess the meanings of the variables:
bind -> binary digit
ascd -> ascii digit
strp -> string pointer

i agree, its not totally incomprehensible. but you'll need some experience or knowledge under your belt. not really need a fancy experience, just some format converting experience or basic number converter normally available explained in book or somewhere else... such as this line clicks something...

Code: [Select]

strp = strp + (strlen-1);
this tells we move the position of string pointer to the end of the string.. and then...

Code: [Select]

ascd = bind % 10;
ascd |= '0';
*strp = ascd;
--strp;
bind = bind / 10;
take remainder (sort of base 10) of bind into ascd, format it somehow with '0' and put it at the end of the string and then moves the pointer backward toward the beginning of the string and fill it with formatted ascd. divide bind with base 10 again and redo...

trying to understand a piece of library/code base will need a lot of work (but not nearly the work of reinventing it from scratch), patience and slowly walk through it, regardless if its readable or not (currently i'm doing such task on Marlin FW ver 2 for 3D printer to put it in my 5-axis CNC project). one simple trick is copy paste that piece of code into our "workframe", inject it with expected input and see what its output in debug mode etc... another fancier method is map all the files in the library for their dependency and correlation (this function calls to which functions in which files?). but it needs a proper managerial tool SW. i diy one long time ago, but not really a completed one. anyway, you'll know if you have some blood to it or not sooner or later. the reason behind why you are doing it also can drive you to move forward and more... if you think its not worth it, then its a "quitter" ymmv.

[Syntax Error]

It's good your prospective client has let you see the source code before you commit. Many clients/employers recruit stars who are experts in stuff like node, django or jenkins, and then drop them into their legacy CMS cesspool. Six weeks later, they're looking to fill their vacancy, again.

My opinion, the code is dense, suggesting the guys who wrote it were proficient coders when it was written. So don't expect any major fails. Or the need to rewrite it. But don't fall into the trap of over promising. It could take 6 months to unravel this coder's mindset. Not to mention managing the client's expectation of what you can achieve with the little they gave you.

Talk - ask what do they REALLY want you to do - or walk.

Good luck.

+++edit+++ Looking at the style of the code, it seems somewhat 'low level'. Maybe the developer was from an assembly language background?

[ace1903]

I experienced myself the same situation several times.
It depends if there is QA test team or some form of requirement specification for the project.
The worst case was when my employer wanted me to add a new feature on the existing spaghetti code.
I did that and several errors were reported. I was sure that my changes can not be blamed for the errors.
Then again I asked for a specification of how the product should work and perform in different test cases. I was told to look at the code and learn myself.
Found that several software developers worked on it and each of them did their tasks but not knowing specification they introduced several fatal bugs.

Now if I am offered a project with no documentation I simply refuse it.

You can learn what code does but not what it should do. It is easy to get the wrong conclusions just by reading the code.
If there is QA team you can ask which tect case fails, which tests are performed and expected results.

One man show for project and under time pressure is disaster for health both mental and physical. 

[snarkysparky]

Well,  the decision was made for me.  They can't hire at this time and I am out the door.

Was going to decline the work anyway.  I don't like solving puzzles just to be solving them so i would be unhappy untangling that spaghetti code. I prefer to write my own 

Thanks for the input.   They don't have budget for anyone else.   Crazy times....

[SiliconWizard]

They don't have budget for anyone else.   Crazy times....

I have seen that already. Yep, pretty crazy...

[SimonR]

Was going to decline the work anyway.  I don't like solving puzzles just to be solving them so i would be unhappy untangling that spaghetti code. I prefer to write my own 

Probably the right decision, I have done this on a similar sized project, it takes forever. You have to convert the bits you are working on as you go along otherwise you run the risk of breaking it. This can take many times as long to do so you need your employer to agree that this is going to happen otherwise you have no chance.
The converted project is still running after 5 years and is much improved but I still find bits that have never changed.

+++edit+++ Looking at the style of the code, it seems somewhat 'low level'. Maybe the developer was from an assembly language background?

Not necessarily true. The example is original K&R C which had a limitation on symbol name length of 8 characters so everybody used cryptic names back then. Almost all C code looked like that.
You could use longer names but only the first 8 characters were counted so you ran the risk of mixing variable names up and not realize.

Code: [Select]

May be the only available compiler is that old.
Non ANSI C still compiles in C99, or at least the example here still does. The only major difference is the parameter definitions appearing before the { and that still works. Probably the next problem are the differences in what is determined to be undefined behaviour.

Code: [Select]

No takers to re-implement the code example?
If you don't care what the exit value is then you could just replace it with something like

ULONG strlen = 10;
sprintf(strp, "%10ld", bind);

However if you care about the number of digits in the decimal number then you have to re-write the existing function.
Also you might have a dynamically sized buffer which is a problem for the format string "%10ld"

[Siwastaja]

A totally flat huge blurb of C functions, some of which are unused, while not a dream job, is not at all the worst possible. It looks nasty, but there is a limit to its nastiness: it likely isn't totally unsalvageable. This kind of project looks like horror on the first sight, but you start making sense on it piece-by-piece: you can easily delete unused code, then follow function calls.

Naming conventions are like assholes, everybody has one and no one likes the others'. You start getting used to the random-seeming names.

The "benefit" of such design is that if any of those functions are actually doing something difficult to reproduce, they are easy to reuse as-is because there is no huge tree of architectural dependencies like in an over-engineered and badly designed object-oriented cathedral which is not too uncommon at all. Such a case may look like very well-structured on surface, but once you start working on it, you find out you can't do anything on it since it would need complete redesign. Such projects may have tens of thousands of source files which all contain almost exclusively boilerplate.

If you have 350 C files with a bunch of functions each, you have good chances of being able to actually locate the interesting parts (algorithms and datatypes) and follow them.

[NivagSwerdna]

sprintf(strp, "%10ld", bind);

Nice try but think of the overhead of pulling in sprintf... might be too expensive... might pull in too much code and static stuff... if it even exists.

[SimonR]

sprintf(strp, "%10ld", bind);

Nice try but think of the overhead of pulling in sprintf... might be too expensive... might pull in too much code and static stuff... if it even exists.

It does exist, sprintf is in K&R version 1, look it up, I did that's why I suggested it. It uses almost no resources just 0x58 bytes of code and no ram according to my MAP file. I have only 32K of RAM in my system which is very heavily used by my application.

Plus its only a suggestion, a time period compatible one. You would have to move to a newer compiler to get longer names anyway

[ataradov]

It uses almost no resources just 0x58 bytes of code and no ram according to my MAP file. I have only 32K of RAM in my system which is very heavily used by my application.

There is absolutely no way printf() uses 0x58 bytes of code. Typical printf() on Cortex-M0+ is about 20K of code.

So it is possible that compiler is smart and parsed the string and substituted some version of itoa().

Obviously if you already use printf() some place else, it does not really matter, might as well use it here.

[SimonR]

You are of course correct I only did a quick look without thinking about it.
Adding up all the bits of my xprintflarge library seems to add up to about 14K, there's probably some dependencies as well. I used to use the small library until I needed something it didn't support.
I originally avoided it for a very long time for fear it used too much resource, then discovered it wasn't as bad as I thought. I don't care so much about the code size as much as the RAM footprint.

In any case it was just a period correct alternative suggestion assuming you didn't depend on the returned count being correct.

[SiliconWizard]

Yes, compilers are smart enough to do that.
Likewise, if you're only using printf() with no format tags, the compiler will usually call puts() instead, which takes very little code.

[andersm]

Dealbreakers for me would be unrestricted use of globals and pointers. With cryptic, but otherwise correct code, you have the option of encapsulating it, and add documentation as you go along, but if it's also broken you're probably better off starting over from scratch.

If they were explicitly looking for help with modernizing their code base, there are people who specialize in that, but as with all experts, they can be expensive. You just have to make the judgement call if it'll save money in the long run.

[HwAoRrDk]

A consultancy I once worked for was employed once (not me personally) to 're-write' a codebase where the outgoing developer had written it with the variable names all being expletives or such phrases... Apparently some were quite involved... of course it made their purpose somewhat tricky to derive.   

I once worked on a codebase of a piece of software that my then-employer's parent company bought out from a third-party consultancy that ran it as a SaaS for their clients (of which my employer was one). The lead programmer apparently favoured 'shit' as his one-size-fits-all noun that was used liberally in comments all across the code. For example: "copy the shit", "take the shit from the DB result and put it in the list", "delete this shit", "format the shit", etc, etc.