Should I set a license in the constructor or a static constructor? We are using the Aspose PDF library to work with PDF files. Aspose requires the license to be set once per process. Accordingly, I have created the following common static method that sets the license. Any code that needs Aspose will call into this method.

public class ConfigurationService
{
    private static bool isAsposePdfLicenseSet = false;

    public static void SetAsposePdfLicense()
    {
        if (isAsposePdfLicenseSet)
        {
            return;
        }

        // Set the license... (Moderately expensive - accesses the file system)

        isAsposePdfLicenseSet = true;
    }
}

I need to make sure the license is set before using my class, PdfInspector, but I only need to set it once, so I gave that class a static constructor:

static PdfInspector()
{
    ConfigurationService.SetAsposePdfLicense();
}

However, when I run Code Analysis in Visual Studio, it tells me that static constructors should be avoided. The alternative is to set the license in the constructor:

internal PdfInspector()
{
    ConfigurationService.SetAsposePdfLicense();
}

While this would be called every time the class is instantiated, the SetAsposePdfLicense would short circuit before doing any real logic. Bottom line, which implementation is preferable or is there an alternative that I haven't considered?

This is what made me want to answer:

// Set the license... (Moderately expensive - accesses the file system)

You do not want any constructor, static or not, to do work like this. A constructor that accesses the file system is screaming "I CAN BLOW UP ANYTIME!", and can throw file system -related exceptions. You don't want that in a constructor, even less a static one:

  A static constructor is used to initialize any static data, or to perform a particular action that needs to be performed once only. It is called automatically before the first instance is created or any static members are referenced.

  http://msdn.microsoft.com/en-us/library/k9x6w0hc.aspx

Making your field static ensures that the type (not the instance) has the field value, and your logic ensures it's set only once in the lifetime of your application. If your application isn't starting any process (requirement is once per process, right?), it can probably work.

I don't like that you're assuming that your application will only ever call this method in the same process though - it's a fair assumption to make, but since the requirements say once per process, I think your code should account for System.Diagnostics.Process.GetCurrentProcess().Id, and call the SetAsposePdfLicense() method if the method wasn't called in the process your code is running in.

I'm thinking of a factory living in a Singleton scope (or just as a plain Singleton), that would maintain an ICollection<int> (a HashSet<int> would be ideal for this) where each item is a process ID for which SetAsposePdfLicense was called.

public class PdfInspectorFactory
{
    private readonly ICollection<int> _processes = new HashSet<int>();

    private void SetLicenseForCurrentProcess()
    {
        var currentProcessId = System.Diagnostics.Process.GetCurrentProcess().Id;
        if (_processes.Contains(currentProcessId))
        {
            return;
        }

        // set the license...

        _processes.Add(currentProcessId);
    }

    public PdfInspector Create()
    {
        SetLicenseForCurrentProcess();
        var inspector = new PdfInspector();

        // do whatever you need to do with your inspector before returning it

        return inspector;
    }
}

Then your client code can use this PdfInspectorFactory and call its Create method to get a PdfInspector without having to care about whether or not the license is set.

The client code will not call the factory method in its constructor, rather only when it needs it.

public class SomeClass
{
    private readonly PdfInspectorFactory _factory;

    public SomeClass(PdfInspectorFactory factory)
    {
        _factory = factory;
    }

    public void DoSomething()
    {
        try
        {
            var inspector = _factory.Create();
            // here you go, now do what you need to do with your inspector...

        }
        catch(Exception exception)
        {
            // todo: log or otherwise report exception
            throw;
        }
    }
}

The factory instance should only exist as a Singleton instance - if you're using an IoC container that is very easy to accomplish. Otherwise you need to implement the factory class itself, as a Singleton.

I think you can use a Nullable<bool> or bool? :

private static bool? isAsposePdfLicenseSet = null;
private static bool IsAsposePdfLicenseSet
{
    get
    {
        if (isAsposePdfLicenseSet == null)
            ConfigurationService.SetAsposePdfLicense();

        return isAsposePdfLicenseSet.Value;
    }
}

The validation logic will be executed as soon as you start using IsAsposePdfLicenseSet. Assuming SetAsposePdfLicense will set the value for the backing field, it will only be initialized once.


Creating a simple login with AngularJS I'm brand new to AngularJS. I would like advice as to whether I'm approaching the design of a simple login section of an app built with Angular correctly.

The app consists of two view partials: login.html and user-admin.html. Of what I have so far, the user types their username into the login page. A controller checks if that username is listed in users.json file, if so then login is successful and the user-admin.html partial replaces login.html.

I feel that the controller which checks the typed username against the usernames in users.json could be written better. Is there a more efficient way than using the 'for' statement?

To develop this further I will want to add some way of preventing a logged in user seeing the login page and a non-logged in user seeing the admin page. My initial thought is to use cookies. But would the 'Angular' way be to create a service that perpetuates the logged in status between views? If that is the best way to implement it, what would that service look like?

app.js

angular.module('userApp', ["ngResource"]).

config(['$routeProvider', function($routeProvider) {
    $routeProvider.
        when('/login', {templateUrl: 'partials/login.html',   controller: LoginCtrl}).
        when('/loggedin', {templateUrl: 'partials/user-admin.html', controller: UserCtrl}).
        otherwise({redirectTo: '/login'});
}],[ '$locationProvider', function($locationProvider) {
    $locationProvider.html5Mode = true;
}]).

factory("User", function($resource) {
    return $resource("users/:userId.json", {}, {
        query: {method: "GET", params: {userId: "users"}, isArray: true}
    });
});

controllers.js

function LoginCtrl($scope, $route, $routeParams, $location, User) {
    $scope.users = User.query();
    $scope.loginUser = function() {
        var loggedin = false;
        var totalUsers = $scope.users.length;
        var usernameTyped = $scope.userUsername;

        for( i=0; i < totalUsers; i++ ) {
            if( $scope.users[i].name === usernameTyped ) {
                loggedin = true;
                break;
            }
        }

        if( loggedin === true ) {
            alert("login successful");
            $location.path("/loggedin");
        } else {
            alert("username does not exist")
        }
    }
}

function UserCtrl($scope, $route, $routeParams, $location) {
    $scope.logoutUser = function() {
        $location.path("/login");
    }
}

users.json

[
    {
        "userId": 1,
        "name": "Tommy",
        "password": "123456",
        "log": {
            "registration": "2012.12.14",
            "lastLog": "2013.01.15"
        }
    },
    {
        "userId": 2,
        "name": "Anne",
        "password": "123456",
        "log": {
            "registration": "2012.12.24",
            "lastLog": "2012.12.29"
        }

    },
    {
        "userId": 3,
        "name": "Miles",
        "password": "abc",
        "log": {
            "registration": "2013.02.01",
            "lastLog": "2013.02.01"
        }
    }
]

login.html

Login

    <form ng-submit="loginUser();">
        Username<input type="text" id="userUsername" ng-model="userUsername">
        <input type="submit" value="Login">
    </form>

There is much wrong with your code..

You are iterating over every record to find a username, what happens if you have lots of users ?
You are not verifying the password !?
It seems like you are exposing the REST services with all the user id's and passwords !?
It seems that if the user knows the /loggedin URL, then the user can skip logging in since you do not validate loggedIn anywhere. Even worse, loggedIn is a variable local to LoginCtrl.

It is good to try and write everything yourself, it brings valuable experience. But please, use this only for throwaway websites.

There are lots of mistake with your code:

The database seems to be limited
Your code is not authenticating the username and password


Formatting some data about some airlines function reformat($data, $registration = false) {
    // Initializing aircompanies codes
    $iata = new iata;
    for ($i = 0; $i < count($data); $i = $i + 6) {
        if ($registration) {
            $r = str_replace('<nobr>', '', $this->prep_value($data[$i]));
            $block[$i]['reis'] = str_replace('</nobr>', '', $r);
            $a = explode(' ', $block[$i]['reis']);
            $block[$i]['company'] = $a[0];
        } else {
            $r = explode('&nbsp;', $this->prep_value($data[$i]));
            $block[$i]['reis'] = (strlen($r[0]) > 2) ? strrev(substr(strrev($r[0]), 15, 2)) . ' ' . $r[1] : $r[0] . ' ' . $r[1];
            $block[$i]['company'] = (strlen($r[0]) > 2) ? strrev(substr(strrev($r[0]), 15, 2)) : $r[0];
        }
        $block[$i]['companyname'] = $iata->aircompanies[strtoupper($block[$i]['company'])];
        if (file_exists(ROOT_HTML_PATH . '/content/aircompanies/' . $block[$i]['company'] . '.png')) {
            $block[$i]['company'] = $block[$i]['company'] . '.png';
        } else {
            /**
             * @todo Write e-mail notifications, but only once;
             */
        }
        $block[$i]['airport'] = $this->prep_value($data[$i + 1]);
        $block[$i]['terminal'] = substr(strrev($this->prep_value($data[$i + 2])), 2, 1);
        $block[$i]['shedtime'] = $this->prep_value($data[$i + 3]);
        $block[$i]['facttime'] = str_replace('&nbsp;', '', $this->prep_value($data[$i + 4]));
        $block[$i]['status'] = str_replace('&nbsp', '<br />', trim(strip_tags($this->prep_value($data[$i + 5]), '<img />'), '&nbsp;/r/n'));
        $block[$i]['status'] = str_replace(';', '', $block[$i]['status']);
    }
    return $block;
}

function reformat($data, $registration = false) {
    // Initializing aircompanies codes
    $iata = new iata;
    $block = array();
    for ($i = 0; $i < count($data); $i += 6) {
        $block[$i] = array(
            'airport'  => $this->prep_value($data[$i + 1]),
            'terminal' => substr(strrev($this->prep_value($data[$i + 2])), 2, 1),
            'shedtime' => $this->prep_value($data[$i + 3]),
            'facttime' => str_replace('&nbsp;', '', $this->prep_value($data[$i + 4])),
            'status'   => str_replace(';', '',
                              str_replace('&nbsp', '<br />',
                                  trim(
                                      strip_tags($this->prep_value($data[$i + 5]), '<img />'),
                                      '&nbsp;/r/n'
                                  )
                              )
                          ),
        );

        if ($registration) {
            $r = str_replace('<nobr>', '', $this->prep_value($data[$i]));
            $block[$i]['reis'] = str_replace('</nobr>', '', $r);
            $a = explode(' ', trim($block[$i]['reis']));
            $block[$i]['company'] = $a[0];
        } else {
            $r = explode('&nbsp;', $this->prep_value($data[$i]));
            $block[$i]['company'] = (strlen($r[0]) > 2) ? substr($r[0], -17, 2) : $r[0];
            $block[$i]['reis'] = $block[$i]['company'] . ' ' . $r[1];
        }
        $block[$i]['companyname'] = $iata->aircompanies[strtoupper($block[$i]['company'])];
        if (file_exists(ROOT_HTML_PATH . '/content/aircompanies/' . $block[$i]['company'] . '.png')) {
            $block[$i]['company'] = $block[$i]['company'] . '.png';
        } else {
            /**
             * @todo Write e-mail notifications, but only once;
             */
        }
    }
    return $block;
}

