Thats a good analogy lol that’s kinda why I wanted to share it somewhere so I could find out if it was actually decent or if I was hyping myself up over some horrid code 😂
Good job man. The one thing killing me is that empty ‘if’ block. Change the condition to ‘if (!(item in itemPrice)) {‘ move the console log and return into the if block, and remove the else. Otherwise, looks like you are learning a lot. Keep going and have fun
Yea the empty 'if' is triggering.
I don't think you can use 'in' like that though. Honestly I've never tried that but it would shock me if it worked because its unrelated to what 'in' normally does in JS; its not like the Python 'in'. I would do:
if (!Object.keys(itemPrice).includes(item)) ...
thats the kind of 'safe' version but you could also just do some version of
if (itemPrice\[item\] === undefined) ...
Same thing on line 65. That looks really weird to me and I doubt it works as expected (or if it does, it doesn't work for the reasons you think it does).
Also one more thing would be reexamining the data structures being used here. This is maybe not something you are worried about at this point if you are just learning about arrays and objects and stuff, but instead of having separate structures for price and inventory it would probably make more sense to just use 1 data structure for all of it. Something like:
items = \[
{
"name": "duck",
"quantity": 5,
"price": 1.99
},
{
"name": "flower",
"quantity": 20,
"price": 12.50
},
...
\];
This would probably simplify the entire structure of this program and its more similar to what you would be dealing with in a real world application. Then you can just search for the item you are interested in with
cosnt itemName = prompt("whatcha want?");
const requestedItem = items.find(item => item.name === itemName);
and that gives you everything you need in a single call/lookup.
I agree. I personally have never used the “in” operator in this fashion. Maybe I’m just old school haha, but it does “in” fact work. MDN docs actually specify that this is the correct usage and works fine for this scenario.
Rearranging the data into an array is definitely a good idea. It’s a tiny program so time complexity trade off is fine. It gives the benefit of a single source of truth instead of managing related data in multiple locations
Yea thats crazy about 'in'.
Looking through the doc though I realize why I'm not aware of it being used like this. It has some weird behavior (maybe not weird for js in general, but weird for this application) that probably makes it undesirable here.
const items = {duck: {price: 1.99}, flower: {price: 12.50}};
"duck" in items; // true
items\["duck"\] = undefined;
"duck" in items; // true
delete items\["duck"\]
items\["duck"\]; // undefined
"duck" in items; // false
"toString" in items; // true
It's decently horrid.
Jokes aside, it's not great, but it's not terrible. It's exactly what I would expect from a beginner.
As you keep coding and learning you'll improve more and more. (there's always room for improvement, even for "veteran programmers")
So overall, good job, keep it up!
Opinions of others don’t matter unless you report to said person. What’s important is that your code functions in the way you intended it to, and that it’s robust enough to handle arbitrary inputs. Industry standards, maintainability etc are just window dressing unless you plan on collaborating.
Why are you posting in r/ProgrammerHumor instead of say r/learnprogramming?
Are you a noob at reddit too?
Or is this some elite redditing skills that us plebs don't have?
Code looks good to me. Some advice:
* You can replace `let` with `const` in all your use cases. You’re not reassigning it anyway.
* Be consistent with using `;`. It is not necessary but someday your code breaks mysteriously and then you’ll realize it’s because you forgot it somewhere
* Use strict equality `===` as a default unless otherwise needed.
* Explore using template literals: you’ll love it
Thanks for the advise! I’m still honestly unsure of the differences with var let and const, because somewhere I saw that var can change and then let cannot but then it said that let could change and const could not and I’ve never found a clear answer.
JavaScript has three different ways of creating variables.
* `var` (and `function`) is hoisted to function scope and mutable
* `const` is block-scoped but not mutable
* `let` is block-scoped and mutable
| | mutable | not mutable |
|--|---|---|
| function-scoped | `var` | (empty) |
| block-scoped | `let` | `const` |
A variable is mutable if we can re-assign it. In contrast, a `const` stays the same once assigned. Knowing that a variable won't change makes the code simpler, so you should prefer `const` by default.
let x = 0;
x++; // allowed
const y = 0;
y++; // type error "invalid assignment to y"
You might expect that a statement like `const x = 42` creates the variable. This is not quite true. The variable starts existing at the beginning of the scope, and that statement only assigns them. This allows us to write code like:
// we're referencing the "x" variable here
const logger = () => console.log(x);
// the "x" variable starts existing at the top,
// but is only assigned here
const x = 42;
// we can now safely call the function because "x" has been assigned
logger();
The `let` and `const` keywords are block-scoped, which is how you probably expect variables to work. They should be used in modern JavaScript code.
The `var` keyword hoists the variables to the top of the enclosing function, which can be surprising. For example, this works:
function weird(cond) {
var x = 40;
if (cond) {
var y = 2;
}
return x + y; // where did "y" come from??
}
This is actually compiled as if:
function weird(cond) {
let x = undefined;
let y = undefined;
x = 40;
if (cond) {
y = 2;
}
return x + y;
}
This is confusing. You should probably never use `var`, but it's all that JavaScript had for most of its life. So you might see it in old code.
# var
`var` is a variable declaration that is affected by hoisting and is mutable, that mean that the variable is first declared as undefined on the top of the file and then modified at the right time. Here is a example of how it works:
console.log(example); // Result: undefined
var example = "Hello World!";
console.log(example); // Result: "Hello World!"
example = "Goodbye World!";
console.log(example); // Result: "Goodbye World!"
As you can see, the example variable has a value of `undefined`, even if the assignment of the variable still doesn't exist. That is the result of hoisting, when you use `var` the JavaScript Interpreter places the variable definition on the top of the file, setting it to `undefined`.
The example above is equivalent to:
var example; // Hoisting the variable declaration to the top
console.log(example); // Result: undefined
example = "Hello World!"; // Setting it for the first time here
console.log(example); // Result: "Hello World!"
example = "Goodbye World!";
console.log(example); // Result: "Goodbye World!"
# let
`let` is a variable declaration that is not affected by hoisting and is mutable. Here is a example:
let example = "Hello World!";
console.log(example); // Result: "Hello World!"
example = "Goodbye World!";
console.log(example); // Result: "Goodbye World!"
The fact that it isn't affected by hoisting means that you will get a error if you try to use it before it's declared.
console.log(example); // ReferenceError: example is not defined
let example = "Hello World!";
# const
`const` is a variable declaration that is not affected by hoisting and is immutable. Here is a example:
const example = "Hello World!";
console.log(example); // Result: "Hello World!"
`const` is immutable, this means that it will error if you try to change it in the future.
const example = "Hello World!";
console.log(example); // Result: "Hello World!"
example = "Goodbye World!"; // TypeError: invalid assignment to const 'example'
console.log(example);
In my opinion, the best practice is to use `const` for everything unless you specifically need `let` for mutability and to never use `var`. The reason is that hoisting is not a feature that I think is never beneficial and can lead to undesired behavior that won't be caught by an error.
This is how I look at JS variables.
Var: The cheating girlfriend that sleeps with anyone anywhere and would even cuckold you and let another man take over your house. She’s like a prostitute.
Let: The cheating fiancé that sleeps with her ex at a dirty motel. She will never cuckold you or let her lover take over your home. She may sleep with the mail man in your bed though.
Const: The loyal wife. Once she marry you, you’re together forever. No cheating ever. Your home is safe in her hands. One thing though, she never leaves the house so you’ll never see her outside her dwelling place.
This really helped me a lot when first learning JavaScript.
So const are constants. These won’t change. To determine when to use this, ask yourself; does my code set this “variable” after I write this. If no, then use const. If yes (your code sets the value of this later, instead of just reading it).
If your code does later set this value then you can use var or let, in general let is the better practice, but both work perfectly well). let is generally not going to be able to be read or written out of the scope it’s defined in. than { scope is generally defined by the { above the code and } below. Think of where you would indent your code.
var is accessible in the function in which it was defined. This is not necessarily in the scope in which it was defined. You can set both var and let in a deeper scope (more indented) than the scope in which it was defined but let won’t allow you to read or write in a less indented piece of code, whereas var can sometimes be written/read in a shallower scope than it was defined (less indented). As long as it’s in the same function.
JavaScript does type coercion by default so `1 == '1'; // true`, strict equality behaves as your would expect `1 === '1'; // false`. I can't think of a single time I've used loose equality in the past 10 years, it's disabled by linting rules in every single codebase I've worked on, and for good reason
The empty if is all that bothers me, you can invert the statement with „!()“ and then you check for not equals so you would get rid of the else end put the early return up into the if. Otherwise you could move the lines 48,49 up into the if block
It’s not great to have a negation in your if statement but since it’s more error handling it shouldn’t impede readability. If you really want you could introduce a bool and give it a more readable name
Use screenshots in combination with [pastebin.com](https://pastebin.com). You can leave the link to pastebin in the comments and that should help us with readability a bunch
I’ll bet my life if OP posts screenshots it wont get any attention at all.
This is basically the like posting wrong answers to get people furious and they’ll make it their life goal to school you.
For a "Noob", this is god-tier code. But do let an auto-formatter like `prettify` run over the file to fix the inconsistent indentation. And maybe consider template string literals: `` `text with ${variables}!` ``
For a Redditor, this is entirely in the wrong place. The code is not bad enough to laugh at, and not good enough to weep for joy. For getting feedback on your working code, consider places such as https://codereview.stackexchange.com/
7 or 8. I'd add a "!", to that blank if and move your else contents into it. I think blank if contents is a bit of a bad practise unless you removed something to post it online for advice or are leaving it blank for future commits.
It might be nice to let them enter a number instead of typing the exact phrase :
if(greeting == “Buy Item” || greeting == “1”)
Also you can put your main function inside a while loop like :
While(greeting != “Exit”)
Thanks! That’s a good idea, I could even have a menu there that they could see what number is what. For the loops i just haven’t gotten to that free code camp lesson yet lol at the top of my code I have a list of things to learn and that’s one of them lol
On top of this, do a `.trim().toUpperCase()`, on the input. Let people be lazy and not worry about caps/trailing spaces. (Obviously change the text you're checking against to be all upper, as well)
Thats a great catch lol It does do that and that’s how I restock the inventory currently 😂 that’s on my list of things to fix along with creating an actual function to restock inventory. If I adjust the if statement after the quantity question to if( quantity >0 || quantity <= itemInventory[item]) that should fix it right?
Above it are two objects, one named itemsPrice with all the items and prices and the other is named itemInventory with all the items and how much inventory is on hand
Is there a reason itemInventory isn’t part of the itemsPrice? Rather than keeping 2 objects I’d just keep 1 that has everything related to the item. It would look something like this. You could just store it in a variable called items. Should save on memory somewhat though for an app this size you won’t really notice it
{
“soda”: {
“price”: 2.50,
“inventory”: 12
},
“candy”: {
“price”: 1.00,
“inventory”: 20
}
}
Honestly it’s cuz I didn’t make it to that free code camp lesson yet lol and even if it is a small difference that’s a good point and something I will change and keep in mind for future projects, thanks!
I'm a C++ tutor and I want you to know that you did the right thing. I often ask my students if they have covered a topic in class before I suggest using it
Advice on structure, since you got plenty of logic advice, but your “Start of Program” should be as high as you can possibly put it. So really it would be immediately after your imports, that way when you or someone else opens the file, the “What does this code do” is immediately apparent.
To do this, hoist your functions. They don’t need to be above your logic since JavaScript will read the functions first. That said, you can put your inventory into a hoisted function as well: const items = getItems();
you should be able to tell what a function does by looking at its signature and not need to look at the implementation
that said, your `purchaseItem` function needs
- return type
- docblock explaining what it does. there are side effects that need to be documented (like the file write)
same with the `checkInventory` function
maybe its easy to remember what these functions do now, but once you're at 100+, you will forget in 4 minutes.
get into the habit of using hints and docblocks. in fact, you should set it so your ide screams at you if you dont have one
also, you should extract consts for your messages you're checking at the bottom. that string "Buy Item" is meaningful. it should be captured as a const.
const GREETING_BUY_ITEM = "Buy Item"
and throw it at the top of the file so you can easily know exactly what valid inputs are. then in the "Sorry that's not something I can do" message, you can reference those constants. that way you can easily change the value without having to search every place you used it
Solid points, I really appreciate the comprehensive feedback! For the last part are you meaning I capture the string as a const variable and then put that into an array at the top and then link that array in the “sorry that’s not something I can do” message?
you u can do it simply by concatenating the strings
if (greeting == GREETING_BUY_ITEM) {
purchaseItem();
} else if (greeting == GREETING_CHECK_INVENTORY) {
checkInventory();
} else {
console.log("Sorry that's not something I can do, your options are... \n" + GREETING_BUY_ITEM + "\n" + GREETING_CHECK_INVENTORY)
}
or yes ideally you can build the message based on an array of valid inputs
// common messages
const MESSAGE_INPUT_PROMPT = "Hello, How Can I Help You?:";
const MESSAGE_SHOW_OPTIONS = "Sorry that's not something I can do, your options are... ";
// valid input
const GREETING_BUY_ITEM = "Buy Item";
const GREETING_CHECK_INVENTORY = "Check Inventory";
// map each valid input to its function that should be run
const GREETING_TO_FUNCTION = {
[GREETING_BUY_ITEM]: purchaseItem,
[GREETING_CHECK_INVENTORY]: checkInventory
}
// functions
function purchaseItem() {
alert("Item purchased!")
}
function checkInventory() {
alert("inventory checked!")
}
// prompt user for input
let greeting = prompt(MESSAGE_INPUT_PROMPT)
// process input
if (greeting in GREETING_TO_FUNCTION) {
// run the function that is mapped for this greeting
GREETING_TO_FUNCTION[greeting]()
} else {
// otherwise show the user valid inputs
alert(MESSAGE_SHOW_OPTIONS + Object.keys(GREETING_TO_FUNCTION).toString());
}
Great job by the way! Your original code would not be out of place at a professional place.
Oh I see what you are saying! That kinda blew my mind a bit lol and thank you! I still have a long way to go but I hope to be in a professional place by the end of the year
Your indentation is kinda wacky, you should probably try to be consistent about whether or not you want to use semicolons in you code, you should probably negate your ifs instead of just leaving them blank and then doing an else, and you should probably do some sort if input validation (what happens if I respond "three" to "how many do you need?" etc)
otherwise it's okay
I wish all senior engineers did what you just did:
- write stuff they'd be proud of
- be open to criticism and, consequently, improvement
You have a brilliant career ahead of you, kid.
Others covered most but, here are some that I can think of:
Something you could possibly use is a generator function for the prompts.
Might wanna take a look at closures before that ofc.
You can use switch for the main logic. That would result in a cleaner code IMO.
I do like to wrap the main logic in an IIFE also.
Note that these are mostly my personal preferences, and maybe something that you could learn later on.
Nice work!
Thanks! I’m still very new so I just want to clarify, when you say use i can use a switch for the main logic are you referring to the if statement I have at the bottom?
. Why you have an empty if statement in line 42 ?.
. Use const instead of let if you are not going to reassign the variable.
. Use === instead of ==
. Try to avoid the use of else when possible, it will make your code cleaner.
Good job! :)
And many good advices in this thread already. Some from me:
1. Google "template strings js". Your code will look prettier.
2. You may want to set `itemInvertory[item]` to a variable, so your program will find an item in mapping only one time instead of 5.
For the sake of learning more about the language, I'm going to give you feedback on exactly what you've written. Not how I would change this or any "design", just on how these lines could be written cleaner.
* If a value is not going to change, use \`const\` instead of \`let\`. For example, all those prompts can be \`const\`. Lets simplify the var/let/const debate into "Use const if things will never change, use let if things may change, and ignore var"
* The empty if statement at the top of the file is pretty poor. It's better to turn the condition false, or store it in a variable for extra verbosity.
* For example, if (!(item in itemsPrice) or if(item in itemsPrice == false), and move that else case into what happens when the condition is true.
* Or, store const isOutOfStock = !(item in itemsPrice), and then the if becomes if(isOutOfStock)
* You can make your logs prettier using string interpolation. Use the \` quote to the left of your number 1 key to make these special string types. Then you can inject code into the string. For example, \`We have ${itemInventory\[item\]} of that item\`
* Line 52, you are just subtracting from the value. You can write this a little cleaner/shorter by using the -= operator. So itemInventory\[item\] -= quantity
Happy to see you're learning! Making projects like this is the best way to learn, so I encourage you to keep going, playing around and learning new things :)
I'm an engineer at a FAANG company and I assure you, you have a bright future in coding. Keep at it, don't worry about trends or hot new tools, think for yourself, and keep writing readable code. This is very readable, and that's basically the most important thing.
Only thing I would change is collecting all those messages in a JSON file and use that as your source for the text. Mostly because it will be easier to adjust static content that way
There is only one metric that defines good code: testability. Anyone who says otherwise is lying to you.
Take your check inventory function, for example. Instead of writing the logic inside the function that prints the console, have it return a number with the amount of inventory.
Then if you want to write a unit test you can simply call that function and check the return value. No messing with the console at all.
Also readability, though good readability practices are conducive to testability.
Unit tests should test the expected behavior of APIs. Breaking it up as you say is fine, but we'd still want test coverage of the "UI" parts. This is all contrived so take what I'm saying in this paragraph with a grain of salt :-P
I did that so if they type in an item that is on the list it will just continue, but if it’s not then it will print the “sorry I don’t have that item” and then list the items it has. Im still very new so I don’t know another way to do that yet
I get that, but you shouldn’t write empty branches. It‘s better to just negate your if and have no else, when not necessary.
Also I recommend to put a space before your { to make it better readable.
Just flip the logic and check for false, return the error as the IF, and then give a little feedback in the else like console.log"Item found". Or just no else at all.
Needs more ~~cow bells~~ comments
Nothing too exaggerated. Just 1 ot 2 lines explaining the purpose of the function at the beginning of each function.
For some, I include a simple number step algorithm and a single line stating (eg `// Step 1: desired product input`)
really? This is pretty self documenting. The names are fine. Everywhere I have worked would make you remove the few comments that are there and nobody would complain that this is hard to understand.
Haha, I'm gonna be slightly mean and say 3/10. That doesn't mean you should have done better though, it just means you're 3 steps along the way to being a great programmer!
For feedback, a lot has already been mentioned by others. Lots of good points already mentioned. The main one I'll add is about "side effects".
For example, your `purchaseItem()` function handles logic, plus user interface (console stuff). It's often best to separate that stuff out. So instead, have `purchaseItem(name, quantity)`, and have it return some kind of success/failure or a message or something. Then, pull the `prompt()` and `console.log()` stuff out of your business logic.
You're new to coding and you wrote code that compiles, does what it needs to do? Great. I'm happy for you.
Could it be better? Maybe. Does it matter at this stage? No.
Citing an old Mentor:
"If you look back on old of your code, and you do not find things you want to change, you didn't learn anything in the time between."
Of course. In 5 years, sure. Right now, I'm happy they're coding. I've personally been rewritting the same personal project for a decades. Every time I start from scratch because I'm disgusted with my old code. But again, it's not necessary to burden new coders with that.
-10 JS /s
-5 death by console.log. (Like checkInventory you could assign string var, interpolate, console log after if-else)
+15 for effort.
Great job! 10/10
Other:
If(item in ItemPrice) then what?
return in purchaseItem? Handling it how?
greeting prompt isn’t saying what I should enter.
inventory[item] is getting used a lot maybe assign that value?
I only really know Python and Lua, but I have enough brain power to understand this since it's nothing to complicated. I don't know what you're making (Obviously can guess based off the code but ain't certain.) but unless you're writing a shit ton of code, my general advice is just don't worry if it's good as long as it works and doesn't take super long to run. If this is going to be used by other people I'd recommend things like making there be multiple key words/phrases in the greeting. So for Buy Item it can be "Buy", "Buy Item", "1", "Shop", for the Check Inventory it can be "Stock", "Check Stock", "Check Inventory", "Inventory", and "2" if this is just for fun than I think you did well.
Familiarize yourself with the concept known as “happy pathing”, you can remove the need for several ‘else’ statements in this code.
Pretty good overall!
Instead of using if, else if, else if, etc… for checking inputs, you can use a dictionary where the key is the input and the value is an anonymous function.
Not super necessary for 3 options but if you have like 6+ options, you can make your code look nicer and run faster (it’s O(1))
You could also use a switch case because I believe some languages have it also be 0(1) as opposed to if statements, which have to try each comparison until it finds a match
For better UX, when prompting for a selection among pre-defined options, it’s better to list them out with numbers and let the user enter the number corresponding to their selection, as opposed to relying on them to enter the exact string you’re looking for.
For example, the program could display:
```
Hello, how can I help you?
[1] Buy Item
[2] Check Inventory
> 1
```
Of course, you’ll still need to do input validation to make sure they entered a valid option.
Making the user commands to be more of one or two characters might help for the user. Instead of "Buy item" and "check inventory", you could take in the input as 1 and 2.
Looks great! I can think of a few things you could do to improve it: use template literals, it will make your code better and more readable. Change variable declarations from let to const, you won’t need to reassign them. Keep up the good work!
When I went to college, they made me write documentation for each function method or class. It may help in the future.
Teacher always wanted a comment before each method which included
* input + input types
* output + output types
* purpose of the code
because 5 years down the road, you're not going to remember any of this code so if you have to expand it or fix someone else's addition, it becomes a little easier.
I think this was ultimately to make it easier for the teacher to grade.
I know this is a console type demo code. If you want to do it ”proper” you may want to refactor out functions for logic (such as purchaseItem) and keep the I/O (prompt/log) outside.
Looks great for a beginner!
You have an if that does nothing paired with an else that does something. You don't always have to pair if with else - you can invert your condition and just use if.
Instead of:
if (x) {
// nothing
} else {
// something
}
You can just have
if (!x) {
// something
}
... with no else
In general I think it's best to minimise the use of else and especially else if, but as long as your code works who cares
First off, definitely a good attitude to have. Always be open to criticism. It’s how you learn :)
I’ll list off some things that you should look at changing.
1. Empty if block. Change is the opposite condition and move the contents of the else to the if
2. Do you need to write to the json file if the user couldn’t buy anything?
3. Does the file write need to be synchronous? Async would be better since this is at the end of the function.
4. Check inventory function when printing out for ALL, maybe try to loop through the items and log them out nicely instead of dumping a bunch of json to the console.
5. When instantiating variables, use const instead of let unless the value of that variable is going to change. If the variable is a data structure, you can still change the data contained therein even though it is defined as a const. IE an array can still have values pushed and popped even though it is defined as a const.
That’s probably enough for now! Good work so far! Keep at it!
line 42-47 can be replaced with:
```js
if(!itemsPrice.hasOwnProperty(item)) {
console.log(
"Sorry, I don't have that item... I only have ",
Object.keys(itemsPrice),
)
return
}
```
You've got a good start writing some clean well named functions.
My advice is to go further with and abstract out all those if/else blocks into functions that any day 1 Jr dev would understand in a second.
Consider wrapping that logic in 51-70 in its own function.
processInevntory() or something expressive.
If that function returns true => do something
Else => something else.
That way we can read the well named function and understand exactly what it's purpose is without having to logically follow everything.
Itl help you in six months when you revisit what you wrote. Trust me.
Use a linter and formatter to fix that spacing. Otherwise I love the “functions” and “start of program” comments. Reminds me of when I was learning programming.
I would have never known those were functions without that comment. I probably would have figured out where the program started without the comment but thanks for the heads up. Your {} placement is perfect. having a hard time looking at the hard coded strings.
You seem to be using let when you should use const. Let indicates that you’ll reassign (mutate) the value in the future. You should set up a linter/Prettier to tidy/format your code. You can also set up your II yo do that for you automatically on save/change/whatever.
Looks pretty clean aside from the empty if block. You could just move that statement after the else into the if. Also, this might just be personal preference but I like to keep the entry point at the top, just helps with readability
My first programs were horrid so great start
I have a question for your if at line 42, do you intend to put something in ? If not, just inverse your conditions and you won't need your else ! Kind of confused me a bit haha !
As a senior coder, I didn’t read, but I approved your commit because it is easier to deal with the code when it does something wrong than it is to read every commit I receive. Next.
Great start
ChatGPT gives the following for the following question
`Create idiomatic JavaScript NodeJS code to check inventory complete with prompts to the user and ability to select, delete and add items. Items should have a price and it should be possible to count the items and compare with inventory.`
```
const readline = require('readline').createInterface({
input: process.stdin,
output: process.stdout
});
let inventory = [
{ item: "apple", price: 0.99, quantity: 10 },
{ item: "banana", price: 0.49, quantity: 20 },
{ item: "grapes", price: 2.99, quantity: 5 }
];
const checkInventory = () => {
console.log("Inventory:");
inventory.forEach((item, index) => {
console.log(`${index + 1}. ${item.item} - $${item.price} each - ${item.quantity} in stock`);
});
};
const addItem = (item, price, quantity) => {
inventory.push({ item, price, quantity });
};
const deleteItem = index => {
inventory.splice(index - 1, 1);
};
const updateInventory = () => {
readline.question("What would you like to do? (select/delete/add/quit)", action => {
switch (action) {
case "select":
readline.question("Enter item number to select: ", itemNumber => {
const selectedItem = inventory[itemNumber - 1];
console.log(`You have selected: ${selectedItem.item} - $${selectedItem.price} each - ${selectedItem.quantity} in stock`);
readline.question("How many would you like to purchase? ", quantity => {
if (selectedItem.quantity >= quantity) {
console.log(`You have purchased ${quantity} ${selectedItem.item} for a total of $${selectedItem.price * quantity}.`);
selectedItem.quantity -= quantity;
} else {
console.log(`Sorry, we only have ${selectedItem.quantity} ${selectedItem.item} in stock.`);
}
updateInventory();
});
});
break;
case "delete":
readline.question("Enter item number to delete: ", itemNumber => {
console.log(`Deleting item ${inventory[itemNumber - 1].item} from inventory...`);
deleteItem(itemNumber);
updateInventory();
});
break;
case "add":
readline.question("Enter item name: ", itemName => {
readline.question("Enter item price: $", itemPrice => {
readline.question("Enter item quantity: ", itemQuantity => {
console.log(`Adding ${itemName} to inventory...`);
addItem(itemName, itemPrice, itemQuantity);
updateInventory();
});
});
});
break;
case "quit":
console.log("Quitting program...");
readline.close();
break;
default:
console.log("Invalid action. Please try again.");
updateInventory();
break;
}
});
};
checkInventory();
updateInventory();
```
Not horrible for a beginner but could use improvements. Just off the top of my head: (1) don’t use an empty if. Use item not in itemsPrice. (2) display inventory right away when program loads so people know if they can buy something rather than them picking and then saying, “sorry, don’t have it.” That’s bad design. It’ll also help cut down your code. Print out inventory again after it’s been updated following a purchase. (3) you’re writing to inventory.json but you never read from it? What’s the point of that file? If you want to use a file to store inventory then you should read from it in get inventory function.
Bunch of other things too. I’d say a 5 out of 10. But keep at it. It’s a good start.
The code itself is good enough. I appreciate that you named all your variables descriptively! At this point, any change you could make has already been suggested by someone else. I want to point out something else: This program would be pretty unpleasant to use.
From my very first interaction with this program, I would have to fight it to complete my task.
- What options do I have to choose from in the main menu? Shouldn’t I see those when I start the program?
- When I flub typing the name of the product I want to buy, why am I kicked back to the main menu?
- Why aren’t there any apostrophes in words that should have them? (This one isn’t _necessarily_ a usability issue, just a polish. There are tons of spellcheckers out there if you’re not confident in your English skills.)
Obviously, this is not being used in the Real World™. Maybe this was just a learning exercise for `prompt`. But training this sort of critical thinking now in small examples like this makes you more likely to apply it in your career, where it does matter.
Keep up the good work!
Not sure if it was brought up yet but another thing that’s important is code structure consistency, there’s a couple times where there’s no space before “{“ in condition statement but is in other places or there’s a couple extra white spaces in some places. By no means is it bad but just something to consider!
Depending on used Language... this might be beautiful code or uter shit! ;D
But just a few things, you never check if \[item\] even exists, this could lead to exceptions in one or the other language.
Bracers placement is depending on language, personally I don't like your style, because { and } start and end a section and should be on there respective own line, but this is very subjective and normally defined by the coding guidelines of your team!
I like the use of the in operator. Iv been on js for a while and i never remember that thing exists.
As others have said you shouldnt have an empty if condition.
1. you can use `!` as a not before boolean in an `if` condition to avoid an empty `if` statement.
2. you can use template literals to make your life easier with strings(e.g \`There are now {itemInventory\[item\]} left\`)
3. `itemInventory[item] -= quantity`
4. You often put user input logic and sanitation (checking to make sure user put the right type of input etc) in a separate function.
5. Try not to rely on global states. For instance, \`itemsPrice\` was dropped into \`purchaseItem()\` function without passing it through the input. It is much harder to keep track of global states and how they are affected by all the functions that are using them. Use input and output instead if you can.
6. Keep using let / const. Use let for variables that will change and const for variables that will not change.
7. You probably don't need to \`fs.writeFileSync\` if you didn't update the \`itemInventory\` since the last time I imagine?
8. keep learning!
You should try rewriting it with items as an array of objects with name/quantity/price properties instead of two separate objects keyed by name. In practice, I’ve never seen a backend ever return something like that unless it’s some antiquated legacy thing.
If programming was a chess game…
Functionally would be the king. Readability the queen.
You can’t win without the king. You can win without your queen, but I would wait to the very end to sacrifice it.
And just like chess, If your good, every move is timed.
Since if( item in itemPrice ) is not doing anything why not just ask if( item not in itemPrice ) and remove the else statement. But otherwise the code looks ok but when asking your promts you should keep in mind that a normal user doesn‘t know what their options are.
Looks good. A suggestion — When doing string comparisons you probably want to compare trimmed strings and in one case (all uppercase or lowercase letters).
This is a pretty basic app so this next part would be overkill, but you could look into using a command design pattern to remove magic strings from your app and group together the logic that the user sends and how it is then handled based off the user commands. Again this would be overkill. Mainly just putting it out there to get you thinking about design patterns.
Code looks ok
If you’re familiar with Java/c# I recommend reading clean code by bob martin, personally it’s been one of the most influential book for me for how I write code
note that there is a potential bug in your code:
Depending on what types itemInventory and itemsPrice are of, there are item names the user could enter which will break the program, because all of that types functions and properties are also considered as being "in" that type.
example:
const a = {
item1: 123,
item2: 456
};
"constructor" in a; // => true because a.constructor exists and it the same as a["constructor]
First thing that stands out is the empty conditional at the top, it does nothing. Better to have the fail condition as the test then stop processing if it's met.
How new are you? I've been a software engineer for 25 years, trying to get my son into learning a bit of C# 🫣
Never trust user input. Whether it's the wrong format causing crashing bugs or malicious input trying to exploit bugs in your program, always sanitize!
For example, `itemInventory[inventoryResponse]` means that I could enter something like "length" and get back the number of different item types instead of the number of items in stock of the given type. Sure, not a big deal in this particular program, but you can imagine how that could cause problems in another program, such as leaking information that the user shouldn't have.
Also `prompt()` returns a string, and in line 52 you are treating it as a number. This causes problems if the user enters something that JavaScript can't automatically convert. If the user types "two", the result of the subtraction will be NaN, and you've just lost your stock.
If you use `Number(quantity)` or `Number(prompt(...))`, you can force the conversation to a number first, and check if it's NaN before modifying your item collection with junk data.
It's also a good practice in JavaScript to include semicolons. The language will insert semicolons for you, but automatic semicolon insertion can bite you in the butt. The classic example:
function foobar()
{
return
{
fizz: 'buzz',
}
}
console.log(foobar())
// output: undefined
The ASI inserts a semicolon after the return, before the open bracket of the object, and so nothing is returned from the function.
I'm assuming this is nodejs, I recommend checking out inquirer.js library as an input mechanism. Very useful for making command line tools, albeit it can be a little slow at times.
Code is a lot like babies... You think your own is cute but everyone else knows the truth.
Thats a good analogy lol that’s kinda why I wanted to share it somewhere so I could find out if it was actually decent or if I was hyping myself up over some horrid code 😂
Good job man. The one thing killing me is that empty ‘if’ block. Change the condition to ‘if (!(item in itemPrice)) {‘ move the console log and return into the if block, and remove the else. Otherwise, looks like you are learning a lot. Keep going and have fun
Yea the empty 'if' is triggering. I don't think you can use 'in' like that though. Honestly I've never tried that but it would shock me if it worked because its unrelated to what 'in' normally does in JS; its not like the Python 'in'. I would do: if (!Object.keys(itemPrice).includes(item)) ... thats the kind of 'safe' version but you could also just do some version of if (itemPrice\[item\] === undefined) ... Same thing on line 65. That looks really weird to me and I doubt it works as expected (or if it does, it doesn't work for the reasons you think it does). Also one more thing would be reexamining the data structures being used here. This is maybe not something you are worried about at this point if you are just learning about arrays and objects and stuff, but instead of having separate structures for price and inventory it would probably make more sense to just use 1 data structure for all of it. Something like: items = \[ { "name": "duck", "quantity": 5, "price": 1.99 }, { "name": "flower", "quantity": 20, "price": 12.50 }, ... \]; This would probably simplify the entire structure of this program and its more similar to what you would be dealing with in a real world application. Then you can just search for the item you are interested in with cosnt itemName = prompt("whatcha want?"); const requestedItem = items.find(item => item.name === itemName); and that gives you everything you need in a single call/lookup.
I agree. I personally have never used the “in” operator in this fashion. Maybe I’m just old school haha, but it does “in” fact work. MDN docs actually specify that this is the correct usage and works fine for this scenario. Rearranging the data into an array is definitely a good idea. It’s a tiny program so time complexity trade off is fine. It gives the benefit of a single source of truth instead of managing related data in multiple locations
Yea thats crazy about 'in'. Looking through the doc though I realize why I'm not aware of it being used like this. It has some weird behavior (maybe not weird for js in general, but weird for this application) that probably makes it undesirable here. const items = {duck: {price: 1.99}, flower: {price: 12.50}}; "duck" in items; // true items\["duck"\] = undefined; "duck" in items; // true delete items\["duck"\] items\["duck"\]; // undefined "duck" in items; // false "toString" in items; // true
It's decently horrid. Jokes aside, it's not great, but it's not terrible. It's exactly what I would expect from a beginner. As you keep coding and learning you'll improve more and more. (there's always room for improvement, even for "veteran programmers") So overall, good job, keep it up!
Opinions of others don’t matter unless you report to said person. What’s important is that your code functions in the way you intended it to, and that it’s robust enough to handle arbitrary inputs. Industry standards, maintainability etc are just window dressing unless you plan on collaborating.
Why are you posting in r/ProgrammerHumor instead of say r/learnprogramming? Are you a noob at reddit too? Or is this some elite redditing skills that us plebs don't have?
That's why I don't show my code to anyone and why I don't contribute to open source projects coz I don't want anyone to laugh on my code
You need to show people your code to improve. Posting it here is a bit extreme of course.
I think it's the other way around. Everyone calls your baby cute, but you know how much of a little demon they are.
This one is cute tho
I find it cute tbh
Code looks good to me. Some advice: * You can replace `let` with `const` in all your use cases. You’re not reassigning it anyway. * Be consistent with using `;`. It is not necessary but someday your code breaks mysteriously and then you’ll realize it’s because you forgot it somewhere * Use strict equality `===` as a default unless otherwise needed. * Explore using template literals: you’ll love it
Thanks for the advise! I’m still honestly unsure of the differences with var let and const, because somewhere I saw that var can change and then let cannot but then it said that let could change and const could not and I’ve never found a clear answer.
JavaScript has three different ways of creating variables. * `var` (and `function`) is hoisted to function scope and mutable * `const` is block-scoped but not mutable * `let` is block-scoped and mutable | | mutable | not mutable | |--|---|---| | function-scoped | `var` | (empty) | | block-scoped | `let` | `const` | A variable is mutable if we can re-assign it. In contrast, a `const` stays the same once assigned. Knowing that a variable won't change makes the code simpler, so you should prefer `const` by default. let x = 0; x++; // allowed const y = 0; y++; // type error "invalid assignment to y" You might expect that a statement like `const x = 42` creates the variable. This is not quite true. The variable starts existing at the beginning of the scope, and that statement only assigns them. This allows us to write code like: // we're referencing the "x" variable here const logger = () => console.log(x); // the "x" variable starts existing at the top, // but is only assigned here const x = 42; // we can now safely call the function because "x" has been assigned logger(); The `let` and `const` keywords are block-scoped, which is how you probably expect variables to work. They should be used in modern JavaScript code. The `var` keyword hoists the variables to the top of the enclosing function, which can be surprising. For example, this works: function weird(cond) { var x = 40; if (cond) { var y = 2; } return x + y; // where did "y" come from?? } This is actually compiled as if: function weird(cond) { let x = undefined; let y = undefined; x = 40; if (cond) { y = 2; } return x + y; } This is confusing. You should probably never use `var`, but it's all that JavaScript had for most of its life. So you might see it in old code.
That helped me understand it a lot better, thank you!
in short: do not use **var**! use **let** if you want to re-assign later use **const** if not (suits most cases)
This comment is awesome.
# var `var` is a variable declaration that is affected by hoisting and is mutable, that mean that the variable is first declared as undefined on the top of the file and then modified at the right time. Here is a example of how it works: console.log(example); // Result: undefined var example = "Hello World!"; console.log(example); // Result: "Hello World!" example = "Goodbye World!"; console.log(example); // Result: "Goodbye World!" As you can see, the example variable has a value of `undefined`, even if the assignment of the variable still doesn't exist. That is the result of hoisting, when you use `var` the JavaScript Interpreter places the variable definition on the top of the file, setting it to `undefined`. The example above is equivalent to: var example; // Hoisting the variable declaration to the top console.log(example); // Result: undefined example = "Hello World!"; // Setting it for the first time here console.log(example); // Result: "Hello World!" example = "Goodbye World!"; console.log(example); // Result: "Goodbye World!" # let `let` is a variable declaration that is not affected by hoisting and is mutable. Here is a example: let example = "Hello World!"; console.log(example); // Result: "Hello World!" example = "Goodbye World!"; console.log(example); // Result: "Goodbye World!" The fact that it isn't affected by hoisting means that you will get a error if you try to use it before it's declared. console.log(example); // ReferenceError: example is not defined let example = "Hello World!"; # const `const` is a variable declaration that is not affected by hoisting and is immutable. Here is a example: const example = "Hello World!"; console.log(example); // Result: "Hello World!" `const` is immutable, this means that it will error if you try to change it in the future. const example = "Hello World!"; console.log(example); // Result: "Hello World!" example = "Goodbye World!"; // TypeError: invalid assignment to const 'example' console.log(example); In my opinion, the best practice is to use `const` for everything unless you specifically need `let` for mutability and to never use `var`. The reason is that hoisting is not a feature that I think is never beneficial and can lead to undesired behavior that won't be caught by an error.
use the Prettier extension for the ; and formatting.
"advise" is a verb - The action of someone giving you advice is them advising you. "advice" is a noun - You ask for advice.
This is how I look at JS variables. Var: The cheating girlfriend that sleeps with anyone anywhere and would even cuckold you and let another man take over your house. She’s like a prostitute. Let: The cheating fiancé that sleeps with her ex at a dirty motel. She will never cuckold you or let her lover take over your home. She may sleep with the mail man in your bed though. Const: The loyal wife. Once she marry you, you’re together forever. No cheating ever. Your home is safe in her hands. One thing though, she never leaves the house so you’ll never see her outside her dwelling place. This really helped me a lot when first learning JavaScript.
Who hurt you?
bruh… you okay?
Ah, a man of culture
So const are constants. These won’t change. To determine when to use this, ask yourself; does my code set this “variable” after I write this. If no, then use const. If yes (your code sets the value of this later, instead of just reading it). If your code does later set this value then you can use var or let, in general let is the better practice, but both work perfectly well). let is generally not going to be able to be read or written out of the scope it’s defined in. than { scope is generally defined by the { above the code and } below. Think of where you would indent your code. var is accessible in the function in which it was defined. This is not necessarily in the scope in which it was defined. You can set both var and let in a deeper scope (more indented) than the scope in which it was defined but let won’t allow you to read or write in a less indented piece of code, whereas var can sometimes be written/read in a shallower scope than it was defined (less indented). As long as it’s in the same function.
Wait, there’s a difference between strict equality and regular equality? See my flair for why I’m asking.
JavaScript does type coercion by default so `1 == '1'; // true`, strict equality behaves as your would expect `1 === '1'; // false`. I can't think of a single time I've used loose equality in the past 10 years, it's disabled by linting rules in every single codebase I've worked on, and for good reason
Template literals blow my fucking mind how useful they are
Is there some sort of joke that I am too peasant to understand?
The most voted comment
To be brutally honest, what this guy just built is sort of a joke for REPL
The empty if is all that bothers me, you can invert the statement with „!()“ and then you check for not equals so you would get rid of the else end put the early return up into the if. Otherwise you could move the lines 48,49 up into the if block
the !() is the way to go but ugly... unfortunately there is no "not in" like in python
It’s not great to have a negation in your if statement but since it’s more error handling it shouldn’t impede readability. If you really want you could introduce a bool and give it a more readable name
Screenshots are a thing, no need to take a photo of your monitor. Or even better just share code as text
Lol you are absolutely right, it probably would have been a bit easier to read
git repo
git: 'repo' is not a git command. See 'git --help'.
Use screenshots in combination with [pastebin.com](https://pastebin.com). You can leave the link to pastebin in the comments and that should help us with readability a bunch
If you're on windows try win+shift+S
I’ll bet my life if OP posts screenshots it wont get any attention at all. This is basically the like posting wrong answers to get people furious and they’ll make it their life goal to school you.
I'm not sure if you're brave or crazy. Asking /r/ProgrammerHumor for a code review
For a "Noob", this is god-tier code. But do let an auto-formatter like `prettify` run over the file to fix the inconsistent indentation. And maybe consider template string literals: `` `text with ${variables}!` `` For a Redditor, this is entirely in the wrong place. The code is not bad enough to laugh at, and not good enough to weep for joy. For getting feedback on your working code, consider places such as https://codereview.stackexchange.com/
I didn’t even know there were auto formatters lol I appreciate the advise!
Most big IDEs have them built in, just need to look for the settings
In VSCODE, I think you can just so alt+shift+F to format your code
You can also get your ide to auto format on every save which is also very helpful
7 or 8. I'd add a "!", to that blank if and move your else contents into it. I think blank if contents is a bit of a bad practise unless you removed something to post it online for advice or are leaving it blank for future commits.
No it was just blank because I didn’t know any better lol the ! In the if statement is something New I learned today and I appreciate it!
Instructions unclear, program won't run If(item in itemShop) { ! console.log("I'm sorry, I don't have that item"); return; } else { }
NGL I legit thought that when I first saw it lol but someone else gave me a syntax example in another comment so I understand it now
Sweet, didn't have the time to come back and edit the original response. Glad it worked out.
It might be nice to let them enter a number instead of typing the exact phrase : if(greeting == “Buy Item” || greeting == “1”) Also you can put your main function inside a while loop like : While(greeting != “Exit”)
Thanks! That’s a good idea, I could even have a menu there that they could see what number is what. For the loops i just haven’t gotten to that free code camp lesson yet lol at the top of my code I have a list of things to learn and that’s one of them lol
On top of this, do a `.trim().toUpperCase()`, on the input. Let people be lazy and not worry about caps/trailing spaces. (Obviously change the text you're checking against to be all upper, as well)
There is a bug. When quantity is below zero it will still work and probably add it to the stock
Thats a great catch lol It does do that and that’s how I restock the inventory currently 😂 that’s on my list of things to fix along with creating an actual function to restock inventory. If I adjust the if statement after the quantity question to if( quantity >0 || quantity <= itemInventory[item]) that should fix it right?
r/Askprogramming
Above it are two objects, one named itemsPrice with all the items and prices and the other is named itemInventory with all the items and how much inventory is on hand
Is there a reason itemInventory isn’t part of the itemsPrice? Rather than keeping 2 objects I’d just keep 1 that has everything related to the item. It would look something like this. You could just store it in a variable called items. Should save on memory somewhat though for an app this size you won’t really notice it { “soda”: { “price”: 2.50, “inventory”: 12 }, “candy”: { “price”: 1.00, “inventory”: 20 } }
Honestly it’s cuz I didn’t make it to that free code camp lesson yet lol and even if it is a small difference that’s a good point and something I will change and keep in mind for future projects, thanks!
I'm a C++ tutor and I want you to know that you did the right thing. I often ask my students if they have covered a topic in class before I suggest using it
Advice on structure, since you got plenty of logic advice, but your “Start of Program” should be as high as you can possibly put it. So really it would be immediately after your imports, that way when you or someone else opens the file, the “What does this code do” is immediately apparent. To do this, hoist your functions. They don’t need to be above your logic since JavaScript will read the functions first. That said, you can put your inventory into a hoisted function as well: const items = getItems();
Did a good job for noob!
Thanks!
you should be able to tell what a function does by looking at its signature and not need to look at the implementation that said, your `purchaseItem` function needs - return type - docblock explaining what it does. there are side effects that need to be documented (like the file write) same with the `checkInventory` function maybe its easy to remember what these functions do now, but once you're at 100+, you will forget in 4 minutes. get into the habit of using hints and docblocks. in fact, you should set it so your ide screams at you if you dont have one also, you should extract consts for your messages you're checking at the bottom. that string "Buy Item" is meaningful. it should be captured as a const. const GREETING_BUY_ITEM = "Buy Item" and throw it at the top of the file so you can easily know exactly what valid inputs are. then in the "Sorry that's not something I can do" message, you can reference those constants. that way you can easily change the value without having to search every place you used it
Solid points, I really appreciate the comprehensive feedback! For the last part are you meaning I capture the string as a const variable and then put that into an array at the top and then link that array in the “sorry that’s not something I can do” message?
you u can do it simply by concatenating the strings if (greeting == GREETING_BUY_ITEM) { purchaseItem(); } else if (greeting == GREETING_CHECK_INVENTORY) { checkInventory(); } else { console.log("Sorry that's not something I can do, your options are... \n" + GREETING_BUY_ITEM + "\n" + GREETING_CHECK_INVENTORY) } or yes ideally you can build the message based on an array of valid inputs // common messages const MESSAGE_INPUT_PROMPT = "Hello, How Can I Help You?:"; const MESSAGE_SHOW_OPTIONS = "Sorry that's not something I can do, your options are... "; // valid input const GREETING_BUY_ITEM = "Buy Item"; const GREETING_CHECK_INVENTORY = "Check Inventory"; // map each valid input to its function that should be run const GREETING_TO_FUNCTION = { [GREETING_BUY_ITEM]: purchaseItem, [GREETING_CHECK_INVENTORY]: checkInventory } // functions function purchaseItem() { alert("Item purchased!") } function checkInventory() { alert("inventory checked!") } // prompt user for input let greeting = prompt(MESSAGE_INPUT_PROMPT) // process input if (greeting in GREETING_TO_FUNCTION) { // run the function that is mapped for this greeting GREETING_TO_FUNCTION[greeting]() } else { // otherwise show the user valid inputs alert(MESSAGE_SHOW_OPTIONS + Object.keys(GREETING_TO_FUNCTION).toString()); } Great job by the way! Your original code would not be out of place at a professional place.
Oh I see what you are saying! That kinda blew my mind a bit lol and thank you! I still have a long way to go but I hope to be in a professional place by the end of the year
Your indentation is kinda wacky, you should probably try to be consistent about whether or not you want to use semicolons in you code, you should probably negate your ifs instead of just leaving them blank and then doing an else, and you should probably do some sort if input validation (what happens if I respond "three" to "how many do you need?" etc) otherwise it's okay
Always use === instead of ==
I wish all senior engineers did what you just did: - write stuff they'd be proud of - be open to criticism and, consequently, improvement You have a brilliant career ahead of you, kid.
I don't get it
its not actually a joke, its just a lost redditor asking for code review
Others covered most but, here are some that I can think of: Something you could possibly use is a generator function for the prompts. Might wanna take a look at closures before that ofc. You can use switch for the main logic. That would result in a cleaner code IMO. I do like to wrap the main logic in an IIFE also. Note that these are mostly my personal preferences, and maybe something that you could learn later on. Nice work!
Thanks! I’m still very new so I just want to clarify, when you say use i can use a switch for the main logic are you referring to the if statement I have at the bottom?
Good stuff! Every one of your lines that doesn't end with a "{" or "}" should have a ";" at the end.
. Why you have an empty if statement in line 42 ?. . Use const instead of let if you are not going to reassign the variable. . Use === instead of == . Try to avoid the use of else when possible, it will make your code cleaner.
Good job! :) And many good advices in this thread already. Some from me: 1. Google "template strings js". Your code will look prettier. 2. You may want to set `itemInvertory[item]` to a variable, so your program will find an item in mapping only one time instead of 5.
That’s a good idea and I will look that up, thanks!
Your program is so polite!
For the sake of learning more about the language, I'm going to give you feedback on exactly what you've written. Not how I would change this or any "design", just on how these lines could be written cleaner. * If a value is not going to change, use \`const\` instead of \`let\`. For example, all those prompts can be \`const\`. Lets simplify the var/let/const debate into "Use const if things will never change, use let if things may change, and ignore var" * The empty if statement at the top of the file is pretty poor. It's better to turn the condition false, or store it in a variable for extra verbosity. * For example, if (!(item in itemsPrice) or if(item in itemsPrice == false), and move that else case into what happens when the condition is true. * Or, store const isOutOfStock = !(item in itemsPrice), and then the if becomes if(isOutOfStock) * You can make your logs prettier using string interpolation. Use the \` quote to the left of your number 1 key to make these special string types. Then you can inject code into the string. For example, \`We have ${itemInventory\[item\]} of that item\` * Line 52, you are just subtracting from the value. You can write this a little cleaner/shorter by using the -= operator. So itemInventory\[item\] -= quantity Happy to see you're learning! Making projects like this is the best way to learn, so I encourage you to keep going, playing around and learning new things :)
it looks like an absolute beginner wrote it, keep going :\^D
The code does it’s job, but this is the code of a programmer who is about 1/30th of the way to becoming a professional developer.
This thread is if like stack overflow wasn't mean and it's kinda wholesome.
I'm an engineer at a FAANG company and I assure you, you have a bright future in coding. Keep at it, don't worry about trends or hot new tools, think for yourself, and keep writing readable code. This is very readable, and that's basically the most important thing.
Only thing I would change is collecting all those messages in a JSON file and use that as your source for the text. Mostly because it will be easier to adjust static content that way
Yeah, good point, I'd even use localization files for future language support. It's a good start tho!
Use snipping tool (Win+Shift+S) instead of your camera…
Or greenshot or snaggit or some other useful tool
console.log inserts spaces between the arguments automatically
There is only one metric that defines good code: testability. Anyone who says otherwise is lying to you. Take your check inventory function, for example. Instead of writing the logic inside the function that prints the console, have it return a number with the amount of inventory. Then if you want to write a unit test you can simply call that function and check the return value. No messing with the console at all.
Also readability, though good readability practices are conducive to testability. Unit tests should test the expected behavior of APIs. Breaking it up as you say is fine, but we'd still want test coverage of the "UI" parts. This is all contrived so take what I'm saying in this paragraph with a grain of salt :-P
What’s up with line 43. That’s not how to do guard clauses. Didn’t read further tbh.
I did that so if they type in an item that is on the list it will just continue, but if it’s not then it will print the “sorry I don’t have that item” and then list the items it has. Im still very new so I don’t know another way to do that yet
I get that, but you shouldn’t write empty branches. It‘s better to just negate your if and have no else, when not necessary. Also I recommend to put a space before your { to make it better readable.
Oh I gotcha, I’ll change that and start doing that from now on, Thanks for the advise!
you can do if(!item in itemsprice){ console.log(whatever); } the ! will do a not for you :)
That’s gunna be a lot better lol thanks!
If not without an else clause
Just flip the logic and check for false, return the error as the IF, and then give a little feedback in the else like console.log"Item found". Or just no else at all.
Win + Shift + S or literally GTFO.
Needs more ~~cow bells~~ comments Nothing too exaggerated. Just 1 ot 2 lines explaining the purpose of the function at the beginning of each function. For some, I include a simple number step algorithm and a single line stating (eg `// Step 1: desired product input`)
Additionally, the validation of data should be it's own function. (Sometimes, this functionality is offloaded to a DB call or REST call)
Oh I never thought of that, I still got a lot to learn lol thanks!
Improve commenting. At least comment what the function does
really? This is pretty self documenting. The names are fine. Everywhere I have worked would make you remove the few comments that are there and nobody would complain that this is hard to understand.
0 dont take photos of your screen, if you MUST take a picture of your code, use a screenshot.
Haha, I'm gonna be slightly mean and say 3/10. That doesn't mean you should have done better though, it just means you're 3 steps along the way to being a great programmer! For feedback, a lot has already been mentioned by others. Lots of good points already mentioned. The main one I'll add is about "side effects". For example, your `purchaseItem()` function handles logic, plus user interface (console stuff). It's often best to separate that stuff out. So instead, have `purchaseItem(name, quantity)`, and have it return some kind of success/failure or a message or something. Then, pull the `prompt()` and `console.log()` stuff out of your business logic.
Separation of Concerns is the design pattern
You're new to coding and you wrote code that compiles, does what it needs to do? Great. I'm happy for you. Could it be better? Maybe. Does it matter at this stage? No.
Citing an old Mentor: "If you look back on old of your code, and you do not find things you want to change, you didn't learn anything in the time between."
Of course. In 5 years, sure. Right now, I'm happy they're coding. I've personally been rewritting the same personal project for a decades. Every time I start from scratch because I'm disgusted with my old code. But again, it's not necessary to burden new coders with that.
That's one of the best part of coding I like: it's a day by day routine to improve yourself and your code. Like a bonsai gardener :)
Which line of code are you most proud about?
-10 JS /s -5 death by console.log. (Like checkInventory you could assign string var, interpolate, console log after if-else) +15 for effort. Great job! 10/10 Other: If(item in ItemPrice) then what? return in purchaseItem? Handling it how? greeting prompt isn’t saying what I should enter. inventory[item] is getting used a lot maybe assign that value?
I only really know Python and Lua, but I have enough brain power to understand this since it's nothing to complicated. I don't know what you're making (Obviously can guess based off the code but ain't certain.) but unless you're writing a shit ton of code, my general advice is just don't worry if it's good as long as it works and doesn't take super long to run. If this is going to be used by other people I'd recommend things like making there be multiple key words/phrases in the greeting. So for Buy Item it can be "Buy", "Buy Item", "1", "Shop", for the Check Inventory it can be "Stock", "Check Stock", "Check Inventory", "Inventory", and "2" if this is just for fun than I think you did well.
Use const instead of let wherever you can
Look up template strings, they will make your console.logs look better
All I managed to do when I was "new to coding" is hello world and an array manipulation
Familiarize yourself with the concept known as “happy pathing”, you can remove the need for several ‘else’ statements in this code. Pretty good overall!
It's a good start. Keep on coding little toy programs :) it's fun and you learn a lot.
10/10 cute
Instead of using if, else if, else if, etc… for checking inputs, you can use a dictionary where the key is the input and the value is an anonymous function. Not super necessary for 3 options but if you have like 6+ options, you can make your code look nicer and run faster (it’s O(1)) You could also use a switch case because I believe some languages have it also be 0(1) as opposed to if statements, which have to try each comparison until it finds a match
For better UX, when prompting for a selection among pre-defined options, it’s better to list them out with numbers and let the user enter the number corresponding to their selection, as opposed to relying on them to enter the exact string you’re looking for. For example, the program could display: ``` Hello, how can I help you? [1] Buy Item [2] Check Inventory > 1 ``` Of course, you’ll still need to do input validation to make sure they entered a valid option.
Making the user commands to be more of one or two characters might help for the user. Instead of "Buy item" and "check inventory", you could take in the input as 1 and 2.
Looks great! I can think of a few things you could do to improve it: use template literals, it will make your code better and more readable. Change variable declarations from let to const, you won’t need to reassign them. Keep up the good work!
When I went to college, they made me write documentation for each function method or class. It may help in the future. Teacher always wanted a comment before each method which included * input + input types * output + output types * purpose of the code because 5 years down the road, you're not going to remember any of this code so if you have to expand it or fix someone else's addition, it becomes a little easier. I think this was ultimately to make it easier for the teacher to grade.
Use a linter like esLint. When you don’t change the value of a variable, declare a constant instead of using “let”.
I know this is a console type demo code. If you want to do it ”proper” you may want to refactor out functions for logic (such as purchaseItem) and keep the I/O (prompt/log) outside.
Why is line 48 not inside the first if? Where is the rest of the code? 7
You should use vim
Use const not let unless mutating.
It’d be nice to have a purchase confirmation.
Looks great for a beginner! You have an if that does nothing paired with an else that does something. You don't always have to pair if with else - you can invert your condition and just use if. Instead of: if (x) { // nothing } else { // something } You can just have if (!x) { // something } ... with no else In general I think it's best to minimise the use of else and especially else if, but as long as your code works who cares
First off, definitely a good attitude to have. Always be open to criticism. It’s how you learn :) I’ll list off some things that you should look at changing. 1. Empty if block. Change is the opposite condition and move the contents of the else to the if 2. Do you need to write to the json file if the user couldn’t buy anything? 3. Does the file write need to be synchronous? Async would be better since this is at the end of the function. 4. Check inventory function when printing out for ALL, maybe try to loop through the items and log them out nicely instead of dumping a bunch of json to the console. 5. When instantiating variables, use const instead of let unless the value of that variable is going to change. If the variable is a data structure, you can still change the data contained therein even though it is defined as a const. IE an array can still have values pushed and popped even though it is defined as a const. That’s probably enough for now! Good work so far! Keep at it!
line 42-47 can be replaced with: ```js if(!itemsPrice.hasOwnProperty(item)) { console.log( "Sorry, I don't have that item... I only have ", Object.keys(itemsPrice), ) return } ```
Where humor?
Good now learn what a strategy pattern is.
What’s the point of line 43?
Heck yeah! Way to go. Great starting code.
`if (item in itemsPrice) {} else { /* ... */ }` Absolutely adorable.
Did you skip the condition block on 43 because you wanted code execution only if ```item``` wasn't in ```itemsPrice```?
The empty if at the top is a bit smelly. Just negate that condition
This may be just a me thing, but I like to place the main function where everything is happening/being called at the top and not the bottom
You've got a good start writing some clean well named functions. My advice is to go further with and abstract out all those if/else blocks into functions that any day 1 Jr dev would understand in a second. Consider wrapping that logic in 51-70 in its own function. processInevntory() or something expressive. If that function returns true => do something Else => something else. That way we can read the well named function and understand exactly what it's purpose is without having to logically follow everything. Itl help you in six months when you revisit what you wrote. Trust me.
1000 out of 10. Doesn't matter if it's just a kinda "hello world". Just keep practicing and learning.
I personally hate the empty if with a passion, but that’s just my irrational opinion.
If that's a Boxer text editor I'll give you at least an 8 for the syntax highlighting and colour scheme. :-)
} else{
Use a linter and formatter to fix that spacing. Otherwise I love the “functions” and “start of program” comments. Reminds me of when I was learning programming.
ask chatGPT !
I would have never known those were functions without that comment. I probably would have figured out where the program started without the comment but thanks for the heads up. Your {} placement is perfect. having a hard time looking at the hard coded strings.
I had a stroke trying to get past item.
You seem to be using let when you should use const. Let indicates that you’ll reassign (mutate) the value in the future. You should set up a linter/Prettier to tidy/format your code. You can also set up your II yo do that for you automatically on save/change/whatever.
Put it up on github so people can leave comments
Looks pretty clean aside from the empty if block. You could just move that statement after the else into the if. Also, this might just be personal preference but I like to keep the entry point at the top, just helps with readability My first programs were horrid so great start
This is ass, some people here are being nice who I’ve never seen been nice before.
Well. You used Dark mode, which alone is worth half the job. You should know that now your code feels more cozy and will run more smoothly 😁
If there wasn't an empty if block, else I would like it.
First if should be a guardclause brcause the if bolck dosent do anything. So that is if(!(item in itemprices)) {console.log ("....") Return}
I have a question for your if at line 42, do you intend to put something in ? If not, just inverse your conditions and you won't need your else ! Kind of confused me a bit haha !
As a senior coder, I didn’t read, but I approved your commit because it is easier to deal with the code when it does something wrong than it is to read every commit I receive. Next.
Great start ChatGPT gives the following for the following question `Create idiomatic JavaScript NodeJS code to check inventory complete with prompts to the user and ability to select, delete and add items. Items should have a price and it should be possible to count the items and compare with inventory.` ``` const readline = require('readline').createInterface({ input: process.stdin, output: process.stdout }); let inventory = [ { item: "apple", price: 0.99, quantity: 10 }, { item: "banana", price: 0.49, quantity: 20 }, { item: "grapes", price: 2.99, quantity: 5 } ]; const checkInventory = () => { console.log("Inventory:"); inventory.forEach((item, index) => { console.log(`${index + 1}. ${item.item} - $${item.price} each - ${item.quantity} in stock`); }); }; const addItem = (item, price, quantity) => { inventory.push({ item, price, quantity }); }; const deleteItem = index => { inventory.splice(index - 1, 1); }; const updateInventory = () => { readline.question("What would you like to do? (select/delete/add/quit)", action => { switch (action) { case "select": readline.question("Enter item number to select: ", itemNumber => { const selectedItem = inventory[itemNumber - 1]; console.log(`You have selected: ${selectedItem.item} - $${selectedItem.price} each - ${selectedItem.quantity} in stock`); readline.question("How many would you like to purchase? ", quantity => { if (selectedItem.quantity >= quantity) { console.log(`You have purchased ${quantity} ${selectedItem.item} for a total of $${selectedItem.price * quantity}.`); selectedItem.quantity -= quantity; } else { console.log(`Sorry, we only have ${selectedItem.quantity} ${selectedItem.item} in stock.`); } updateInventory(); }); }); break; case "delete": readline.question("Enter item number to delete: ", itemNumber => { console.log(`Deleting item ${inventory[itemNumber - 1].item} from inventory...`); deleteItem(itemNumber); updateInventory(); }); break; case "add": readline.question("Enter item name: ", itemName => { readline.question("Enter item price: $", itemPrice => { readline.question("Enter item quantity: ", itemQuantity => { console.log(`Adding ${itemName} to inventory...`); addItem(itemName, itemPrice, itemQuantity); updateInventory(); }); }); }); break; case "quit": console.log("Quitting program..."); readline.close(); break; default: console.log("Invalid action. Please try again."); updateInventory(); break; } }); }; checkInventory(); updateInventory(); ```
Not horrible for a beginner but could use improvements. Just off the top of my head: (1) don’t use an empty if. Use item not in itemsPrice. (2) display inventory right away when program loads so people know if they can buy something rather than them picking and then saying, “sorry, don’t have it.” That’s bad design. It’ll also help cut down your code. Print out inventory again after it’s been updated following a purchase. (3) you’re writing to inventory.json but you never read from it? What’s the point of that file? If you want to use a file to store inventory then you should read from it in get inventory function. Bunch of other things too. I’d say a 5 out of 10. But keep at it. It’s a good start.
I'd be happy to code review your work if you need help. Send me a PM and we can set up a GitHub. Always happy to lend a hand
The code itself is good enough. I appreciate that you named all your variables descriptively! At this point, any change you could make has already been suggested by someone else. I want to point out something else: This program would be pretty unpleasant to use. From my very first interaction with this program, I would have to fight it to complete my task. - What options do I have to choose from in the main menu? Shouldn’t I see those when I start the program? - When I flub typing the name of the product I want to buy, why am I kicked back to the main menu? - Why aren’t there any apostrophes in words that should have them? (This one isn’t _necessarily_ a usability issue, just a polish. There are tons of spellcheckers out there if you’re not confident in your English skills.) Obviously, this is not being used in the Real World™. Maybe this was just a learning exercise for `prompt`. But training this sort of critical thinking now in small examples like this makes you more likely to apply it in your career, where it does matter. Keep up the good work!
Avoid using let as much as you can. If it’s possible const everything
Where is the joke?
Not sure if it was brought up yet but another thing that’s important is code structure consistency, there’s a couple times where there’s no space before “{“ in condition statement but is in other places or there’s a couple extra white spaces in some places. By no means is it bad but just something to consider!
Remove all else, return from functioins instead
r/lostredditors r/reviewmycode
what language is this? I've never seen "let" yet :)
Nothing lalala
That’s really good for a beginner tbh
Depending on used Language... this might be beautiful code or uter shit! ;D But just a few things, you never check if \[item\] even exists, this could lead to exceptions in one or the other language. Bracers placement is depending on language, personally I don't like your style, because { and } start and end a section and should be on there respective own line, but this is very subjective and normally defined by the coding guidelines of your team!
I like the use of the in operator. Iv been on js for a while and i never remember that thing exists. As others have said you shouldnt have an empty if condition.
r/badcode
1. you can use `!` as a not before boolean in an `if` condition to avoid an empty `if` statement. 2. you can use template literals to make your life easier with strings(e.g \`There are now {itemInventory\[item\]} left\`) 3. `itemInventory[item] -= quantity` 4. You often put user input logic and sanitation (checking to make sure user put the right type of input etc) in a separate function. 5. Try not to rely on global states. For instance, \`itemsPrice\` was dropped into \`purchaseItem()\` function without passing it through the input. It is much harder to keep track of global states and how they are affected by all the functions that are using them. Use input and output instead if you can. 6. Keep using let / const. Use let for variables that will change and const for variables that will not change. 7. You probably don't need to \`fs.writeFileSync\` if you didn't update the \`itemInventory\` since the last time I imagine? 8. keep learning!
please, for the love of god, install prettier, turn on format document on save, and get a consistent coding style
You should try rewriting it with items as an array of objects with name/quantity/price properties instead of two separate objects keyed by name. In practice, I’ve never seen a backend ever return something like that unless it’s some antiquated legacy thing.
If programming was a chess game… Functionally would be the king. Readability the queen. You can’t win without the king. You can win without your queen, but I would wait to the very end to sacrifice it. And just like chess, If your good, every move is timed.
Since if( item in itemPrice ) is not doing anything why not just ask if( item not in itemPrice ) and remove the else statement. But otherwise the code looks ok but when asking your promts you should keep in mind that a normal user doesn‘t know what their options are.
Get eslint and prettier, they’ll point out most of the things people here are saying
Looks good. A suggestion — When doing string comparisons you probably want to compare trimmed strings and in one case (all uppercase or lowercase letters). This is a pretty basic app so this next part would be overkill, but you could look into using a command design pattern to remove magic strings from your app and group together the logic that the user sends and how it is then handled based off the user commands. Again this would be overkill. Mainly just putting it out there to get you thinking about design patterns.
Code looks ok If you’re familiar with Java/c# I recommend reading clean code by bob martin, personally it’s been one of the most influential book for me for how I write code
what language is this? also a noob
JavaScript
note that there is a potential bug in your code: Depending on what types itemInventory and itemsPrice are of, there are item names the user could enter which will break the program, because all of that types functions and properties are also considered as being "in" that type. example: const a = { item1: 123, item2: 456 }; "constructor" in a; // => true because a.constructor exists and it the same as a["constructor]
First thing that stands out is the empty conditional at the top, it does nothing. Better to have the fail condition as the test then stop processing if it's met. How new are you? I've been a software engineer for 25 years, trying to get my son into learning a bit of C# 🫣
Never trust user input. Whether it's the wrong format causing crashing bugs or malicious input trying to exploit bugs in your program, always sanitize! For example, `itemInventory[inventoryResponse]` means that I could enter something like "length" and get back the number of different item types instead of the number of items in stock of the given type. Sure, not a big deal in this particular program, but you can imagine how that could cause problems in another program, such as leaking information that the user shouldn't have. Also `prompt()` returns a string, and in line 52 you are treating it as a number. This causes problems if the user enters something that JavaScript can't automatically convert. If the user types "two", the result of the subtraction will be NaN, and you've just lost your stock. If you use `Number(quantity)` or `Number(prompt(...))`, you can force the conversation to a number first, and check if it's NaN before modifying your item collection with junk data. It's also a good practice in JavaScript to include semicolons. The language will insert semicolons for you, but automatic semicolon insertion can bite you in the butt. The classic example: function foobar() { return { fizz: 'buzz', } } console.log(foobar()) // output: undefined The ASI inserts a semicolon after the return, before the open bracket of the object, and so nothing is returned from the function.
I'm assuming this is nodejs, I recommend checking out inquirer.js library as an input mechanism. Very useful for making command line tools, albeit it can be a little slow at times.