7

Construction like boolean pre = getPrefix().equals("4") ? true:false; is redundant. The following is ok: boolean pre = getPrefix().equals("4"); just as if(pre && len) { return true; } return false; this could be just: return pre && len; Try to avoid 'magic numbers' like 55 or 4. Extract them as constants and give them a proper ...


7

The standard signature for an event-handler delegate is void HandlerName(object sender, HandlerArgs args) Where HandlerArgs inherits from System.EventArgs So to follow this convention, you should create a new class: class OrderSubmittedEventArgs : EventArgs { OrderDetails { get; set; } } And then you could theoretically define the event as: /...


7

In chronological order: session.php: If you're into ternaries, you could always change it to: (session_status() != PHP_SESSION_NONE ?: session_start() ); My Ajax is terrible, so I'll have to skip that, sorry. add_to_cart.php: You call two main variables: $error & $success. For $error, instead of storing strings like 'Error', use booleans. $error = ...


6

General You should reconsider the naming of some methods. For example, interface IComparable { public function equals($obj); } is more clear about what it really does (comments are omitted to save space; of course, proper DocBlock comments should always be included with your code). This way, you can easily add other methods like isLessThan, ...


6

You need to add foreign keys that reference the primary keys of the tables they are linked to. I don't usually do a whole lot with building the actual tables but I know that you are missing the foreign keys. In this Table: CREATE TABLE `shipping_type` ( `type_of_shipping` VARCHAR(30) NOT NULL DEFAULT '', `price` DOUBLE(6, 2) NOT NULL, `...


6

I agree with both of your suspected issues: Pointless iteration through @items hash: As you suspected, iterating through all entries of a hash defeats the purpose of a hash. That takes O(I * S) time — the number of items times the number of specials. Why not iterate through @specials, then look up the item by name? That's only O(S). while loop: The loop ...


6

I won't add to @mtj's comment (I totally agree that an array defined as a means to exchange information is a code smell). I agree with @RobAu that you should stay far a away from those magic numbers and arrays. That would avoid bugs like: this.counts[APPLE_INDEX] += other.getCounts()[ORANGE_INDEX]; Now to my comments. Strategy is not the name of the ...


5

I'm not seeing blatant non-OOPness, but there is a little funkiness here: The setters ideally should be usable -- and should be used -- everywhere you import potential user input into the object. Since they validate the data before modifying anything, if you use them consistently, you can ensure the coupon's fields are valid without actually having to ...


5

Based on @nibra's solution Same ICompareable interface. Same ICart interface but derived from IteratorAggregate. interface ICart extends IteratorAggregate {} CartItem class Storing an item. Storing the quantity if the item. final class CartItem { private $_item; private $_quantity; public function __construct(IComparable $item, $quantity = ...


5

Crikey there's alot of duplicated code in there. Before I would even consider reviewing it too closely you might want to think about refactoring all those copy and pasted case statements into one method Remove code that has very likely been "copied and pasted". Oh how I cringe at copy and pasted code: x[a]="iPhone 5 16GB Black "; System.out.println("How ...


5

Security Questions Should I make sessions somehow secure, if there's no authenticated login and sessions are deleted when browser closes? or is session_start() enough in this case? The default should be fine. Most of session security is about server configuration, and for a shopping card you probably don't need stuff like regenerating the session id ...


5

As one of your previous answer-ers, I can say this has definitely improved! I'll work through by the type this time. Variables: Your beginning (block) variables are defined like $firstname, but the secondary block variables are defined like $merchant_secret, with an underline instead of a no-space. I, personally, would suggest using underlines for ...


5