But it is many questions here:

$this->prep_value($data[$i + 2])

It is really bad idea. May be you need a little another cycle, like

for ($i = 0; $i < count($data) - 6; $i ++) { // or something for preventing array index overloading

It would be much better.
But I cannot know it for sure.


Finding the maximum value in a binary tree I have to find the maximum value in a binary tree. This is how I did it iteratively:

int maxValue(Node* node)
{
    if (node == nullptr)
        throw "BT is empty";

    int max = node->data;
    for (Node* left = node; left != nullptr; left = left->left)
    {
        if (left->data > max)
            max = left->data;
    }

    for (Node* right = node; right != nullptr; right = right->right)
    {
        if (right->data > max)
            max = right->data;
    }

    return max;
}

I don't know if this is the best way to do it. How can this be improved? Is there a recursive solution?

Trees are often most useful when they're sorted. If the tree is sorted, you can just descend into the right side of the tree.

Since we're assuming an unsorted tree, we have to search the whole thing. Let's build this up by cases. First assume that the current node has the largest value:

int maxValue(Node *node)
{
    if (node == nullptr)
        throw "BT is empty";

    max = node->data;
    return max;
}

Nice, but not likely. We can do better. What if the largest value is in the left side of the tree?

int maxValue(Node *node)
{
    if (node == nullptr)
        throw "BT is empty";

    max = node->data;

    if(node->left != nullptr) {
        int leftMax = maxValue(node->left);
        if(max < leftMax)
            max = leftMax;
    }

    return max;
}

Great! Now we have a function that will check its left side for larger values, all the way down the left side. But what if the largest value is on the right of some node? We'd better cover that case too:

int maxValue(Node *node)
{
    if (node == nullptr)
        throw "BT is empty";

    int max = node->data;

    if(node->left != nullptr) {
        int leftMax = maxValue(node->left);
        if(max < leftMax)
            max = leftMax;
    }

    if(node->right != nullptr) {
        int rightMax = maxValue(node->right);
        if(max < rightMax)
            max = rightMax;
    }

    return max;
}

Now since we only have  to check for NULL that will throw on the first node we can optimize slightly:

int maxValue(Node *node)
{
    if (node == nullptr)
        throw "BT is empty";

    return maxValueNonNull(node, node->data);
}
int maxValueNonNull(Node* node, int currentMax)
{
    if (node == NULL)
    {    return currentMax;
    }

    currentMax = currentMax > node->data ? currentMax : node->data;

    int leftMax  = maxValueNonNull(node->left,  currentMax);
    int rightMax = maxValueNonNull(node->right, currentMax);

    return leftMax > rightMax ? leftMax : rightMax;
}

That should do it.

why do you search left side of the tree, no need just one for loop enough since you are searching for Max number.


Generating sequence that uses the previous outcome for the current I do not like to use while loops in Ruby. I was wondering how I can generate a "squared sequence" (e.g., squares the first input, then squares the outcome, etc.) in a more idiomatic Ruby way than this one:

value = 2
while value < 10000
    puts value = value**2
end

# => 4
#    16
#    256
#    65536

I have a suspicion that there is a way/method to do this, but I do not know which one. I hope someone can point me in the right direction.

Using while would be idiomatic in almost any language. while is basically the way to iterate, uh, while a condition is true. Hence the name - it's practically plain English.

You can postfix the while and save a couple of lines, but that's about it

puts value = value**2 while value < 10000

or use until if you want, but same difference

puts value = value**2 until value >= 10000

If you were dealing with a fixed or known number of iterations, you could do something like

4.times.inject(2) do |memo, _|
  puts memo = memo ** 2
  memo
end

but the whole point here is really that you don't know the number of iterations.

I see nothing wrong in using the while word since your function logically is doing smth while ...
Wolfram Mathematica has NestWhile, that can be implemented in Ruby like this:

def nest_while f, expr, test
  expr = f[expr] while test[expr]
end

nest_while ->i{ (i**2).tap &method(:puts) }, 2, ->i{ i<10000 }

4
16
256
65536
=> nil

Also you might be interested in implementing NestWhileList: using yield like at SO: Are there something like Python generators in Ruby?.
.


Python code to retry function I am trying to write a python function which retries a given function until given time or function returns True with given delay.

I have written the following function, but was thinking if there is a better way to do it?

Example function I want to retry for

def is_user_exist(username):

    try:
        pwd.getpwnam(username)
        log.info("User %s exist" % username)
        return True
    except KeyError:
        log.exception("User %s does not exist." % username)
        return False

My retry function

def retry(func, *func_args, **kwargs):
    retry_count = kwargs.get("retry_count", 5)
    delay = kwargs.get("delay", 5)
    while retry_count > 1:
        if func(*func_args):
            return
        log.debug("waiting for %s seconds before retyring again")
        sleep(delay)
        retry_count = retry_count - 1

    return func(*func_args)

I like all of Ev. Kounis' answer, so I'll add some higher level details.

Let it be truth-y

Right now you aren't strictly requiring func to return True or False, just something truth-y or false-y. This is fine, and Pythonic. I think you would benefit from pointing that out in a docstring (which you should also write). Additionally, it may be worthwhile to return the actual value of func(*func_args) instead of True or False; this way you can actually get a value from the function if you wanted to take advantage of the truth-yness of the value.

Better kwargs support

You also don't allow any keyword arguments to be passed to the function - the function might support them, so you should to. I would promote the two keyword arguments you pull out of kwargs to explicit keyword arguments, with appropriate defaults (5 in your case).

Exceptions

It's weird to me that this function does have any concept of retrying after exceptions. I wouldn't want you to do something like this, for what I hope are obvious reasons

for _ in range(retry_count):
    try:
        if func(*func_args):
            return True
    except:
        pass
    log.debug("wating for %s seconds before retrying again")
    sleep delay)

However, in many cases I suspect you would know what exceptions you might expect (for example, a network timeout or connectivity blip) and that you might want to be handled by the retrying framework. To this end, I think something like this could be nice:

def retry(func, *args, retry_count=5, delay=5, allowed_exceptions=(), **kwargs):
    for _ in range(retry_count):
        try:
            result = func(*args, **kwargs)
            if result: return result
        except allowed_exceptions as e:
            pass

This obviously isn't a complete implementation; I left out some of the other pieces you have, and it behaves oddly if it fails on the last iteration, but it should be enough to start.

Fancy stuff

I think we could also get more value from this if it was a decorator. Then consumers of a function don't even need to know if they want it to retry or not; they just call their function and see that it works, and whether or not it was retried becomes irrelevant. Don't forget to use functools.wraps to preserve metadata.

import functools

def retry(retry_count=5, delay=5, allowed_exceptions=()):
    def decorator(f):
        @functools.wraps(f)
        def wrapper(*args, **kwargs):
            for _ in range(retry_count):
                # everything in here would be the same

        return wrapper
    return decorator

Then you can enable retrying for everyone, like so:

@retry(retry_count=5, delay=5)
def is_user_exist(username):
    try:
        pwd.getpwnam(username)
        log.info("User %s exist" % username)
        return True
    except KeyError:
        log.exception("User %s does not exist." % username)
        return False

Really fancy stuff

Why block when you're waiting? You could be doing so much more (this is for Python 3.5 and above) using asyncio.  There isn't built-in support for this before that, but I know there are asynchronous frameworks that should be able to accomplish the same task.

By awaiting an asynchronous function that just runs for n seconds, you achieve the same goal but allow other asynchronous functions to do work while you're just waiting. Note that depending on the event loop you might end up waiting for slightly more or less time.

I also cleaned up the issues I mentioned about handling exceptions; it now always returns the result of the function if it has one, and it'll re-raise the exception without losing any traceback if there was one. That also uses a Python 3 only feature; I've left a comment for how to do it in Python 2.

Note, I'm not as familiar with asyncio as I never got to do any serious dev there, so I might not have this piece of code exactly correct; the theory should be sound though.

import functools
import asyncio

def retry(retry_count=5, delay=5, allowed_exceptions=()):
    def decorator(f):
        @functools.wraps(f)
        async def wrapper(*args, **kwargs):
            result = None
            last_exception = None
            for _ in range(retry_count):
                try:
                    result = func(*func_args, **kwargs)
                    if result: return result
                except allowed_exceptions as e:
                    last_exception = e
                log.debug("Waiting for %s seconds before retrying again")
                await asyncio.sleep(delay)

            if last_exception is not None:
                raise type(last_exception) from last_exception
                # Python 2
                # import sys
                # raise type(last_exception), type(last_exception)(last_exception), sys.exc_info()[2]

            return result

        return wrapper
    return decorator

You can replace your while loops and retry_count countdown with a simple for loop via range()

def retry(func, *func_args, **kwargs):
    retry_count = kwargs.get("retry_count", 5)
    delay = kwargs.get("delay", 5)
    for _ in range(retry_count):
        if func(*func_args):
            return
        log.debug("waiting for %s seconds before retyring again")
        sleep(delay)

    return func(*func_args)


Function to return the sum of multiples within a range Project Euler problem 1 says:

  Find the sum of all the multiples of 3 or 5 below 1000.

I have written a program that will return the sum of the multiples of any list of positive integers. So for example if I wanted to know the sum of every integer that was a multiple of 3 and 5 that is less than 10 I would add 3,5,6, and 9 to get 23.

My function can do this for any number of positive integers, between any range that starts from 1.

# Function to return sum of multiples that are within a certain range
def MultiplesSum(multiple, end, start=1):
    mSum = 0
    naturals = range(start, end)
    offset = -start
    if start == 0:
      raise ZeroDivisionError('Cannot start with the number 0')
    for num in multiple:
      for i in naturals:
        if i % num == 0 and naturals[i+offset] != 0:
          # print mSum,"+", i,"=",i+mSum
          mSum += i
          naturals[i+offset] = 0
        i += 1
    return mSum

problem1 = MultiplesSum([3,5,2], 16)
print problem1
# prints 88

Simpler implementation

You don't need naturals at all, and offset either.
You could make the outer loop iterate over the range(start, end),
and for each value check if it can be divided by any one of nums,
and add to the sum.

msum = 0
for i in range(start, end):
    for num in nums:
        if i % num == 0:
            msum += i
            break
return msum

API

The interface of your function is not intuitive:

The range start parameter goes after the range end parameter. That's not intuitive. Consider how Python's range works:

With one arg, the first arg is the end of the range
With two args, the first arg is the start, the second is the end of the range. It's natural that way

I suggest to follow that example
The first arg is called "multiple", singular. It's not a multiple. It's a collection of numbers. So nums would be a better name, and it would make it easier for readers to understand how to use this function

Input validation

In this part:

if start == 0:
  raise ZeroDivisionError('Cannot start with the number 0')

The zero division error is inappropriate. You did not divide by zero,
and it won't make sense for callers to catch this exception and handle it.
In other languages this should be an IllegalArgumentException, in Python ValueError would be more appropriate.
The real problem is not division by zero, but inappropriate use of the function.

But then, why is start=0 inappropriate? I see nothing wrong with that.
What would be wrong is having 0 in nums.
That's what you should validate instead.

Coding style

The recommended practice is to use snake_case for function names, not CamelCase. See more coding style recommendations in PEP8.

I would suggest a simpler solution because Python is 'battery-included':

def multiple_sum(start, end, step):
    return sum(range(start + (start % step), end, step))

This makes use of (fast) built-ins and reads like pseudocode.


Find all pairs in an array that sum to a given number without using HashMap Find all pairs in an array that sum to a given number without using HashMap.
  Duplicate pairs are not allowed. Input array cannot be modified.

  input:   {-2, -1, -1, 5, 7, 7, 7, 7, 8}, target = 7

  output: (-1, 8)

As it is mentioned, using HashMap is not allowed, so I decided to use binary search.

GitHub

public class TwoSumProblemUsingBinarySearch {

    public static class Pair {

        private final int x;
        private final int y;

        public Pair(int x, int y) {
            this.x = x;
            this.y = y;
        }

        @Override
        public int hashCode() {
            return Objects.hash(x, y);
        }

        @Override
        public boolean equals(Object other) {
            if (other instanceof Pair) {
                Pair o = (Pair) other;
                return this.x == o.x && this.y == o.y;
            }

            return false;
        }

        @Override
        public String toString() {
            return String.format("(%d, %d)", x, y);
        }
    }

    public static Set<Pair> findAllParis(int input[], int target) {
        int numbers[] = Arrays.copyOf(input, input.length);
        Set<Pair> pairs = new HashSet<>();

        Arrays.sort(numbers);

        for (int low = 0, high = input.length - 1; low < high; ) {
            int sum = input[low] + input[high];

            if (sum > target) {
                high--;
            } else if (sum < target) {
                low++;
            } else {
                pairs.add(new Pair(input[low], input[high]));
                high--;
                low++;
            }
        }

        return pairs;
    }
}

@Test
public void findAllParis() throws Exception {

    System.out.println(TwoSumProblemUsingBinarySearch.findAllParis(new int[]{-2, -1, -1, 5, 7, 7, 7, 7, 8}, 7));
    assertEquals(1, TwoSumProblemUsingBinarySearch.findAllParis(new int[]{-2, -1, -1, 5, 7, 7, 7, 7, 8}, 7).size());
}

Typo in method name as pointed out by @Stingy. Should be findAllPairs.
Accessing wrong array: You are reading values from input instead of your sorted copy numbers. This is probably a typo. E.g. int sum = input[low] + input[high]; should be int sum = numbers[low] + numbers[high];
Your for loop has an empty update statement, this isn't wrong per-se but a bit unusual. Consider an alternative using while instead:

int low = 0, high = numbers.length-1;
while (low < high) {
    int sum = numbers[low] + numbers[high];
    //...
}

Insufficient test: As evidenced by its failure to catch the bug in 2. the test is too limited to be of much help. Consider checking more than one example (especially corner-cases like an empty input array) and checking against the expected output instead of just its size.

You would also have to add this to the findAllPairs method to sort the initial list which is the input:

Arrays.sort(input);


I am trying to filter a queryset based on dates, I keep getting a duplicate error on the for loops,How do I refactor these code to remove duplicates? I am trying to filter the events table to get ongoing events and complete events, but pycharm keeps underlining my code because of duplicated code. How do I prevent for loop code duplicates on the two functions below?
Better yet, how do I optimally refactor these two functions?
Thanks

def get_ongoing_events():
    ongoing_events = Events.objects.filter(
            Q(event_begin_datetime__lte=current_time),
            Q(event_end_datetime__gt=current_time),
        )
    for event in ongoing_events:
        event.event_status = 'ongoing'
        event.save()

        event.venue.status = 'booked'
        event.venue.save()

        reserve_data = dict()
        reserve_data["sensor_id"] = event.venue.sensor_id
        reserve_data["status"] = event.venue.status

        return reserve_data

def get_complete_reservation():
    """
    Update reservations and sensors
    :return:
    """
    completed_events = Events.objects.filter(
            Q(reservation_begin_datetime__lt=current_time),
            Q(reservation_end_datetime__lte=current_time),
        )

    for event in completed_events:
        event.reservation_status ='complete'
        event.save()

        event.venue.status = 'free'
        event.venue.save()
        reserve_data = dict()
        reserve_data["sensor_id"] = event.venue.sensor_id
        reserve_data["status"] = event.venue.status
        return reserve_data
