using numbers in your code is bad

One of my viewers submitted some code that they wanted me to review. Code reviews are EXTREMELY important in programming. You should always be giving them, and getting them, to improve your skills as a programmer.

In this video, I grade the viewers submission on the codes functionality, ease of understanding, if the code is maintainable, and if someone could expand on it.

EMAIL ME YOUR CODE: lowlevellearning(at)gmail(dot).com

🏫 COURSES 🏫

🔥🔥🔥 SOCIALS 🔥🔥🔥

[JoelJosephReji]

I always though that the code comments should go in the .h file because that is the file which sort of gives the functions which are available to be used.

[catlooks]

I think a huge problem with maintainability here is the double interval input for generating character list. It works with 4 choices, but the moment there are more than 4, 2 intervals won't cut it (for example "135" will require 3 intervals). Then the array intervals are the main point of confusion, so they probably need to be constants. So the solution is to:
- move the constants inside the function, near the array they are refering to
- make a for-loop to paste in characters

For the "1234" parser, you could just take the number as a bunch of characters, and then checking them one by one if they match 1, 2, 3 or 4 and storing results in a bitfield (essentially what you did in the video), and just passing it to the generator, so no switch-cases are needed and instead handled by for-loop mentioned above.

[protonmaster]

Instead of having all the allowable character types in one array and then using magic number indexes into the array for the different types, have 4 arrays of the character types. Then build an array of the types the user wants and make the random selections from there. It makes the function with the large case statement in it redundant, the user can select types in any order, all the magic numbers disappear and its way easier to maintain.

[AllanSavolainen]

Oh an no, you should not duplicate code in the comments, so don't do:

// if the user supplies 0
if (ptr->length >0) break;

The comment doesnt add anything in this case, and might be incorrect if someone changes the code and forgets to update said comment.

If you must add comment here, better would be just:

// validate input parameters

[Eysvar]

My biggest issue looking at this code is how things are named. You touched a little on this with function names, but using Array as the name of the struct being passed around is objectively a bad idea. Container is about the same level of abstract (but much less confusing), however using a name like RandStrContainer would have probably been the best option here because of how descriptive it is. Plus, good names reduce the need for comments (doc comments are always good though).

[obvMellow]

its funny that i dont know almost anything about C and i only code for fun but i still enjoy watching your videos. keep up the good work!!

[Hliosphan]

Yeah the srand bit won't work well if it's in a microcontroller or embedded processor - chances are the whole chip will bootstrap in the same way most times and the millisecond count will be close if not the same, THEN you'd use something different. Only introducing a fair bit of asynchronous-ness to the code, RTOSes, eg blocking on unpredictable IO devices, things like that. The fact as a user on a PC you can choose to launch the program when you want, is where tv_nsec as a random seed works fine.

[KramerEspinoza]

Nice review, mostly agree. Only the suggestion to add comments like : returns void falls to me in the category a = 1 // set a to one
Which is totally superfluous and actually distracting. That the function returns void is obvious… of course if return something more complex, you should describe it…

[dcjlobz]

Love the video! Lots of people only encounter their first code review on the job, so it's super helpful to see how people read and use code in more of these videos :)

I'd also love to see more videos on C specific project creation (including compiler arguments, directives, makefiles, directory structure, etc.) for us low-level noobs.

[ViniciusNegrao_]

IMHO, magic numbers are fine as long as they're immediately understandable. For example: writing (24 * 60 *60) instead of (86400)

[hexadecimalpickle]

Function names should be verbs. `arraySize()` is not a good name. `resizeArray()` is better as it explains the action taken and allows you to use nouns such as `arraySize` for variables.
There's a very few cases where number literals are OK in code, like known value checks - `if (0 == array->size)`. Anything else should be a constant to allow you to give context to the number `#define MAX_WORD_LEN 69` and then `if (arraySize > MAX_WORD_LEN)` gives you context on what you mean with a given value; also it helps refactor as stated in the video.

[AllanSavolainen]

I dont think that function method commenting is useful unless you want to use some tool to generate documentation. In this case the comment doesnt add anything useful:

This function does X
function: arraySizes
param: ptr, pointer to the array object
return: void

vs

void arraySize(Array *ptr)
and 12 lines of simple code

The only thing I would change is the function name, something like promptArraySize would be better.

[hkp]

I would love to see some regular code review videos.
There is so much to learn :)
C, C++, Rust :)

I like these review videos, please make more.

One thing that wasn't mentioned was the way repeat or start over worked, it could be seen briefly when you scrolled through the code. Basically, the same functions were called again from within arrayOutput, which means that the stack would constantly grow, and eventually you would run out of stack memory and crash. Would be better to have a loop in the main function.

[jongeduard]

Great video. I agree on almost everything you said, except 1 single very small part, and that is your criticism on the camelCase style used in the code, because this is really just a personal choice and can vary from project to project. I also say this because C and C++ have no clear standards for this, a different situation from many newer languages that actually do have such standards.
And personally, I like PascalCase a lot as I am also very used to it in due to all the C# coding in my daily life, but I am no stanger to this camelCase style as also used Java, JS and many others.
However I have also become a Linux fan and I fell in love with the Rust language, so I really learned to love snake_case as well, even though it asks from someone to type extra underscores everywhere.

[williamdrum]

Personally I think certain numbers are fine like 0, 1, and powers of 2, or (power of 2) minus 1. How you display them is very important. I always do memory addresses in hex and bitmasks in binary

[timsmith]

@5:47 This is an example of why I believe good programmers need to be lazy: If this had been my first idea, once I realized that I needed to generate all those permutations in my head, I would have devised an easier-to-implement method.

[lennarthedlund]

One should first of all use good descriptive names, which will make the code easy to read. If there are still things that are not obvious, add comments.

Then it is important to actually use the variables for their explicit purpose. In this example, ptr->size is used to store the seed for the random generator?!? This will definitely cause issues when someone is modifying the code.

Another thing that I note is that everything is put in a single large struct. This is not much better than having everything as globals. The struct both contains members called size and length, without specifying what they refer to, which is confusing. I think it would be much better to have it separated, so that the user inputs have their own struct (making it easy to insert the options from another source such as the command line), the string of candidate characters have their own struct, etc.

[mikebrightman]

I always recoil a bit when I hear "more comments", as my experiences lines up well with the idea that a comment is a lie waiting to happen. Renaming those functions/variables in a reasonable way and breaking things up into smaller functions means less to parse, and (while I despise the phrase self-documenting code) it becomes readable without. For maintain and expand, every function having a 5 line comment saying "this function called get array from pointer takes a pointer to an array and returns an array contain the content of the array at the end of the pointer" really grinds down reading (in this case, the comments take up lines that could be used to display more code to give the reader context etc).

Overall I mostly agree with things, magic numbers should live in a global const with a clear name or an enum type (perfect for switch statement use and quick reading), excessively compressed naming and the others things you pointed out are fair.

I agree with the other comment that the header is a great place for the documentation comments. Perfect place to live in the interface file for the code

[Argletrough]

This would've been easier to write and use without ncurses: Enable the character sets, specify length etc with command line options, and print just the random string (following the unix philosophy).
Then the user can do stuff you didn't think of with your program, like append the random string to a file: ./prog >> somefile.txt

You could probably avoid allocating memory for the array entirely by just printing the characters one at a time, as they're generated, in a loop (putc).