Create a map from card.type to what class it should be: var cardTypeToClass = { 'visa': 'e-visa', 'master-card': 'e-master-card', //... }; This creates an associative array so if you give it a key value then you can get out the associated value. in your case that is done like: if (...


5

Three points which immediately strike out to me: Mutable double array in the interface. Yuck! This is unclear, hard to read and generally leaves a bad feeling. As you only use it to calculate a single value, try to refactor this to method return values and somehow combine the results. A single discount strategy. While this may solve the problem posted in ...


5

You are having a lot of code duplication in the second part, for all the different card vendors. Use the different if blocks just to determine the vendor and store it in a variable, and then do the checking and printing afterwards. number_str = input("Credit Card Number: ") # already str in Python 3 if 12 < len(number_str) < 17 : first2 = ...


4

You should write a thin wrapper around localStorage so that you don't have to constantly ask if (localStorage) every time you want to use it. You can also do that test once, and if it doesn't exist, stub it out with a simple {}: if (!localStorage) { console.log("LocaleStorage is not supported, data will not be persisted") // Let the program use a stub ...


4

Your code looks OK to me, very clear and organised. It's more important to minify your code to decrease load time. There's a whole bunch of minification tools on the Internet.


4

Now I went for nested switch over functions, cause I wanted to try out something new- biggest mistake in my life. I hope you never think that's a good idea again. You could also really use a function to show a menu: int showMenu(String header, String [] options) { ... } This function takes the menu information as arguments, and would return the ...


4

There are two primary concerns I have. First, rather than accessing [[UserManager sharedManager]isUserLoggedIn] twice, let's access it just once: BOOL loggedIn = [[UserManager sharedManager] isUserLoggedIn]; Now just use the loggedIn variable in place of calling these methods. In this case, it's not going to make a huge difference, but it's a good ...


4

There are a few things: Comments like this one: parent::__construct(); // to run the constructor of parent provide no real value; they just repeat what the code does. Prefer reserving comments for tricky bits of code that aren't self-explanatory. You never use $dbLink; get rid of it. If subclasses need a database, let them create one. __destruct is ...


4

I see a number of things that may help you improve your code. Break up the code into smaller functions The addPoints() function is quite long and does a series of identifiable steps. Rather than having everything in one long function, it would be easier to read and maintain if each discrete step were its own function. Eliminate "magic numbers" This code ...


4

It seems the value you're trying to replace for is really just the event.card.type prefixed by e-, so just string concatenate them together. However, if some of the replace variables do not follow this pattern, it'd be best to use @ratchet freak's solution. if (event.isValid) { $("#card-number").addClass(CREDIT_CARD_LIST.replace(CREDIT_CARD_LIST, 'e-' +...


4

You're on the right track, I think. If I had to pick one general criticism, it would be to watch out for redundant special cases. In my opinion, the exercise guides you towards making an unnecessary complication in toDigits and doubleEveryOther. Actually, the list of digits from right to left, as produced by toDigitsRev, is the more natural representation ...


4

First I'd like to start off by saying that if that code suits you, then it's not a bad idea to stick with it. It's fairly clean and pretty neat. I'm just going to point out a few things that may diverge from different types of implementation. Let's talk about the store and update methods. You are injecting a Reqeust class in it, which means your repository ...


4

I think you can refactor the sources as ITaxRateSource like this: public interface ITaxRateSource { List<Taxes> GetTaxRates(Address address); } and implement each one in a separate class: public class DefaultTaxRateSource : ITaxRateSource { public List<Taxes> GetTaxRates(Address address) { ... } } public class ...


4

The code overall looks clean, there will always be debates on writing classes or not (both approaches have great reasoning behind them, of course): Stop Writing Classes (PyCon 2012) Start Writing More Classes (2013) Some things you can improve: the status code handling. Build a mapping between status codes and exception types - this would help to make ...


4

To add to mjt's answer: You never introduce the very sensible object Product (or SKU if you like). Instead, you use String and indices to encode them. Having a class of its own makes it all much more readable. I would skip the magic index values and magic numbers in the ItemCounter. Why not just use a Map<Product, Integer>? This makes the switch() ...


4

First, you should get in the habit of doing early-exits. You have if (cartId != Guid.Empty), followed by a massive block of code. Then an else with a couple simple error-handling lines. The better way to do this, so the error sequence is obvious to any viewer without dissecting the method, is: if (cartId == Guid.Empty) { response.SetUnexpectedError("...


4

Instead of returning false 10+ times, just return false at the end since the only case you'll return true is if the case is 0. Also, you could reduce some repetition by putting the message in a string and printing it at the end instead/ private boolean mapStatus(int status) { String message = status + ": "; switch (status) { case 0: return ...


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