We’re rewarding the question askers & reputations are being recalculated! Read more.

Hot answers tagged

44

I guess what they were looking for was methods on classes. Also I recommend you download resharper and see what it says about your naming conventions as they are non standard. A 'Classic OO' as I call it approach might look a bit like public class Product { public string Name { get; private set; } public decimal BasePrice { get; ...


26

People who want to teach other should not write code like this. Not even slightest indication that you understand what Separation of Concerns is. The pattern that you are using is called Big Ball of Mud. You have everything in the same file. No understanding of HTML. The code you wrote is not valid, it uses tables for layout and completely ignores semantics....


26

As an interviewer, I want to see candidates demonstrate the skills I'm hiring for. In particular, I want to see unit tests, because I want you to write tests for your code if you come work for me. If you had written unit tests for your code, you probably would have caught many of the items listed in the other answers. You at least would have demonstrated ...


26

First of all, you should encapsulate your functionality in a function, unless you really want to use your code only once. At the moment you would have to duplicate your code somewhat just to check a second credit card. So instead let us write a function with a doc string: def is_valid_card_number(sequence): """Returns `True' if the sequence is a valid ...


21

Some minor changes I would recommend: Replace the magic numbers with symbolic constants. It will be easier to modify your program when these rates change. constexpr long double ANNUAL_MORTGAGE_COST = 0.03; constexpr long double LOAN_PRINCIPAL_PLUS = 0.06; constexpr long double MARGINAL_TAX_RATE = 0.35; Instead of doing the calculations inside main() I ...


20

Your assessment of your own code is pretty close to accurate: it seems quite long and looks like a huge mess. On the other hand, you have worked through what appear to be all the use cases, and you have comprehensively solved the problem. You have put a lot of work in to manually building out a logic tree, and calculation system, to get the conversions ...


20

I think they were probably looking for you to split out the responsibilities into classes. You created your product class to encapsulate the data but that's where you stopped. You stuffed everything to do with UI (writing/reading the console) and calculating the tax in the main Program class. You at least need a separate class for calculating tax: public ...


18

Are you a horrible programmer...no probably not. Is there room for improvement? There usually is. here's my two cents: I see one factory class, but then you have a whole-lotta creation work going on throughout the rest of your code new fMoney, new TaxBracketManager, new TaxBracket, etc... When you hard code these dependencies into your methods how ...


18

Here a few suggestions (in no particular order) for improvements on your code: Make variables have locality The line: items = 1 is at the top of the method, but is only used by the loop at the bottom of the method. It is likely better located just in front of the loop that increments it. Same applies to: tickerdict = {} You can break strings across ...


16

A number of points: Do not use floating-point data types (float or double) for amounts of money. Floating-point data types have limited precision, they cannot represent all decimal values exactly, and you're going to get roundoff errors sooner or later. Use integers instead (int or long) and store cents instead (for example, 1040 cents) or use BigDecimal. ...


16

I'm going to be brutally honest, because I think it will do the most good for you as you look for a job. You say you "have the right frame of mind" and "just need someone to 'nudge' [you] in the right direction." You're way ahead of most of the pack, as you are capable of something much harder than FizzBuzz. However, this code needs a lot of work and a lot ...


16

The company was likely looking for code that demonstrates object-oriented modeling. You have Product objects, but the description also mentions Taxes and Receipts. A more complete model might look like the following: public interface Product { string Description { get; } decimal Price { get; } bool IsImport { get; } bool IsFoodBookOrMedical {...


16

You're using the wrong tool for the job. If there's ever going to be a loan that I will pay an amount to every week for 2 years, you're gonna have to write a script to generate the code for you, because doing it by hand would suck. You'd have to write code for 204 different enum values, including the switch cases. If you had to map months to strings, you ...


16

Python doesn't have private anything but we still have a need to separate inners of a class from the public interface. By convention, attributes or methods whose name starts with a single underscore are considered internals of the class and, thus, not part of the public API. There is no mechanism in place to restrict their access, so user that know what ...


16

In addition to what @StephenRauch wrote in his excellent answer, let me add one more comment: Your class is completely useless. Not its functionality, which is fine, but it being a class is completely unnecessary. It does not inherit anything from a parent or has children which inherit from it, it has no attributes and therefore no state, and the one ...


15

Start using New IO The java.io packages have been replaced for a reason. They don't integrate well into what the language tries to become and are at best unwieldy. Instead you should rely on the new IO or nio packages, that work on Paths instead of Strings and have some other advantages. Loading your properties can be simplified to: try { ...


14

Over-all, you say you are a beginner programmer, but you are off to a very nice start! All my suggestions focus around one concept: the Interface. You are definitely ready to read Joshua Bloch's "Effective Java 2nd Edition" which is all about creating great interfaces. +1 using generics with collections -1 import wildcard - Though it's a pain, you should ...


14

A few minor things to complete what has already been said. Almost none of them are significant, but good practice is good practice. You don't need to use std::endl: it prints a newline character and flushes the buffer, which is unneeded here. You can simply use \n. It won't make a difference in your program, but it is still good practice. Note: as pointed ...


14

Account Structure Since it looks like you are trying to model the bank account slightly more realistically than a typical getting started tutorial, I'd like to address the conceptual structure of your BankAccount class. In real life, an account is much more likely to be composed of a collection of transactions, rather than an updated balance. Getting the ...


13

Veilig is right and made some very good points +1. There's nothing "horrible" about this code. Everyone can stand to use some improvement somewhere, even professionals. So saying this is "horrible" is just naive and immature. Don't listen to anyone with such negative criticism, especially if they aren't willing to explain themselves. Even constructive ...


13

Overall it's not bad, but here are a few things that stuck out for me. I'd consider renaming getBalance to getBalances, since you're actually returning the balances from multiple bank accounts for that user. balance = bankAccount.getBalance(userId); Consider creating and returning an array of AccountBalance type from the getBalance method instead of an ...


13

In addition to all the other good points mentioned: This code massively mixes up presentation logic, business logic, and back end database implementation details. Those are three separate things; they should be kept far apart in the code. This code has "sql injection attack" written all over it. Don't ever use strings to build up SQL queries. Use stored ...


13

Comments /** * @param args the command line arguments * @throws java.io.FileNotFoundException */ You're stating the obvious and forgetting the important things. Yes, args is the command line arguments, but that's standard for the Java main method. I'd suggest documenting which command line arguments you actually accept - namely only one, and it should ...


13

Janos has some great pointers. There are other issues that stuck out quite a bit, from my experience, so I'll add mine: Product-specific logic outside of Product Class Your Generate() function has logic that seems specific to a Product, which contradicts the object-oriented nature of C#. The specific code I'm referring to is: foreach (var p in products) ...


13

In this answer I'm not going to review your code, but instead discuss the design of a book-keeping system. (I know that you're just using a bank account as an exercise, but an important part of programming is thinking about design like this.) The code in the post implements account objects with methods for transactions. For example here's a transaction ...


12

As @Keith says, credit card numbers should be treated as strings, not numbers, and definitely not floating-point numbers. If you want to ensure that the input contains only digits (and maybe spaces), use strspn(). Squeeze out any spaces, then validate the length using strlen(). The Luhn checksum check should be in its own function. The loop indexes would ...


12

This is way too complicated. You could have just the cents as a member and have some helper functions that converts it to the respective parts. This will simplify most of your functions a lot. const Money operator +(const Money& amount1, const Money& amount2) { return Money(amount1.cents + amount2.cents); } const Money operator -(const Money&...


12

A couple of small suggestions. Var: You spend a lot of time redeclaring variable types when they are already well established by your code. For example: decimal totalprice = 0.00m; foreach (ShoppingCartItem item in itemList){...} List<ShoppingCartItem> lstItems = new List<ShoppingCartItem>(); I know there can be some debate about whether var ...


12

You can do most of this with regex. Going through your requirements: It must only consist of digits (0-9). And so we can use [0-9] for each digit. It must contain exactly 16 digits. And so we can use something like [0-9]{16}. However, that only checks that it contains sixteen digits, so 1234567890123456abc would be valid. So we need something like ^[...


11

"User Proofing" Don't use double As already mentioned by toofarsideways in the comments, storing currency values as float or double is problematic. Floating point cannot represent all numbers; it will get you close though. It is highly recommended that you use another option better suited for accuracy (like BigDecimal) 0-length input Optional last name ...


Only top voted, non community-wiki answers of a minimum length are eligible