Nkdir

[Maxxzzz]

This is our little spin on the steam locomotive in this case when you misspell mkdir with nkdir. After you make qemu, run the command nkdir. Refer to our skulldocs.txt if you want spoilers otherwise run the command and see what happens.

[ctpham6]

I'll code review this one

[ctpham6]

Alright let's get into it

High Level Checks:

• [ ? ] Is documentation included? This should explain how to build, run, and test the software.
  • Yes, but I don't know if it's good enough. See 1. in the review.
• [ yes ] PR fully resolves the task/issue
  • sl is an annoyance. nkdir is even more so.
• [ yeah ] Does the PR have tests?
  • Technically there's one test for it
• [ yeah ] Does the PR follow project code formatting standards?
  • Pretty weird to gauge. It's good for me because everything is consistent. Indentations are nice. Definitions are all caps. Everything is snake cased. I like it.
• [ yeah ] Does the OS still compile and run?
  • I included pics for proof

Code Checks:

• [ yeah(?) ] Does the code achieve its intended purpose?
  • Is it an annoyance? Yes, and that's good enough for me. Though nkdir can be cancelled by going out the OS, sl can also be SIGSTOPed so that's alright for me
• [ yeah ] Were all edge cases considered?
  • Running the program with and without arguments works all the same
• [ yeah? ] Is the code organized and maintainable?
  • Although the lyrics are also in the c file, the spaghetti code is small enough to go through and understand. For me, it's a pass but the question mark is me thinking that's not really the correct practice for organizing code.
• [ yeah ] Does the code maintain the same level of performance? (no performance regressions)
  • I am going to compare this program to ls. If ls prints out my files line by line, then I should not expect this program to print out the skulls any faster. I think it's just the fault of the OS that makes printing very slow.

Awesome. The skull is cool. Keeping the name 'sl' would still work in my opinion since it would stand for 'skull lamentation' but that's just my mental gymnastics. Anyways, here's the review:

1. I would move your documentation, skulldocs.txt, to the docs folder. Here's another suggestion: some games that try to delve into horror often have a fake description and a real description. For example, if I have a horror mod for Minecraft, my fake description can be "help me! Don't download this mod!" and the real description is something like "credits: colin pham. all rights reserve to the model makers". What I'm getting at is that your documentation right now is good, but it doesn't flat out tell me what the thing actually does. I have to make assumptions based off of your language. Documentations for Python aren't usually like "print(): yeah this thing was made by my friend jimbo so I guess you can call it a built-in function. It makes it so words appear in your command line, but that's what Jimbo told me". Maybe have a section under your current description that puts all the jokes aside and presents your program like you're talking to the President of the United States

user/nkdir.c
2. print_centered() : I love how you guys think about the sizes of everyone's screens when printing out your skull. Unfortunately, it doesn't work. Here's what I see on my 1920x1080 screen:

Here's what it looks like when I window it:

Now, I know not everyone likes UX/UI and aligning stuff but the function doesn't work and I have to put the work on you.
Suggestion:

• I think SKULL_HEIGHT and the defined screen size is fine as is. It's the PADDING that might be the issue. I tinkered the padding a little bit. Here's what the output looks like with vertical_padding = 14 and horizontal_padding = 52.

That's just hardcoding. I have no idea how you can do the math for that. The skull now looks off-centered because of it. I believe editing the constant and the padding is needed.

3. Make the skull laugh more! steam locomotive runs for over 8 seconds! What a pain! The skull is just there for like 1 second. Let me wait! Let me feel the burn! Let me be annoyed! Let the skull laugh for 3-5 more times!
   Suggestion: main(). Do a nice loop. MAYBE switch up the loop amount. Maybe a nkdir can be a second. Maybe a nkdir can be 10 seconds. Who knows!

4. The skull's jaw is dislocated! Help him! I can get if you're going for a "NYEH HEH HEH" laugh but the code shows both skulls' teeth are aligned. This is what I see when the skull laughs on my screen:

Suggestion:

• For a result like this:

I would replace the Skull_open art constant to

HOWEVER, the SKULL_HEIGHT definition cuts out the chin. skull_open and skull_closed DO have different heights so I believe you should account for that. Maybe another parameter to print_centered()? Maybe calculate SKULL_HEIGHT within the function?

• The above suggestion is more of a hard coding solution. Obviously, you would want to account for ALL screen sizes. Messing with my window size still prints the skull in a way that's like the picture above. If my window is super long, then it will skip a line. like this:

The new constant is still aligned for those windows so it theoretically should be a permanent fix, but I would look more into how you're calculating sizes and buffers just in case.

6. Minor: I don't really know if this was the intention but when the programs for ls, mkdir, echo, and grep are replaced with lyrics, the fix is to go out of the OS, go to the .c files of them, ctrl+s the files, and make qemu again. If you're going for a PERMANENT DAMAGE result, then it didn't work (in the scope of the Linux shell and FOGOS but yes in the scope of the FOGOS only). If you're going for a ANNOYANCE result, then yeah you can ignore it. I wouldn't even know how to fix it unless you can somehow access the XV6 files outside of qemu from the inside. My other thought is to remove the privileges of the user to go into the .ls (and other) files to prevent a fix.

7. Minor: Can you put the lyrics in separate .txt files and call it in your C file? Putting them there is fine but it might get a little tiring going back and forth the code and scrolling past everything.
   Suggestions:

• copy and paste the lyrics in their own doc file in the docs/ directory.
• Include and call it in user/nkdir.c

8. Minor: It looks like num and song_choice depends on a single uptime() call. While that's good enough as it is, there can be more randomness if you call uptime() twice. Relying on a single number would make it so the num and song_choice values correlate with each other. Again, pretty minor so you can ignore it is you want.
   Suggestion:

• Use the RTC time we developed in the lab. num = curr_time() % 4 and song_choice = (curr_time% 4) + 5.
  More erratic and chaotic results. Woohoo

[ersutera]

High Level Checks:
✅ PR fully resolves the task/issue
| Prints the skull, as expected
✅ Does the PR have tests?
| Only one, but I don't know how many you would need for something like this, so it is okay
✅ 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?
| Legible, but could be cleaned up a bit more
✅ Does the code maintain the same level of performance? (no performance regressions)

Issues:
Output misaligns across different screen sizes, so adjust padding constants; vertical/horizontal padding might need recalculation.
Animation seems to end too quickly.
Documentation is a little unclear.

Other notes:
This was fun, I liked the concept and it was well-done.
I kinda wish this had a different command, I like how sl stands for steam locomotive and is similar to ls, maybe there is some other misspelling of a common command that can be made into a skeleton abbreviation or pun.

[VivianBelle17]

I will be code reviewing this

[lbrod20]

Verification
This project added a cool ASCII art as a misspelling error. When I typed nkdir instead of mkdir, a silly skeleton popped up. This checks off what they set out to do!

Testing
Everything seems to work as intended. The docs are a bit cryptic, but I'd expect nothing less for this type of utility.

If you want to "break" the ascii art you can by just button mashing immediatly following nkdir. I have an image as proof:

I was curious and ran cat + ls/echo/mkdir/grep and whoopty do it printed lyrics and a syllabus and such. If this were going to be a really, really, really annoying thing, you could just completely delete and replace said files, because right when I exit qemu, the files are the same as they were before. I'm not sure if this is user error or not, but if you completely replaced said files and then had to rebase your project each time, that would be very funny. Well not funny but you know what I mean.

Code Walkthrough

Granted, the files had functions that clearly stated what they were going to do or what they do (i.e., print_centered), but some documentation would be nice.

There is a variable called more_padding in print_centered(), You can add a little more intuitive names like from what i can gather the vertical_padding does the padding above the skull and the more_padding does the padding under the skull. You can use upper_padding and lower_padding to make it a little bit easier to follow.

I think it has been mentioned already, but adding some more time or some more motion would benefit. Like, maybe just copy and paste the three calls to print_centered a couple more times. Or better yet, make a function that just prints it over and over again in a while loop. (think like Sophie, re-useable code cough Steven cough)

Overall
Overall, the code does look very nice, and it's very easy to follow. Also ascii art is spot on

High Level Checks:

• PR fully resolves the task/issue
• Does the PR have tests? (no but not sure it needs it)
• Does the PR follow project code formatting standards?
• Does the OS still compile and run?
• Is documentation included? (yes but more thorough documentation is needed)

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)

[VivianBelle17]

I like the creativity here! Although it wasn't annoying since I purposefully ran these myself, it definitely would have been if the skulls took over my screen for a longer period of time.

Verification

Your implementation met all of the goals you described in issue #22 and then some with the added lyrics to break the functionality even more!

Testing

There aren't any test files or automated tests which I guess makes sense since this is a deliberate user error situation. The cat [command] after running nkdir are matching with the intended lyrics.

Code Walkthrough

I would say that the nkdir.c file is running a little long. I would suggest putting the lyrics and/or the skull ascii art in different files for a little better organization. But other than that, your code is formatted nicely and easy to read and understand what's going on!

Also, while it is meant to be a surprise, your documentation is supposed to be meant to describe what your command can do. So, I would suggest (if you really don't want to spoil the surprise) to put just a brief expected output of what can happen (as much as you can since the lyric distribution is a little random).

Once again, I love your idea! Happy Halloween!!

---

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]

So... this is good. Although I have personally never accidentally typed nkdirin my life (just showing off over here).

• completely agree that the skull doesn't display long enough so I can revel in its glory. This could easily be a tool that I install in CS 326 on gojira to troll students, but it needs more polish. I don't think there's much you can do to get around some of the visual artifacts -- if you resize the terminal, it's going to get messed up, same with typing characters. Just limitations of "FogOS" :-)
• I think 80 is a better hard-coded value for the screen width because most terminals are 80x24 by default, and some programs will even refuse to run if you make it smaller.
• I do not understand why you are adding 5 to the "random" value and then checking for values 5-8... maybe this is a deep troll where even the source code is messing with me.
• Okay, I might be able to look past the previous thing, but the repetitive code that destroys a random utility really needs to be refactored. There should be one function that takes a filename and lyrics as its args and does the work... no need to repeat over and over.
• I don't mind the readme, because it should definitely be sketchy. But I did very much enjoy Colin's review.

4.5/5
You can earn +0.5 by fixing those repetitive functions and convincing me why you need to add 5 to your random number... which is not really random, so you need to also fix that by adding a basic RNG (see the casino PR maybe?).

Excellent work, I enjoyed reviewing this thoroughly.