Skip to content

Conversation

@amengesha
Copy link

MATRIX PROJECT :

Abi & Liwam

The matrix command simulates the iconic "falling code" from The Matrix movie, displaying random numbers and symbols streaming down the screen.

Basic functionality:
The command uses a random number generator to display numbers and symbols in random positions on the screen. The characters fall from the top of the screen, simulating the continuous scrolling effect of digital code.

Flags:

  • /matrix: Displays matrix falling code in green
  • /matrix -blue: Displays matrix falling code in blue
  • /matrix -pink: Displays matrix falling code in pink
  • /matrix -rainbow: Displays matrix falling code in rainbow colors (red, yellow, green, blue, magenta, cyan).

@snarayan57982
Copy link

snarayan57982 commented Nov 4, 2024

Adding a review!

Overall, this code works and seems pretty solid! It looks a lot like the falling code in The Matrix :)

Below are the notes I have:

RNG: Implemented correctly
Clear Screen function: implemented correctly, but maybe just call it only once before starting the actual matrix effect
Colored Flag logic: Clear, and effectively sets different colors per column + single color defaults to green based on the arguments
Message: Maybe the printf statement where you mentioned which color the code is set to might not be helpful when you use rainbow (which uses multiple colors), but this is more of a nitpick.

Iterations: Currently the code refreshes in place. Maybe adding a slight delay (like usleep()?) within the main loop to control speed could be an improvement idea.
**Rows and Columns: ** If you have control over terminal size, maybe allowing for more rows and columns, or size changes, could be cool!
Dynamic sections: The code reuses column_pos for each iteration as far as I'm aware. For a more randomized and maybe chaotic effect, varying the initial column positions per iteration could work, though I can see how this might be too chaotic if not done correctly.

The only minor adjustments I've suggested are speed control, size control and visual enhancement. Other than that everything looks super cool!

High Level Checks:

  • PR fully resolves the task/issue
  • Does the PR have tests?
  • Does the PR follow project code formatting standards?
  • Does the OS still compile and run?
  • Is documentation included?

Code Checks:

  • Does the code achieve its intended purpose?
  • Were all edge cases considered?
  • Is the code organized and maintainable?
  • Does the code maintain the same level of performance? (no performance regressions)

@djbirdsall
Copy link

djbirdsall commented Nov 9, 2024

Code Review:
Overall looks pretty good!
Running the program shows something similar to the screen in the matrix for sure.
Inspecting the code, I found a few things that stood out to me:

  1. It looks like your matrix_effect function prints the output in a nested for loop which uses columns and rows inside of a while loop. This would make sense if you were printing out copies of a literal matrix structure, but because you are just trying to print out a set number of randomized lines, one simple for loop inside of a while loop would be sufficient! The for loop managing the line length and the while loop managing the amount of lines printed.
  2. The print conditions are a little confusing, it seems like you could get rid of the first if statement check and it would have the same functionality!
  3. Also, there is not a standardized format used for the same printf statements here:
    char ch = charset[rand_range(charset_len)];
    printf("%s%c\033[0m", current_color, ch);
    and here:
    printf("%s%c\033[0m", current_color, charset[rand_range(charset_len)]);
    They do the same thing but are just written differently in different parts of the code!
  4. Lastly, your /matrix -blue option actually prints out a purple color! You have the value for blue correct for your rainbow option: "\033[34m", but for the blue option you use: "\033[94m".

Otherwise, it is nice that everything is user level and you only had to modify one existing file in FogOS! Cool idea!!

High Level Checks:

  • PR fully resolves the task/issue
  • Does the PR have tests?
  • Does the PR follow project code formatting standards?
  • Does the OS still compile and run?
  • Is documentation included?

Code Checks:

  • Does the code achieve its intended purpose?
  • Were all edge cases considered?
  • Is the code organized and maintainable?
  • Does the code maintain the same level of performance? (no performance regressions)

@malensek
Copy link
Contributor

Basic functionality is here. We are missing the art show option as well as the density configuration mentioned in the original issue.

Everything works well, I'd add an error message / usage if the user passes in an invalid flag (I tried running with -h to start)

Implementation wise, I'm not sure why this needs to track column / row positions or store any state at all. It appears to print random characters and then relies on the terminal's scrolling for the animation (which is fine... but then couldn't you just replace the logic with a single loop?

Stylistically this is fine but it does have a lot of GPT-style comments for everything, some of them could probably be cut down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants