Re: Code Review please.

Tech Tip: Click here to run a free scan for Windows Errors and optimize PC performance

On 2 Aug, 16:03, "Alf P. Steinbach" <al...@xxxxxxxx> wrote:
* Control Freq:

But, taking things in the order I enountered them.

1. Physical packaging.

This is a very simple little program, yet packaged in umpteen files with one
static library. One simple little file would be enough.

2. Use of proprietary library MFC.

You really don't need MFC for this program. The standard library provides all
that you need, except perhaps parsing. In a real world situation it wouldn't be
totally unreasonable to use some (portable) library for parsing, but MFC would
not be it; in the context of a test to be judged, the parsing is simple enough
that you'd be expected to do it yourself.

In addition to MFC serving to increase complexity & obfuscation and ensuring
non-portability, the use of Microsoft "T" macros does the same.

Just don't use those things.

3. Correctness.

The program didn't compile in Debug mode. This was due to inclusion of
<crtdbg.h> before other headers. You need to include it last, if at all.

Also, more formally (leaving the practical aside for the moment), an include
guard like _STACK_H_ is non-portable. Identifiers starting with underscore
followed by uppercase letter are reserved for the implementation. That also goes
for identifiers with two underscores in succession (although not used here).

4. Conventions.

Using a "C" prefix for a class names is a Microsoft abomination, like Hungarian
notations. Just don't use those warts. Using all uppercase for a class name,
like CSTACK, is ungood: reserve all uppercase for macros.

And generally avoid macros. E.g., you define a long multiline string as a macro.
Don't.

The source code is evidently formatted with tab and indent size 2. That's very
uncommon, resulting in code that looks awful and inconsistent with Windows
convention tab size 4, or *nix convention tab size 8. Possibly those who
evaluated you thought you didn't even master simple consistent indenting.

Your 'main' is some 150 lines.

Try to keep functions well under 10 lines (or at least, a function should fit on
a page when printed, and in one screenfull in your editor).

5. Stack design.

A stack class is unecessarily defined. Just use std::stack<int>.


Hi Alf,
Thanks for your input. I guess it is hard to appraise a solution when
you don't know the requirements. An impossible task really.
But, I read your comments with interest. Although my response to most
of your comments is 'Well, I had to do it that way; it was in the
requirements'. And, ' The coding conventions in use at the time had to
be followed'.
Specifically, your items (1) (2) & (5) were all requirements of the
test. I was applying for a MFC programming position, so I had to use
MFC, and I wasn't allowed to use the std::stack<int> method.

The issues I have with your comments are:
1) Keep functions to well under 10 lines?
I read a thread a while ago in C# stating that functions should be no
more than 25 lines long. These arbitrary limits serve no real purpose.
I try to abide by the 'one function does one thing' method.
2) Indenting. This one seems to be dependent on who is in charge of
coding 'conventions'. I do a lot of third party integration work.
Meaning that I get lots of code from different sources and try to
stitch them together into a product. All parties apply their own
coding conventions. So, there are no hard and fast rules when it comes
to the number of spaces to indent by. This applies to *nixes as well
as Windows code.

Having said all of this, I didn't get the job :(.
I was told that I should have used templating techniques, but really I
should have spent more time looking at the exception safety aspects of
my implementation.
I didn't even consider that as an issue. Now I have a book on that
subject I realise that I still have much to learn about C++
programming.

Thanks anyway.

Regards
.

Relevant Pages

  • RE: [PATCH v3] Make the pr_*() family of macros in kernel.h complete
    ... conventions I'm breaking. ... I sent the below patch to four e-mail lists and it lead to orthogonal ... Other/Some pr_*macros are already defined in kernel.h, ... " fmt, ## args) ...
    (Linux-Kernel)
  • Re: Let Over Lambda chapter 4
    ... *earmuff* convention for special variables, ... the are lots of naming conventions you find in Lisp ... distinguishing between macros and functions? ...
    (comp.lang.lisp)
  • Re: UserForm
    ... I'm new to macros, am still learning the conventions, but its ... Doug Robbins - Word MVP ...
    (microsoft.public.word.vba.general)
  • Re: Let Over Lambda chapter 4
    ... all the conventions you mentioned above. ... I was only suggesting ... that conventions shouldn't supersede proper naming of functions, ... macros, or variables. ...
    (comp.lang.lisp)