```

Factoring duplication out
Whenever I have (near-)duplicate code, what I do is take a look at the repeated code. The parts that change ever so slightly are going to be controlled with arguments to functions and etc, whereas the parts that remain the same will just be left more-or-less as-is.
Having said that, here is what I see:

the queries being used change, so those are function arguments;
the attributes of the event being updated change, so those are function arguments;
the final dict being generated is the same, so we leave it the same.

From your code alone, here is a suggested modification:
def filter_and_update_events(Qs, event_changes, venue_changes):
    for event in Events.objects.filter(*Qs)
        for attr, new_value in event_changes.items():
            setattr(event, attr, new_value)
        event.save()
        for attr, new_value in venue_changes.items():
            setattr(event.venue, attr, new_value)
        event.venue.save()

        reserve_data = dict()
        reserve_data["sensor_id"] = event.venue.sensor_id
        reserve_data["status"] = event.venue.status
        return reserve_data

def get_ongoing_events():
    return filter_and_update_events(
        [
            Q(event_begin_datetime__lte=current_time),
            Q(event_end_datetime__gt=current_time),
        ],
        {"event_status": "ongoing"},
        {"status": "booked"},
    )

def get_complete_reservation():
    """
    Update reservations and sensors
    :return:
    """
    return filter_and_update_events(
        [
            Q(reservation_begin_datetime__lt=current_time),
            Q(reservation_end_datetime__lte=current_time),
        ],
        {"reservation_status": "complete"},
        {"status": "free"},
    )

Notice that I used setattr docs to set the attributes of the event and the venue.
Also notice that my new function has a bit of duplication in the loops, but that is the easiest way to deal with the fact that venue is inside event and there is no obvious way to use setattr to deal with the nesting.
If you need to expand your function to update even more things inside event, then I would also recommend creating a helper function that takes an event and a "setting name" and sets it, so that filter_and_update_events doesn't need to take one dictionary per object inside event.
Does this make sense?

although your code has duplication, but it is also modular. Depending on code functionality, you can trade-off this modularity with less duplication.
how about merging the 2 functions and returning two lists like as follows? this will remove code duplication, however, the trade-off would be less code-modularity.
Also, rather than using a dictionary to store booked and free sensor ids, you can just use lists with descriptive names as I have done.
def get_booked_and_free_events():
ongoing_events = Events.objects.filter(
        Q(event_begin_datetime__lte=current_time),
        Q(event_end_datetime__gt=current_time),
    )
completed_events = Events.objects.filter(
        Q(reservation_begin_datetime__lt=current_time),
        Q(reservation_end_datetime__lte=current_time),
    )
for event in ongoing_events:
    event.event_status = 'ongoing'
    event.save()

    event.venue.status = 'booked'
    event.venue.save()

    booked_sensors = []
    booked_sensors.append(event.venue.sensor_id)

for event in completed_events:
    event.reservation_status ='complete'
    event.save()

    event.venue.status = 'free'
    event.venue.save()

    free_sensors = []
    free_sensors.append(event.venue.sensor_id)

return booked_sensors, free_sensors


Funny String Java Solution I'm a little rusty in Java, but I'm using it to get ready for interviews.  Here's the problem description from HackerRank:

  Problem Statement

  Suppose you have a string \$S\$ which has length \$N\$ and is indexed from \$0\$
  to \$N−1\$. String \$R\$ is the reverse of the string \$S\$. The string \$S\$ is funny
  if the condition \$|S_i−S_{i−1}|=|R_i−R_{i−1}|\$ is true for every \$i\$ from 1 to
  \$N−1\$.

  (Note: Given a string str, stri denotes the ascii value of the ith
  character (0-indexed) of str. |x| denotes the absolute value of an
  integer x)

  Input Format

  First line of the input will contain an integer T. T testcases follow.
  Each of the next T lines contains one string S.

  Constraints

  \$1 \le T \le 10\$
  \$2 \le\$ length of S \$\le 10000\$

  Output Format

  For each string, print Funny or Not Funny in separate lines.

This passing solution took me about 20 minutes, so that might be a bit long given the difficulty of the problem.  I'm open to critiques on my speed too.

import java.io.*;
import java.util.*;
import java.text.*;
import java.math.*;
import java.util.regex.*;

public class Solution {
    private static boolean isFunny (String s)
    {
        String rev = reverse(s);
        boolean stillEq = true;
        for (int i = 2; i < s.length() && stillEq; ++i)
        {
            int comp = (int)s.charAt(i) - (int)s.charAt(i-1);
            int comp2 = (int)rev.charAt(i) - (int)rev.charAt(i-1);
            stillEq = Math.abs(comp) == Math.abs(comp2);
        }

        if (stillEq)
            return true;
        else
            return false;
    }

    private static String reverse (String s)
    {
        if (s.length() > 0)
            return s.charAt(s.length()-1) + reverse(s.substring(0, s.length()-1));
        else
            return "";
    }

    public static void main(String[] args) {
        Scanner sc = new Scanner(System.in);
        int tests = sc.nextInt();
        for (int i = 0; i < tests; ++i)
        {
            String out = isFunny(sc.next()) ? "Funny" : "Not Funny";
            System.out.println( out );
        }
    }

Bug

The main loop starts at i = 2. That is, s.charAt(1) - s.charAt(0) is never attended to. Suspicious, isn't it?
Naming

A name comp (and comp2) presumes that it carries some information about comparison. It obviously doesn't. diff and rdiff sound better.
Algorithm

Reversal of the string just wastes time. You may work the same string from both directions simultaneously:

for (int i = 1; i < s.length(); ++i) {
    diff = s.charAt(i) - s.charAt(i - 1);
    rdiff = s.charAt(s.length() - 1 - i) - s.charAt(s.length() - 1 - (i-1));
    ...

Returning the comparison result is an anti-pattern.

return stillEq;

achieves the same result as

if (stillEq)
    return true;
else
    return false;

in a much cleaner way.
If you insist on reversing a string, better come up with an iterative method. Java cannot eliminate tail recursion.

Learning from the answers above can the code be optimized further as

// Complete the funnyString function below

        static String funnyString(String s) {
        String rev = reverse(s);
        boolean stillEq = true;
        for (int i = 2; i < s.length() && stillEq; ++i)
        {
            int comp = (int)s.charAt(i) - (int)s.charAt(i-1);
            int comp2 = (int)rev.charAt(i) - (int)rev.charAt(i-1);
            stillEq = Math.abs(comp) == Math.abs(comp2);
        }
        if (stillEq) {
            return "Funny";
        }
        else{
            return "Not Funny";
        }
        }

        private static String reverse (String s)
        {
            if (s.length() > 0)
                return new StringBuilder(s).reverse().toString();
            else
                return "";
        }


Determine if an array is the reverse of a second array I tried to do it by using ArrayUtils.reverse() but I couldn't figure out how to compare this with the other array. So I wrote this instead:

public class Main {

  public static void main(String[] args) {

int[] array = new int[] {1, 2, 3, 4, 5, 6};

int[] tempArray = new int[] {6, 5, 4, 3, 2, 1};

boolean result = true;

for (int i = 0; i < array.length / 2; i++) {
  int temp = array[i];
  array[i] = array[array.length - i - 1];
  array[array.length - i - 1] = temp;
}

for (int i = 0; i < array.length; i++) {
  if (array[i] != tempArray[i]) {
    System.out.println("nope,not reversed");
    result = false;
    break;
  }
}

if (result) {
  System.out.println("yep,reversed");
}
  }
}

Is there a simpler solution? I'm skeptic because I think breaking the loop and setting a boolean value is a bit unnecessary.

The first comment, is that it would be preferable to factor this into a dedicated method, like areReversed(int[] array1, int[] array2). It would clarify the intention of the code. And, as it turns out, this will point out some improvements about the method itself.

It is currently modifying in-place the first array, in order to reverse it. If you now consider the code to be in a method, you could have

int[] array1 = new int[] { 1, 2, 3, 4, 5, 6 };
int[] array2 = new int[] { 6, 5, 4, 3, 2, 1 };
areReversed(array1, array2); // OK, prints that they are reversed versions
// some code
areReversed(array1, array2); // What? Now it prints that they aren't reversed?!

Calling the method a second time with the same arguments would not have the same result. This would be very surprising, especially so that one doesn't expect that method to behave in such a way. It is always preferable to adhere to the principle of least astonishment.
We want to know whether the two arrays are reversed, and we may want to do more than printing a String if it is the case. Make the method return a boolean that the calling code can use.

The insight is that you do not need to reverse an array, and determine whether the two are equal, for this. First of all, if they don't have the same length, there is no need to do anything, it is already known for sure that the result is false. Then, you can directly compare the arrays, by traversing one in a given direction and the other one in the opposite direction.

B   A   C            B   A   C            B   A   C
|                        |                        |
----x----                x                ----x----
        |                |                |
C   A   B            C   A   B            C   A   B

 Step 1               Step 2                Step 3

The upper part goes from left to right, and it compares elements with the lower part going from right to left. If there's a mismatch at any point, we know the arrays are not reverse of each other. And since we know they have the same length, all element of both arrays will have been considered.

A simple code to do this would be

public boolean areReversed(int[] array1, int[] array2) {
    if (array1.length != array2.length) {
        return false;
    }
    int length = array1.length;
    for (int i = 0; i < length; i++) {
        if (array1[i] != array2[length - i - 1]) {
            return false;
        }
    }
    return true;
}

That you can later use with:

if (areReversed(array, tempArray)) {
    System.out.println("yep,reversed");
} else {
    System.out.println("nope,not reversed");
}

You'll note that this implementation somewhat takes care of the concern you had with the code: "breaking the loop and setting a boolean value". In fact, there is no boolean variable: we return directly from the method once we know for sure what to return. The return inside the loop is really for optimization purposes; once a mismatch is encountered, there is no need to process further the arrays, since it is known that the result can only be false at that point. This is also what the initial break was doing.

Small final note. Instead of

int[] array = new int[] { 1, 2, 3, 4, 5, 6 };
int[] tempArray = new int[] { 6, 5, 4, 3, 2, 1 };

you could just have

int[] array = { 1, 2, 3, 4, 5, 6 };
int[] tempArray = { 6, 5, 4, 3, 2, 1 };

which is a bit more concise, and makes the array declaration a tad more readable.

We will use your same idea, just simplify your code.

In your first loop you do

int temp = array[i];
array[i] = array[array.length - i - 1];   //1
array[array.length - i - 1] = temp;     //2

And then, in the second loop

if (array[i] != tempArray[i])

But by //1 and //2 we can omit the first loop and in the second we change array[i] by array[array.length - i - 1], so we have

boolean result = true;
s = array.length;
if( tempArray.length != s)
    result = false;
i = 0;
while (i < tam/2 and result != false )
    if (array[i] != tempArray[tam-i-1])
        result = false;
if ( result )
    System.out.println("yep, reversed");
else
    System.out.println("nope,not reversed");


Check for Palindrome string in Java I was wondering if you can give me advice on this solution to assess if an user input string is a palindrome or not.
How can I optimize this solution?

import java.io.InputStreamReader;
import java.io.BufferedReader;
import java.io.IOException;

public class Palindrome {
public static void main(String[] args)
    throws IOException {

    // user enters string
    System.out.println ("Please enter string");

    InputStreamReader input = new InputStreamReader (System.in);
    BufferedReader reader = new BufferedReader (input);
    String userinput = "";
    userinput = reader.readLine();

    // test if string is valid
        if (!userinput.matches("[A-Za-z]+")){
            System.out.println ("Invalid Input");
            System.exit(1);
        }

        else if (userinput.length() > 10){
            System.out.println ("String too long");
            System.exit(2);
        }

    String stringReverse = "";
    for (int i = userinput.length()-1; i >= 0; i--){
        stringReverse += String.valueOf(userinput.charAt(i));
    }

    if (userinput.equals(stringReverse)){
        System.out.println ("String is a Palindrome");
        System.exit(0);
    }

    else if (userinput != stringReverse) {
        System.out.println ("String is not a Palindrome");
        System.exit(0);
    }

    }
}

When programming a solution like this, in order to simplify the problem create a separate function for each component. So, I would break it up into three different parts:

String getUserInput() {
    // Get input string and return it...
}

boolean isValidInput(String input) {
    // Return true or false based off of the validity of the input string.
}

boolean isPalindrome(String input) {
    int left = 0;
    int right = input.length();
    while(left < right) {
        if(input.charAt(left++) != input.charAt(--right)) return false;
    }
    return true;
}

The reason for returning a boolean rather than throwing an exception is that a try catch can be expensive especially if there is a good chance of an error being thrown. Now, a boolean means later that there may be some sort of conditional branch, but that is less expensive than if there is a high chance of an error being thrown. Also, there is more usability with returning a boolean, because now you can decide if you want to throw an exception or just print true or false.

Since the question was more geared towards the optimization of the palindrome detection algorithm, I provided a solution that I have found to work the best for when detecting palindromic inputs.

The solution cuts the number of steps down to a quarter of your solution. This is done by assuming that if the first half is the same as the second half then the String must be palindrome. So, we start from both ends and work our way to the center (essentially when left reaches or passes right). If any where along the way we find a point in which is not equal, then we know that we do not have a palindromic String. Therein, we can exit returning false. If we successfully exit the loop, then we know we were given a palindromic String. Therein, we return false.

Do not use System.exit() (somewhere else than in a chatch block inside main) this terminates the JVM and this is not what you want in most cases.

  // user enters string
  // test if string is valid

This type of comment tells you that the code below sould be placed in a method with a name derived from the comment. eg:

String userEntersString(){
  // ...
}

void testIfStringIsValid(String string){

}

then your main would read like this:

public static void main(String[] argd){
   String string userEntersString();
   testIfStringIsValid(string);
   testIfStringIsPalindrome(string);
   System.out.println ("String is a Palindrome");
}

To handle the validation you should have your validation method throwing Exceptions:

void testIfStringIsValid(String string) throws Exception {
    if (!userinput.matches("[A-Za-z]+")){
        throw new Exception("Invalid Input");
    }
   //...
}

then you main would change like this:

public static void main(String[] argd){
   String string userEntersString();
   try{
       testIfStringIsValid(string);
       testIfStringIsPalindrome(string);
       System.out.println ("String is a Palindrome");
   } (catch Exception e) {
       System.err.println(e.getMessage());
       // if you really need to communicate the programms result:
       System.exit(1);
   }
}


Shortening boolean with if statement for document version comparison Can this be shortened to one line?

 bool areVersionsEqual = documentComparer.Equals(version, imported);

if (areVersionsEqual == false)
  {
    changedDocument.Add(imported);
   }

Sure. Use the Not operator.

if (!documentComparer.Equals(version, imported))
{
    changedDocument.Add(imported);
}

To explain a little, it's bad practice to do things like this.

bool boolean = true;
if (boolean == false)
{
//...

because it's completely equivalent to saying

if (true == false)

and can be simplified down to

if (false)

You could try a ternary operator

documentComparer.Equals(version, imported) == false ?  changedDocument.Add(imported) : break;

crappy? Yes


1D Peak Finder Algorithm I'm reviewing MIT Introduction to Algorithm lectures/exercises and am trying to implement a one dimensional peak finder algorithm.

I've got a working copy but it's a bit messy and I've had to put some array size constraints to get it working properly. How can I improve this implementation?

def peakfinder(arr):
    if len(arr) == 0: # If list is 0, there is no peak.
        return None

    if len(arr) == 1: # If list has been reduced to 1 element, it's a peak.
        return arr

    mid = len(arr) / 2

    left  = mid - 1
    right = mid + 1

    if arr[left] <= arr[mid] >= arr[right]:
        return arr[mid] # Base case. This is a peak.

    if arr[mid] < arr[left]: # Look to left side of array for peak.
        return peakfinder(arr[:mid])

    if arr[mid] < arr[right]: # Look to right side of array for peak.
        return peakfinder(arr[mid+1:])

Crashing bug

If I call peakfinder([0, 0]), I get an IndexError:

Traceback (most recent call last):
  File "peaks.py", line 32, in <module>
    peakfinder([0, 0])
  File "peaks.py", line 22, in peakfinder
    if arr[left] <= arr[mid] >= arr[right]:
IndexError: list index out of range

(I’m not sure if these are the “size constraints” you allude to in the question, you didn’t provide details.)

If we inspect these, we discover that we have (L, M, R) = (0, 1, 2), but trying to get the element at index 2 will fail. This will fail whenever you have an array of length 2.

One way to handle this would be to add an extra base case for when you have an array of length 2. Alternatively, you could tweak the bounds checking so that it only looks for elements within the bounds of the array.

Let’s suppose we never pass in an array of size 2. But it turns out other arrays will hit the same, if they reduce to an length-2 array at an intermediate step. For example:

>>> peakfinder([0, 1, 0, 0])
Traceback (most recent call last):
  File "peaks.py", line 35, in <module>
    peakfinder([0, 1, 0, 0])
  File "peaks.py", line 29, in peakfinder
    return peakfinder(arr[:mid])
  File "peaks.py", line 31, in peakfinder
    if arr[mid] < arr[right]: # Look to right side of array for peak.
IndexError: list index out of range

Here’s a small test I wrote with Hypothesis to find some counterexamples:

from hypothesis import given, strategies as st

@given(st.lists(st.integers()))
def test_peakfinder_doesnt_crash(xs):
    """Finding the peak of a list doesn't crash."""
    peakfinder(xs)

The peak of an array must be an element of the array

Sounds reasonable enough, right? But your code will return a list of length 1 if any of the intermediate steps reduce to such a list, an element otherwise. For example:

>>> from peaks import peakfinder
>>> peakfinder([1, 0])
[1]
>>> [1] in [1, 0]
False
>>> peakfinder([1, 2, 3])
2
>>> 2 in [1, 2, 3]
True

You should tweak the case when len(arr) == 1 to return the single element:

if len(arr) == 1:
    return arr[0]

Here’s another small test I wrote to find a counterexample:

from hypothesis import assume, given, strategies as st

@given(st.lists(st.integers()))
def test_peak_is_in_original_list(xs):
    """The peak of a list is in the list."""
    assume(len(xs) > 0)
    assert peakfinder(xs) in xs

The code should work fine once the issues mentioned by @alexwlchan are fixed, I added those changes and code looks like this:

def peakfinder(arr):
    if len(arr) == 0: # If list is 0, there is no peak.
        return None

    if len(arr) == 1: # If list has been reduced to 1 element, it's a peak.
        return arr[0]

    if len(arr) == 2:
        if arr[1]>= arr[0]:
            return arr[1]
        else:
            return arr[0]

    mid = len(arr) // 2

    left  = mid - 1
    right = mid + 1

    if arr[left] <= arr[mid] >= arr[right]:
        return arr[mid] # Base case. This is a peak.

    if arr[mid] < arr[left]: # Look to left side of array for peak.
        return peakfinder(arr[:mid])

    if arr[mid] < arr[right]: # Look to right side of array for peak.
        return peakfinder(arr[mid+1:])

Now as the question mentioned how it can be improved, I went further and modified it to not use recursion to improve on time. The non-recursive code is:

 def peakfinder_norecursive(arr):
    if len(arr) == 0: # If list is 0, there is no peak.
        return None

    elif len(arr) == 1: # If list has been reduced to 1 element, it's a peak.
        return arr[0]

    else:
        left  = 0
        right = len(arr)-1
        while(right-left >1):
            mid = left + (right - left)//2

            if arr[mid-1] <= arr[mid] >= arr[mid+1]:
                return arr[mid] # Base case. This is a peak.

            elif arr[mid] < arr[mid-1]: # Look to left side of array for peak.
                right = mid-1

            elif arr[mid] < arr[mid+1]: # Look to right side of array for peak.
                left = mid+1
        if arr[right]>= arr[left]:
            return arr[right]
        else:
            return arr[left]

To show the difference took a list with large n (such that last element is the peak, worst case scenario) and executed both the solutions, here are the timings-

if __name__ == "__main__":

    A = []
    for i in range(1,9999999):
        A.append(i)

    start = time.time()
    peak = peakfinder(A)
    end = time.time()
    print(end - start)
    print("Recursive Method : Peak : {}\n".format(peak))

    start = time.time()
    peak = peakfinder_norecursive(A)
    end = time.time()
    print(end - start)
    print("Non Recursive Method: Peak : {}\n".format(peak))

Output:
0.0650475025177002
Recursive Method : Peak : 9999998

0.0
Non Recursive Method: Peak : 9999998


Python Knapsack greedy Given a dictionary of cows and their weight:

cows = {'Betsy': 9, 'Oreo': 6, 'Herman': 7, 'Florence': 2, 'Maggie': 3, 'Moo Moo': 3, 'Milkshake': 2, 'Lola': 2, 'Millie': 5, 'Henrietta': 9}

I want to get a list of lists to move all cows, each nested list of cow's a total weight <= 10, hence to first two trips will have just Betsy and Henrietta:

The answer for GREEDY COW TRANSPORT:

[['Betsy'], ['Henrietta'], ['Herman', 'Maggie'], ['Oreo', 'Moo Moo'], ['Millie', 'Florence', 'Milkshake'], ['Lola']]

Here is my code that took too long in a net grader:

def greedy_cow_transport(cows,limit=10):
    train = []
    cart = []

# get tuples from dictionary key ordered by value high to low
    CowTupleList = sorted(cows.items(), key=lambda x: x[1], reverse = True)

# put cow names in list order high to low to loop over
    names = []
    for i in range(len(cows)):
        names.append(CowTupleList[i][0])
    train = []
    while sum(cows.values()) > 0:
        cart = []
        total = 0

        for cow in names:
            if cows[cow] != 0 and cows[cow] + total <=limit:
                cart.append(cow)
                total += cows[cow]
                cows[cow] = 0
        train.append(cart)
    return train

This took too much time for the online grader, so I failed. Can this be done in only a few lines?

You sort the cows, but you don't take advantage of the fact that they're sorted.  Instead of iterating over the cows multiple times (which leads to \$\mathcal{O}(n^2)\$ time w.r.t. the number of cows), iterate over the sorted list once.

Unfortunately, I can't think of an easy way to do this using Python's built-in data structures.  However, if we assume that CowTupleList is some list-like datastructure that has \$\mathcal{O}(\log{n})\$ or better performance for all operations (including del), then we can use binary search to find the largest cow that will fit in a cart's remaining capacity:

def greedy_cow_transport(cows,limit=10):
  # Sort cows from largest to smallest
  CowTupleList = sorted(cows.items(), key=lambda x: x[1], reverse = True)

  while CowTupleList:
    # Add first (largest) cow to a new cart
    name,weight = CowTupleList[0]
    cart = [name]
    remaining_capacity = limit - weight

    # Remove first cow from list
    del CowTupleList[0]

    # Find largest remaining cow that fits in remaining capacity (if any)
    idx = find_largest_fitting(CowTupleList, remaining_capacity)
    while idx is not None:
      # Add the cow at idx to the cart
      name,weight = CowTupleList[idx]
      cart.append(name)
      remaining_capacity -= weight

      # Remove cow at idx from list
      del CowTupleList[idx]

      # Find largest remaining cow that fits (if any)
      idx = find_largest_fitting(CowTupleList, remaining_capacity)

    # No more cows fit => yield the cart
    yield cart

Assuming find_largest_fitting is implemented as a binary search over CowTupleList (and an appropriate data structure is chosen for CowTupleList), this should take \$\mathcal{O}(n\log{n})\$ time.  If linear search is used for find_largest_fitting and/or Python's build-in list type is used for CowTupleList (so that del operates in \$\mathcal{O}(n)\$), then this algorithm will operate in \$\mathcal{O}(n^2)\$ time.

I'll give it a shot too.

I wrote various iterations of the code for this problem and the one I'm the most happy about (and that is easy to read and understand) is the following where I use a while-loop but start the counter at one less than the length and decrease the value each time in order to not go through the list of cows more than once.

def greedy_cow_transport_third_iteration(cows, limit=10):
trips, available = [], limit
# Make a list of cows, sort by weight in ascending fashion (lightest first)
cows = sorted([(weight, name) for name, weight in cows.items()])
while cows:                     # Loop through available cows
    trips.append([cows[-1][1]]) # Allocate heaviest cow on a new trip
    available -= cows[-1][0]    # Adjust available weight accordingly
    cows.remove(cows[-1])       # Remove this cow from future loops
    i = len(cows) - 1           # Reset index
    while i >= 0:               # Loop back through all remaiing cows
        if available > 0 and cows[i][0] <= available:
            trips[-1].append(cows[i][1])  # Allocate this cow!
            available -= cows[i][0]       # adjust available weight
            cows.remove(cows[i])          # Remove it from future loops
        i -= 1                  # Avoid infinite loops
    available = limit           # Reset available weight for next trip
return trips                    # Returns solution

I hope that helps somehow.


Most common words in a text file of about 1.1 million words I'm learning programming with Python.

I’ve written the code below for finding the most common words in a text file that has about 1.1 million words. It is working fine, but I believe there is always room for improvement.

When run, the function in the script gets a text file from the command-line argument sys.argv[1], opens the file in read mode, converts the text to lowercase, makes a list of words from the text after removing any whitespaces or empty strings, and stores the list elements as dictionary keys and values in a collections.Counter object. Finally, it returns a dictionary of the most common words and their counts. The words.most_common() method gets its argument from the optional top parameter.

import sys
import collections

def find_most_common_words(textfile, top=10):
    """ Return the most common words in a text file. """

    textfile = open(textfile)
    text = textfile.read().lower()
    textfile.close()
    words = collections.Counter(text.split()) # how often each word appears

    return dict(words.most_common(top))

filename = sys.argv[1]
top_five_words = find_most_common_words(filename, 5)

This is actually quite good. Good use of the collections module.

One improvement I can think of is switching to the with open(...) as: structure, that way the file will automatically be closed when finished processing.
Secondly it is standard in python to use a if __name__ == '__main__': block.
Lastly sys.argv will work ok for programs with not many arguments, but I'd recommend looking at the argparse module.

I'd read a select amount of the file at a time. Split it into characters, and then split on each empty space. This is better than splitting on each new line as the file may be one line.

To do the former in Python 3 is fairly simple:

def read_chunks(file, chunk_size):
    while True:
        chunk = file.read(chunk_size)
        if not chunk:
            break
        yield from chunk

This has \$O(\text{chunk_size})\$ memory usage, which is \$O(1)\$ as it's a constant. It also correctly ends the iterator, when the file ends.

After this, you want to split the words up. Since we're using str.split without any arguments, we should write just that method of splitting. We can use a fairly simple algorithm:

from string import whitespace

def split_whitespace(it):
    chunk = []
    for char in it:
        if char not in whitespace:
            chunk.append(char)
        elif chunk:
            yield tuple(chunk)
            chunk = []
    if chunk:
        yield tuple(chunk)

This has \$O(k)\$ memory, where \$k\$ is the size of the largest word. What we'd expect of a splitting function.

Finally we'd change from tuples to strings, using ''.join, and then use the collections.Counter. And split the word reading, and finding the most common into two different functions.

And so for an \$O(k)\$ memory usage version of your code, I'd use:

import sys
from collections import Counter
from string import whitespace

def read_chunks(file, chunk_size):
    while True:
        chunk = file.read(chunk_size)
        if not chunk:
            break
        yield from chunk

def split_whitespace(it):
    chunk = []
    for char in it:
        if char not in whitespace:
            chunk.append(char)
        elif chunk:
            yield tuple(chunk)
            chunk = []
    if chunk:
        yield tuple(chunk)

def read_words(path, chunk_size=1024):
    with open(path) as f:
        chars = read_chunks(f, chunk_size)
        tuple_words = split_whitespace(chars)
        yield from map(''.join, tuple_words)

def most_common_words(words, top=10):
    return dict(Counter(words).most_common(top))

if __name__ == '__main__':
    words = read_words(sys.argv[1])
    top_five_words = most_common_words(words, 5)


Google Code Jam: Store Credit Java Solution 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

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};
        }
    }

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;
}


Find the minimum number of coin change Description:
You are given coins of different denominations and a total amount of money amount. Write a function to compute the fewest number of coins that you need to make up that amount. If that amount of money cannot be made up by any combination of the coins, return -1.

coins = [1, 2, 5], amount = 11, return 3 (11 = 5 + 5 + 1)
coins = [2], amount = 3, return -1.

Code:
class Solution {
    public int coinChange(int[] coins, int amount) {
        if (amount == 0) return 0;
        if (amount <  0) return -1;

        int min = -1;
        for (int coin : coins) {
            int currentMin = coinChange(coins, amount - coin);

            // if amount is less than coin value
            if (currentMin >= 0) {
                min = min < 0 ? currentMin : Math.min(currentMin, min);
            }
        }

        return min < 0 ? -1 : min + 1;
    }
}

Questions:
This question has slight deviation from normal one where we just need to return 0, returning -1 got me confused and I found it really hard to think in terms of recursion and handling edge cases.
PS: The solution works perfectly.

I think part of your problem is, you aren't taking the recursion far enough.  I would suggest, that the loop be replaced by recursion.  This can be accomplished quite easily by making sure the array is sorted first, and advancing from highest to lowest.  Since this will require extra parameters and the addition of calling the sort function.  Making the recursive function private and using a  public helper overload, will neaten things quite a bit.  Something like this should work:

public class Solution
{

    public static int coinChange(int[] coins, int amount)
    {
        Arrays.sort(coins);
        return coinChange(coins, amount, coins.length - 1, 0);
    }

    private static int coinChange(int[] coins, int amount, int index, int numCoins)
    {
        if (amount == 0)
        {
            return numCoins;
        }
        if (amount < coins[0])
        {
            return -1;
        }
        if (amount >= coins[index])
        {
            int tempCoins = (int) (amount / coins[index]);
            int tempAmount = amount - (tempCoins * coins[index]);
            if (tempAmount == 0 || tempAmount >= coins[0])
            {
                amount = tempAmount;
                numCoins += tempCoins;
            }
        }
        index--;

        return coinChange(coins, amount, index, numCoins);
    }
}

The usage would still be the same(int numCoins = Solution.coinChange(new int[]{1,2,5},11);).

Your description of the problem didn't mention whether the array would be sorted.  If it is guaranteed to be sorted that line can be left out.

You'll notice that there are still 3 states to check for, but that each one is only checked for once per recursion.

I didn't like the idea of making the amount go below 0.  Simply stopping when the amount was either 0 or less than the smallest coin, seems a cleaner way of doing things, to me.

class Solution {
    HashMap<String, Integer> memo = new HashMap<String, Integer>();
    public int solve(int[] coins, int amount, int startFrom) {
        //0$left, so we can reach 1 way
        if(amount == 0) return 1;
        //already -ve amount means this combination wont work
        if(startFrom < 0 || amount < 0) return 0;
        //Moving right to left, map key where starting and how much left
        int exist = memo.getOrDefault(startFrom+"#"+amount, -1);
        //If already computed dont recompute
        if(exist >= 0) return exist;
        //If we take this, remaining amount adjusted,DONT move left
        // so that we can take this again
        int taken =  solve(coins, amount-coins[startFrom], startFrom );
        //If we dont take this remaining amount stays same but move to left
        int notTaken = solve(coins, amount, startFrom -1 );
        int memoOnStartAndAmount = taken + notTaken;
        memo.put(startFrom+"#"+amount, memoOnStartAndAmount);
        return memoOnStartAndAmount;
    }
    public int change(int amount, int[] coins) {
        return solve(coins, amount, coins.length -1 ) ;//move right to left
    }
}

Here is a more Easy to understand 9 line Solution


Groovy: find all elements in a list equal to the max element Is there any simpler way to find all elements in a list that are equal to the max element.

List v = [ 1,2,3,4,5,5  ]
def max = v.max()
def maxs = v.findAll { it == max }

Thanks!

How you've done it for the simple example is exactly how I would do it.  I may use groupBy if I was dealing with a more complex object.

    List v = [ 1,2,3,4,5,5 ]
    def max = v.max()
    def results = v.groupBy {it}.get(max)
    assert [5,5] == results

It is a shame that Groovy makes it diffuclt for me to find or check element from a list of integers instead of using

      sess.find(valuetobefound)

than

     sess.find{it==valuetobefound}


count all array[index] == index occurrences The method foo gets a sorted list with different numbers as a parameter and returns the count of all the occurrences such that: i == list[i] (where i is the index 0 <= i <= len(list)).

def foo_helper(lst, start, end):

    if start > end:
        # end of recursion
        return 0

    if lst[end] < end or lst[start] > start:
        # no point checking this part of the list
        return 0

    # all indexes must be equal to their values
    if abs(end - start) == lst[end] - lst[start]:
        return end - start + 1

    middle = (end + start) // 2

    print(lst[start:end+1], start, middle, end)

    if lst[middle] == middle:
        #print("lst[" , middle , "]=", lst[middle])
        return 1 + foo_helper(lst, middle+1, end) + \
    foo_helper(lst, start, middle-1)

    elif lst[middle] < middle:
        return foo_helper(lst, middle+1, end)
    else:
        return foo_helper(lst, start, middle-1)

def foo(lst):
    return foo_helper(lst, 0, len(lst)-1)

My question is: is this code's worst-case complexity log(n)?  If not, what should I do differently?

Now let's see if I understand your problem: Given a sorted list of unique numbers you want to find all instances where list[i] == i (the important part is the unique).

This means each number in the list is at least 1 larger than the previous number.
Assume that list[k] == x < k. Because the numbers are strictly increasing it must be that list[k-1] <= x-1 < k-1, list[k-2] <= x-2 < k-2, ... list[k-l] <= x-l < k-l. So if you have an index where the value is smaller than the index then this must also be the case for all previous indices.
Assume that list[k] == x > k. Because the numbers are strictly increasing it must be that list[k+1] >= x+1 > k+1, list[k+2] >= x+2 > k+2, ... list[k+l] >= x+l > k+l. So if you have an index where the value is larger than the index then this must also be the case for all following indices.

The conclusion from these points is that a list with the given properties can be divided into three parts

Start 0 <= s < ms with list[s] < s
Middle ms <= m < me with list[m] == m
End me <= e < n with list[e] > e

Your problem is to find the middle part. The number of items where list[i] == i is then me - ms.

This can be solved by finding the largest index for which list[k] < k and the smallest index for which list[k] > k (or the smallest and largest indices for which list[k] == k - depends on how you want to write the search criteria).

Using two binary searches should yield the desired results which will guarantee a worst case complexity of O(log(n))

Update: Your original implementation is effectively a binary search which will find both end and start point of the middle sequence so you implementation is O(log(n))

I don't know enough Python to comment on your code, but returning a count of where array[index] == index, should be O(n). And as I said, I dunno Python, but it looks like you are making this a lot more complicated than it is. It could be as simple as

function foo(array) {
    int count = 0;
    for(int i = 0; i < array.length; i++) {
        if(array[i] == i) count++;
    }
    return count;
}

Or maybe I just don't understand what you're trying to accomplish...


"P1 attacks P2" and "P2 attacks P1" This is just my simple structure. I want to know if there is a better way than this because I have so many lines of code already.

    <?php
$p1speed = 5;
$p2speed = 3;

//way for next turn: 1 for turn
$p1 = 0;
$p2 = 0;

if($p1speed>$p2speed) {

    $p1 = 1;
    $p2 = 0;

}
else if($p2speed>$p1speed) {

    $p1 = 0;
    $p2 = 1;

}

$count=0;

//begin
while($count<5){

    if($p1==1) {

        echo 'P1 turns
';

        //adjust
        $p1 = 0;
        $p2 = 1;

    }
    else if($p2==1) {

        echo 'P2 turns
';

        //adjust
        $p1 = 1;
        $p2 = 0;

    }

    $count++;

}
?>

This is a refactoring of the original code. It picks the starting player and then uses a simple toggle ($currentPlayer = ($currentPlayer == 1) ? 2 : 1;) to determine which player goes next for 5 rounds.

$currentPlayer = 1;

if ($player2speed > $player1speed) {

    $currentPlayer = 2;

}

$count = 0;

while ($count < 5) {

    echo $currentPlayer . " turns<br/>";

    $currentPlayer = ($currentPlayer == 1) ? 2 : 1;

    $count ++;
}

Note, the current code fails if both player's speeds are the same.

If the 5 in the while loop is fixed you can simply unroll it as: [C code, but I hope you get the idea]

void turn(int player);

void foo(){
    int p1speed = 3;
    int p2speed = 5;

    int first_player;
    int second_player

    if(p1speed > p2speed){
        first_player = 1;
        second player = 2;
    }
    else{
        first_player = 2;
        second_player = 1;
    }

    //now, instead of the while loop you can have:
    turn(first_player);
    turn(second_player);
    turn(first_player);
    turn(second_player);
    turn(first_player);
}

void turn(int player){
    printf("Player %d turn", player);
    int other = 3 - player; // Nice trick
    //Do something
}


Guessing a number, but comments concerning I always get marked down for my comments and I just wanted to see if these comments are acceptable or what I should include/where I should include them.

import java.util.*; //so I can use scanner

public class GuessingGame {
   public static void main(String[] args) {

      Random rand = new Random ();
      int max = 100; //sets limit on the random number generator
      Scanner input = new Scanner(System.in);
      int guess;//so I can compare console input to random number
      boolean play = true;
      int totalGames = 0;
      int totalGuesses = 0;
      int bestGame = Integer.MAX_VALUE;

      System.out.println("Can you guess the word?");
      System.out.println("I am sure you cannot guess!");
      System.out.println("Go ahead and try!");
      System.out.println();

      while (play) { //so game will restart after a win

         System.out.println("I'm thinking of a number between 1 and " + max + "...");
         int numberToGuess = rand.nextInt(max) + 1;
         int numberOfTries = 0;
         //so user can continue guessing until correct
         boolean win = false;
         while (!win) {

            System.out.print("Your guess? " + numberToGuess);
            guess = input.nextInt();
            numberOfTries++;

            if (guess == numberToGuess) {
               win = true;
            } else if (guess > numberToGuess) {
               System.out.println("It's lower.");
            } else if (guess < numberToGuess) {
               System.out.println("It's higher.");
            }
            input.nextLine();
         }

         if (numberOfTries == 1) {
            System.out.println("You got it right in " + numberOfTries + " guess!");
         } else {
            System.out.println("You got it right in " + numberOfTries + " guesses!");
         }

            totalGames++;
            totalGuesses+= numberOfTries;
            System.out.print("Do you want to play again? ");

            //so user can choose to play another game

            String answer = input.nextLine();
            char firstLetter = answer.charAt(0);
            if (firstLetter == 'y' || firstLetter == 'Y') {
            play = true;
            } else {
            play = false;
         }

         bestGame = Math.min(bestGame, numberOfTries);
         System.out.println();
      }

      System.out.println("Overall results:");
      System.out.println("Total games   = " + totalGames);
      System.out.println("Total guesses = " +  totalGuesses);
      System.out.printf("Guesses/game  = %.1f \n", totalGuesses/(double)totalGames);
      System.out.println("Best game     = " + bestGame);
   }
}

Welcome to the hell of comments, and personal taste. There are multiple answers to your core question that may be right:

You have not commented enough.
You have commented all the wrong things.
You have commented too much.
Your comments are in the wrong format.
You are missing the formal comments.

To be clear though, nothing we can say will be in line with what your marks are based on, unless you can tell us why you were marked down... ;-)

About Comments in general
OK, comments are supposed to make plain what the code does not tell us already.
Let me say that again.

Comments are supposed to make plain what the code does not tell us already.

Now, whether you are marked on that, or not, is a different story. I have found that the academic application of marks for comments is often contrary to the need for comments... particularly when the code is good code.
Good code seldom needs comments.
Let me say that again:

Good code seldom needs comments.

So, follow this logic through, and then follow the inverse back...

Comments make plain what the code does not tell us already.
Good code seldom needs comments.

thus....

Good code seldom needs comments.
Good code must make plain what the code does without the need for additional explanation.

Applied to your code
Let's start at the top....

import java.util.*; //so I can use scanner

Now, if your code was:
import java.util.Scanner;

then it would not need the comment.
Your next comment is here:

  int max = 100; //sets limit on the random number generator

Well, that is, actually, not a horrible comment, or code. But, it could be better, if we gave it a decent name:
int randomGeneratorLimit = 100;

It would be even better if we made that magic number a constant:
private static final int RANDOM_GENERATOR_LIMIT = 100;

Now, no need for a comment at all.
Next up, this line:

  while (play) { //so game will restart after a win

Well, this one is interesting.... the comment is actually needed here because of the loop you are using.
If you convert this to a do-while loop, and use some functional extraction... then you can convert your code to a self-documenting, commentless block:
private static boolean playAgain() {
    System.out.print("Do you want to play again? ");
    String answer = input.nextLine();
    char firstLetter = answer.charAt(0);
    return firstLetter == 'y' || firstLetter == 'Y';
}

then you can use that function as follows:
do {

    ....... // game logic

} while (playAgain());

No need for a comment (or a play variable).
Similarly, you have the code:

     //so user can continue guessing until correct
     boolean win = false;
     while (!win) {

This should be a do-while as well, and should look like:
do {
     ..... // game logic
} while (guess != numberToGuess);

Again, that makes the logic clear, no need for a comment.
I believe that has now eliminated all of your comments..... and replaced them with code that does not need a comment, because the code is self-explanatory.
In other news, if you had more functions, with good names, then your code would be simpler to read as well.
Bottom line, though, is that the comments in your code should fill in the blanks that your code does not. In addition, your comments should give details on the motivation, and not the application of your code. You should, in general, comment only on why your code does things, not what your code is doing.
That leads on to the other comments you are missing... JavaDoc.
JavaDoc is documentation that should explain what your code does at an abstract level.
You have no JavaDoc, and you likely should. JavaDoc is where you describe what your code does, because, typically, the people reading the javadoc are not reading the code, so they need something else to tell them what the code does.

Your code is missing several very important comments. Some of the comments you do have are lies, others are trivial, yet others don't say what you should be saying.
The most important comment of all
(From the perspective of the lawyers)
// Copyright 2015 Eric Irwin. All rights reserved.

This doesn't matter so much in the world of classroom assignments. In the working world, it can matter a lot. Some people elaborate on what rights are reserved. Why? Reserve all rights. Or maybe refer to the license agreement. Keeping it short and simple makes it easier to insert this very important comment.
The most important comments of all
(From the perspective of javadoc)
Every class, every function should have a javadoc comment. You do not (you should not!) go overboard. Do not say when you wrote it, or even who wrote it. Write, in plain English (or plain French, or German, or whatever) why the class or function exists.
In the case of a class, say why the class exists, how to use it. Most classes have multiple member functions, multiple data members. An external user who calls those member functions in a random order is most likely to be toast. How member functions need to be sequenced, how they interact with one another: That's the kind of stuff you want to put into that class javadoc comment.
In the case of a function, document what the function does, why it exists -- in plain English. Document the arguments to the function and the return value from the function. In one case (main), that might be blatantly obvious, but a program only has one main. The arguments to main oftentimes are tricky as can be. In this case, you aren't using the arguments -- so say so!
Other missing comments
Your comments show small things but miss the big picture. A perfect example:
while (play) { //so game will restart after a win

Much better, in my opinion:
// Play game after game until the user decides to stop.
while (play) {

That outer loop exists so a user can play multiple games. (This was probably a part of the assignment. So say that.) That's the big picture. So game will restart after a win? That's not "big picture". While well-written code can explain itself to some extent, it does not explain the big picture, aka intent or architecture.
End-of-line comments versus full-line comments
Most of your comments are end-of-line comments. This style of commenting forces people to be terse; more often than not, overly terse. It also encourages people to comment the obvious (e.g., i++; // increment i). The only thing worse than i++; // increment i is a comment that is inconsistent with the code.
Commenting the obvious with an end-of-line comment is easy, but your code for the most part should be obvious. A comment on the obvious is not needed. Commenting the not so obvious? End-of-line comments typically don't do that very well.


Check whether two JavaScript arrays are equal, using basic library only I am writing a function that takes in two arrays and a test name and outputs statements (console.log) saying whether the two arrays are equal. I don't want to convert the arrays to strings or use non-core libraries to do this. I am also new to writing JS so I wanted to get a code review here.

Usage: FAILURE CASE:
var expected = [1, 1, 2, 3, 5, 8, 13];
var actual = [2, 3, 5, 8, 13, 21, 34];
assertArraysEqual(actual, expected, 'generates first 7 Fibonacci
numbers');

console output:
 FAILED [generates first 7 Fibonacci numbers] Expected "1, 1, 2, 3, 5,
8, 13", but got "2, 3, 5, 8, 13, 21, 34"

function assertArraysEqual(actual, expected, testName) {

  var lengthsEqual = (actual.length === expected.length)
  var contentsEqual = true
  for (var i=0; i<actual.length; i++)
  {
    if (actual[i] !== expected[i]) contentsEqual=false
  }
  if(contentsEqual && lengthsEqual){
   console.log("passed")
  }
  else{
    console.log('FAILED [' + testName + '] Expected "' + expected + '", but got "' + actual + '"');
  }
}

Your function should be dedicated to its functionality. Have all the printing done externally. You don't need testName as a parameter.
Have your function return true or false. It's much more useful and reusable that way. Having it return also allows you to exit the function early if you know the arrays are not equal. For example:

function assertArraysEqual(actual, expected) {
    if (actual.length !== expected.length) {
        return false;
    }
    ...

If this is for something like unit testing, you should use a unit test library to like Mocha or Qunit to validate the behavior of the function.
What data types are you dealing with? Are you only concerned with numbers? Do you want strict type checking and/or null checking?

If you're only concerned with validating arrays of numbers and you know the bounds of the input, you can easily rewrite your function like so:

    function assertArraysEqual(actual, expected) {
        if (actual.length !== expected.length) {
            return false;
        }
        for (var i = 0; i < actual.length; ++i) {
            if (actual[i] !== expected[i]) {
                return false;
            }
        }
        return true;
    }

For one thing, it would good practice to end your lines of code with ;.

Also, you did not enclose contentsEqual=false in brackets.

It is not clear if you called the function assertArrays.  Functions do not run automatically.

It would be better if the for construct only ran while lengthseq and contentEqual are true.


Display a number as a product of its prime factors Task:

Link to original problem :https://www.spoj.com/problems/FACTCG2/

  The task in this problem is to write a number in a multiplication of
  prime
      numbers separated by “x”. You need to put the number 1 in this multiplication.

  Input:

  The input consists of several lines.

  Each line consists of one integer N (1 <= N <= 10^7) .

  Example Input:

           1
           2
           4
           8

  Example Ouput:

           1
           1 x 2
           1 x 2 x 2
           1 x 2 x 2 x 2

My approach:

I pre-compute an array Primes which contains the smallest prime factor of every number from 1 to 1e7 using the sieve.  Then to find the answer , I divide the number given in each query by its least prime factor until it becomes '1' or a prime number. Then I display it.

The code works for small inputs but SPOJ gives me time limit exceeded. I think maybe the vector in factorise function has
            something to do with it. I tried printing the answer without storing it in vector but 'x' makes it difficult .

My code:

#include <bits/stdc++.h>
using namespace std;
vector<long long>Primes(1e7);
bool prime(long long x)
{

    for(int i=2;i<=sqrt(x);i++)
    {
        if(x%i==0)
        return false;
    }
    return true;
}
void least_prime_factor()   //To store least prime factor of a
                             //  given number
 {

    Primes[1]=1;
                    for(int i=2;i<=1e7;i++)
                    {
                            if(Primes[i]==0)
                            {
                               Primes[i]=i;
                               for(int j=2;i*j<=1e7;j++)
                                {
                                    if(Primes[i*j]==0)
                                    Primes[i*j]=i;
                                }

                             }
                    }

 }
vector<long long>factorise(int x)  //This is to store the factors
                                   //of a number

{
    vector<long long>ret;
    while(x!=1||prime(x)!=true)
    {
        ret.push_back(Primes[x]);
        x=x/Primes[x];
    }
return ret;
}

int main() {

least_prime_factor();
int n;
while(scanf("%d", &n) == 1)
if(n==1)
{
    cout<<"1"<<endl;
}
else{

    vector<long long>ans=factorise(n);
    cout << "1 x" ;
    for(int i=0;i<ans.size();i++)
    {
        if(i==ans.size()-1)
            printf(" %d ", ans[i]);
        else
            printf(" %d x", ans[i]);
    }
    cout<<endl;
}
    return 0;
}

Don't include <bits/stdc++.h> - it's not standard, and on platforms that provide it, it drags in far too much.

Don't using namespace std - this namespace is not designed for wholesale inclusion, and can produce subtle bugs when its names overload your own.

Don't use a floating-point number such as 1e7 when you want a simple integer 10'000'000.

Don't compute std::sqrt(x) every time around the loop - save the value as an integer for faster comparisons.

Use unsigned types for unsigned arithmetic.  Given that inputs (and therefore factors) are no greater than 10⁷, we can get away with std::uint_fast32_t rather than long long.

Although the code claims to use a sieve, every input number is inefficiently tested for primality without reference to the sieve (multiple times, even).

Why use a std::vector rather than std::array for something whose size is constant and known at compile time?  Also, why write the size in two places (which need to be consistent) instead of simply using Primes.size()?  (Also, fix the off-by-one error where we overstep the end of the vector).

I recommend using C++ streams, rather than mixing C buffered I/O and forcing the two libraries to repeatedly flush each other.

Avoid global variables - Primes can be a local static member (perhaps with an immediately-executed lambda expression as initializer).

Why not always include 1 as the first result, rather than have to add that in the printing code?

To dive in to one function in particular, let's pick on least_prime_factor().  Apart from the misleading name, we're doing a lot more work than we need to here.  Every time around the loop, we're multiplying i by j (a good compiler will hoist the three i*j into a single computation, and reuse that), but we could simply be adding i each time.

A standard improvement to naive sieves is to observe that every multiple of a prime p that's less than p² also has smaller factor than p, so once we've marked p, we can jump straight to p² before striding by p thereafter.

Apart from the first pass (which marks even numbers), we can stride by 2 p each time (knowing that we've already marked even numbers).  We can take that further with a wheel of factors, but even this is reducing the work by a factor of two.

Let's see what we have now:

// Create an array populated with the smallest prime factor of each
// number, or 0 for primes (and unity).
void least_prime_factor()
{
    Primes[1]=1;
    // First, mark all the even numbers; this allows us to halve the
    // work for all other primes.
    for (std::size_t i = 2;  i < Primes.size();  i += 2) {
        Primes[i] = 2;
    }

    for (std::size_t i = 3;  i < Primes.size();  ++i) {
        if (Primes[i]) { continue; }

        Primes[i] = i;
        for (std::size_t j = i * i;  j < Primes.size();  j += 2 * i) {
            if (!Primes[j]) {
                Primes[j] = i;
            }
        }
    }
}

That's noticeably less work, and if we also replace prime() with a simple lookup in Primes[], then we'll have a much more efficient program.

First of all you are using cout and asking for TLE help !!!. You need to remove cout first from your code Also i can see that you use local vector array assignment in our main function. Local vector allocation is time consuming. Try to avoid this too.


BestHTTP asset download I'm using the asset BestHTTP in unity and I'm downloading a few image from amazonaws to use as texture. It used to be one image, but now I have a few.

I was not able to shorten it. When I'm trying to do it in a foreach loop loop it is misbehaving.

Could someone help me to optimize this working code?

for (int i = 0; i < tzPlayInfo.Instance.bc_gametablelist.Count; i++)
{
    dealer_img += tzPlayInfo.Instance.bc_gametablelist[i].dlrimage;
    dealer_img += ",";
}
string[] newLinks = dealer_img.Split(',');

new BestHTTP.HTTPRequest(new System.Uri("**************************.amazonaws.com/resources/"
    + "dealer/pic/" + newLinks[0]),
    (BestHTTP.HTTPRequest req, BestHTTP.HTTPResponse res)
    =>
    {
        var tex = new Texture2D(20, 20);
        tex.LoadImage(res.Data);
        uitex[0].GetComponent<UITexture>().mainTexture = tex;
    }).Send();

new BestHTTP.HTTPRequest(new System.Uri("**************************.amazonaws.com/resources/"
    + "dealer/pic/" + newLinks[1]),
    (BestHTTP.HTTPRequest req, BestHTTP.HTTPResponse res)
    =>
    {
        var tex = new Texture2D(20, 20);
        tex.LoadImage(res.Data);
        uitex[1].GetComponent<UITexture>().mainTexture = tex;
    }).Send();

new BestHTTP.HTTPRequest(new System.Uri("**************************.amazonaws.com/resources/"
    + "dealer/pic/" + newLinks[2]),
    (BestHTTP.HTTPRequest req, BestHTTP.HTTPResponse res)
    =>
    {
        var tex = new Texture2D(20, 20);
        tex.LoadImage(res.Data);
        uitex[2].GetComponent<UITexture>().mainTexture = tex;
    }).Send();

new BestHTTP.HTTPRequest(new System.Uri("**************************.amazonaws.com/resources/"
    + "dealer/pic/" + newLinks[3]),
    (BestHTTP.HTTPRequest req, BestHTTP.HTTPResponse res)
    =>
    {
        var tex = new Texture2D(20, 20);
        tex.LoadImage(res.Data);
        uitex[3].GetComponent<UITexture>().mainTexture = tex;
    }).Send();

Instead of using a foreach here, you need to use a for-loop, since you have to know the index of the image to assign it to the correct texture.

// this line replaces the first for loop you have there
var textureLinks = tzPlayInfo.Instance.bcGameTables.Select(t => t.dlrimage).ToArray();
// verify we have enough uitex elements:
if (uitex.Length < textureLinks.Length)
{
    throw new RuntimeException("too many textures linked");
}
for (int i = 0; i < textureLinks.Length; i++)
{
    new BestHTTP.HTTPRequest(new System.Uri($"**.amazonaws.com/resources/dealer/pic/{textureLinks[i]}"),
        (req, res) => {
            var tex = new Texture2D(20, 20);
            tex.LoadImage(res.Data);
            uitex[i].GetComponent<UITexture>().mainTexture = tex;
        }).Send();
}

There's also some words to be lost on the naming convention you use.

dealer_img should be dealerImg, uitex should be uiTextures, bc_gametablelist should be bcGameTables, dlrimage should be dealerImage...

In general you seem to try to shorten your names and adhere to C++ conventions. That would be fine if you were programming the graphics in C++. But since this is C#, you should be following C# convention (namely camelCase)

In the code above I made use of interpolated strings for the Uri. I also left the variable types of the lambda up for inference by the compiler. It should be smart enough to do so, if BestHTTP properly specified the generics on HTTPRequest.

for (int i = 0; i < tzPlayInfo.Instance.bc_gametablelist.Count; i++)
    {
        dealer_img += tzPlayInfo.Instance.bc_gametablelist[i].dlrimage;
        dealer_img += ",";
    }
    string[] newLinks = dealer_img.Split(',');

    for (int i = 0; i < newLinks.Length - 1; i++)
    {
        var index = i;  // We need to make a local copy because C# captures variables by reference to lambdas.
        new BestHTTP.HTTPRequest(new System.Uri("********************.amazonaws.com/resources/"
            + "dealer/pic/" + newLinks[index]),
            (BestHTTP.HTTPRequest req, BestHTTP.HTTPResponse res)
            =>
            {
                var tex = new Texture2D(20, 20);
                tex.LoadImage(res.Data);
                uitex[index].GetComponent<UITexture>().mainTexture = tex;
            }).Send();
    }

It's weird but it works.