1
\$\begingroup\$

It would be great if I could get this code reviewed. I would love tips on how I could improve my code.

Problem

You receive a credit C at a local store and would like to buy two items. You first walk through the store and create a list L of all available items. From this list you would like to buy two items that add up to the entire value of the credit. The solution you provide will consist of the two integers indicating the positions of the items in your list (smaller number first).

Input

The first line of input gives the number of cases, N. N test cases follow. For each test case there will be:

One line containing the value C, the amount of credit you have at the store.

One line containing the value I, the number of items in the store.

One line containing a space separated list of I integers. Each integer P

indicates the price of an item in the store.

Each test case will have exactly one solution.

Output

For each test case, output one line containing "Case #x: " followed by the indices of the two items whose price adds up to the store credit. The lower index should be output first.

import java.io.File;
import java.io.FileNotFoundException;
import java.util.ArrayList;
import java.util.Scanner;
import java.util.stream.IntStream;

public class StoreCredit {

    private static int case_num = 0;
    private static int num_cases, credit_value, item_cnt;
    private static ArrayList<Integer> prices;
    private static Scanner reader;
    private static ArrayList<String> output;    

    public static void main(String... args) throws FileNotFoundException{
        output = new ArrayList<String>();
        reader = new Scanner(new File("src/store_credit/input.txt"));

        num_cases = reader.nextInt();
        for(int i = 0; i < num_cases; ++i){
            prices = new ArrayList<Integer>();
            credit_value = reader.nextInt();
            item_cnt = reader.nextInt();
            IntStream.range(0, item_cnt).forEach((j) ->  prices.add(j, reader.nextInt()));
            solution();

        }
        for(String s : output)
            System.out.println(s);
    }

    public static void solution(){
        for(int i = 0; i < item_cnt; ++i){
            if(prices.contains(credit_value - prices.get(i))){
                output.add("Case #" + ++case_num + ": " + (i + 1) + " " + (prices.lastIndexOf(credit_value-prices.get(i)) + 1));
                return;
            }
        }
    }
}

Input Information

\$\endgroup\$
  • \$\begingroup\$ Can you add the problem description, and associated test cases? \$\endgroup\$ – h.j.k. Mar 18 '16 at 6:41
  • \$\begingroup\$ @h.j.k. sorry let me do that now \$\endgroup\$ – Ishaan Mar 18 '16 at 6:42
3
\$\begingroup\$

Performance is bad. List.contains() runs in linear time, driving it to quadratic. Recommendation: prepare the list of {price, index} pairs, and stable sort it by prices. The solution is then found in a single scan, along the lines of

    while (head < tail) {
        total = head->price + tail->price;
        if (total < credit) {
            ++head;
        else if total > credit) {
            --tail;
        else {
            return {head->index, tail->index};
        }
    }
\$\endgroup\$
  • \$\begingroup\$ Thanks for pointing that out. Can you please explain how List.contains() drives it to O(n^2) though? Is it because I call solution() for each case then loop again through each item? \$\endgroup\$ – Ishaan Mar 18 '16 at 17:50
  • \$\begingroup\$ @ishyfishy Correct. For each of n elements you call an O(n) method. Total is O(n^2). \$\endgroup\$ – vnp Mar 18 '16 at 17:58
2
\$\begingroup\$

Less static

Your current approach relies entirely on static variables, which may make understanding how the variables are being referenced. In your case, it's still not too hard to figure things out in 30 lines or so, but having 'standalone' methods such as solution(), which takes in no inputs and offers no outputs, is starting to look like some form of code smell. It is a code smell because there's no way to know how it actually works, much less to know to test if its logic is correct.

Interfaces over implementations

It's recommended to declare variables based on their interfaces, not implementations:

// ArrayList<Integer> prices;
List<Integer> prices;

try-with-resources

It's also recommended to use try-with-resources on your Scanner instance for safe and efficient handling of the underlying I/O resource.

\$\endgroup\$
-4
\$\begingroup\$

This is Subset Sum problem. We can solve it in O(CI) time by using Dynamic Programming.

Check this code. Compiles in GCC only.

#include <iostream>
#include <vector>
using namespace std;

int main() {
int Test,CT;
cin>>CT;
Test=CT;
while(Test){
    int sum, items, size;
    cin>>sum;
    cin>>items;

    int item_array[items];

    for(int i=0;i<items;i++){
        cin>>item_array[i];
    }

    cout<<"Case #"<<CT-Test+1<<": ";
    size = sum+1;
    bool T[items][size];
    for (int i = 0; i < items; i++) {
        T[i][0] = true;
    }
    for (int j=1;j<=sum;j++){
        if(j==item_array[0]){
            T[0][j] = true;
        }
        else{
            T[0][j] = false;
        }
    }
    //fill rest of the table
    for (int i = 1; i < items; i++) {
        for (int j = 1; j <= sum; j++) {
            if (j - item_array[i] >= 0) {
                T[i][j] = T[i - 1][j] || T[i - 1][j - item_array[i]];
            } else {
                T[i][j] = T[i-1][j];
            }
        }
    }

    //getting the indices
    int i = items-1;
    int j = sum;
    vector<int> mystack;

    while(j!=0){

    if(i==0){
        mystack.push_back(i+1);
        break;
    }
    else if(T[i-1][j]){
        i=i-1;
    }
    else{
        j=j-item_array[i];
        mystack.push_back(i+1);
    }
    }
    while(!mystack.empty()){
        cout<<mystack.back()<<" ";
        mystack.pop_back();
    }
    cout<<endl;
    Test--;
    }

return 0;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ You have provided an alternative implementation without reviewing the code. On Code Review, all answers should contain a review of the provided code. \$\endgroup\$ – Mast Mar 2 '17 at 10:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.