C++ Software cache for shared resources I wrote a basic C++ template to manage shared resources like textures and shaders in a 3D engine. The idea is that the cache itself holds weak references to the resources (through std::weak_ptr) and turns it into strong references (through std::shared_ptr) when the resource is fetched from the cache or constructed if it's not present. As soon as all strong references go out of scope, the given object gets deleted and removed from the cache by a custom deleter. Suggestions/improvements are welcome:

#ifndef PTRCACHE_H
#define PTRCACHE_H

#include <memory>
#include <map>
#include <functional>
#include <mutex>

namespace MyEngine
{
  template<typename Key, typename Val, typename ... Args>
  class PtrCache
  {
  public:
    PtrCache(const std::function<Val*(Args...)>& creator) :
      _creator(creator),
      _mutex(std::make_unique<std::mutex>())
    {}
    PtrCache(const PtrCache& other) = delete;
    PtrCache& operator=(const PtrCache& other) = delete;
    PtrCache(PtrCache&& other) :
      _cache(std::move(other._cache)),
      _creator(std::move(other._creator)),
      _mutex(std::move(other._mutex))
    {}
    PtrCache& operator=(PtrCache&& other)
    {
      if (this != &other) {
        _cache = std::move(other._cache);
        _creator = std::move(other._creator);
        _mutex = std::move(other._mutex);
      }
      return *this;
    }
    /**
    * Returns true if ret was taken from the cache, false otherwise.
    */
    bool getOrCreate(const Key& key, std::shared_ptr<Val>& ret, Args... args)
    {
      std::lock_guard<std::mutex> lock(*_mutex);
      auto it = _cache.find(key);
      if (it != _cache.end()) {
        ret = std::shared_ptr<Val>(it->second); // Construct from weak_ptr
        return true;
      }
      ret = std::shared_ptr<Val>(_creator(args...), [this, key](Val* ptr) { // Construct from creator and pass custom deleter which removes the element from the cache.
        std::lock_guard<std::mutex> lock(*_mutex);
        _cache.erase(key);
        delete ptr;
      });
      _cache[key] = ret;
      return false;
    }
    inline void clear()
    {
      std::lock_guard<std::mutex> lock(*_mutex);
      _cache.clear();
    }
    inline size_t size()
    {
      std::lock_guard<std::mutex> lock(*_mutex);
      return _cache.size();
    }
  private:
    std::map<Key, std::weak_ptr<Val>> _cache;
    std::function<Val*(Args...)> _creator;
    std::unique_ptr<std::mutex> _mutex;
  };
}

#endif

Basic usage example:

PtrCache<std::string, GLTexture, const std::string&> texture_cache([] (const std::string& path) {
  auto tex = SOIL_load_OGL_texture(path.c_str(), SOIL_LOAD_AUTO, SOIL_CREATE_NEW_ID, SOIL_FLAG_MIPMAPS | SOIL_FLAG_TEXTURE_REPEATS | SOIL_FLAG_COMPRESS_TO_DXT);
  if (tex) {
    return new GLTexture(tex, GL_TEXTURE_2D);
  }
  else {
    throw std::exception((std::string("Could not create texture ") + path).c_str());
    return nullptr;
  }
});

std::shared_ptr<GLTexture> my_texture;
std::string path = "my_texture.png";
texture_cache.getOrCreate(path, my_texture, path);

The only caveat I see is that the user of this class has to ensure that the lifetime of the cache goes beyond the lifetime of any contained managed object. Otherwise, the custom deleter would obviously cause undefined behaviour.

The basic idea of your cache, as I understand it, is that it is a map of weak pointers connected to shared pointers with custom deleters that remove them from the map. As you noticed yourself, that's a risky pattern, because if any resources outlive the cache, you get UB.

Is it really necessary for objects to remove themselves from the cache? What if you just let them destroy themselves naturally, and leave the empty weak pointer in the cache. Then your lifetime worries fade away. From time to time you can purge the cache of entries where expired() == true if you really want, and other functions of the cache will have to take into account expired entries (like size() for example). It even makes your cache more flexible, because if you're not using the deleter you allow the possibility of users providing their own.

I also question the wisdom of keeping the "creator" function object in the cache, for a couple of reasons:

It makes the cache itself so much clunkier to use, because you have to specify all the arguments to the create function in the template parameter list.
It makes the cache less flexible, because there may be multiple ways to create (for example) a texture (such as from a file, or from a memory buffer), so you might want different create function signatures.

Rather than hard-coding the create function's signature in the type, and deciding on the create function for everything in cache at cache construction time, why not just pass an optional create function when and where it's needed?

I'll bring these points up again as I go through the code.

PtrCache(const std::function<Val*(Args...)>& creator) :
  _creator(creator),
  _mutex(std::make_unique<std::mutex>())
{}

Single-argument constructors should pretty much always be explicit.

Since this is a pure sink function, you should really take the arguments by value, not by const&. And then you should move them into their final place:

explicit PtrCache(std::function<Val*(Args...)> creator) :
  _creator{std::move(creator)},
  _mutex{std::make_unique<std::mutex>()}
{}

But I would suggest not having the _creator member variable, so you don't need this constructor at all. Or the Args... template parameter for the class either.

PtrCache(PtrCache&& other) :
  _cache(std::move(other._cache)),
  _creator(std::move(other._creator)),
  _mutex(std::move(other._mutex))
{}
PtrCache& operator=(PtrCache&& other)
{
  if (this != &other) {
    _cache = std::move(other._cache);
    _creator = std::move(other._creator);
    _mutex = std::move(other._mutex);
  }
  return *this;
}

There's no need to write out either of these, because they're just the default implementations. You can just do:

PtrCache(PtrCache&& other) noexcept = default;
PtrCache& operator=(PtrCache&& other) noexcept = default;

Also, both can be noexcept (and move ops always should be).

bool getOrCreate(const Key& key, std::shared_ptr<Val>& ret, Args... args)
{
  std::lock_guard<std::mutex> lock(*_mutex);
  auto it = _cache.find(key);
  if (it != _cache.end()) {
    ret = std::shared_ptr<Val>(it->second); // Construct from weak_ptr
    return true;
  }
  ret = std::shared_ptr<Val>(_creator(args...), [this, key](Val* ptr) { // Construct from creator and pass custom deleter which removes the element from the cache.
    std::lock_guard<std::mutex> lock(*_mutex);
    _cache.erase(key);
    delete ptr;
  });
  _cache[key] = ret;
  return false;
}

This is the meat of the cache, and so the best place to talk about broader problems. But the function has plenty of problems of its own.

First, out-arguments are a bad idea. They're clunky as all hell. Every use of this function - which is the main function of the cache - requires two lines:

std::shared_ptr<GLTexture> result;
texture_cache.getOrCreate("my_texture.png"s, result, "my_texture.png"s);

The natural place for the output of a function is the return value. If you want multiple return values - so you can get that bool as well - use a custom return struct, or a tuple:

std::tuple<std::shared_ptr<Val>, bool> getOrCreate(const Key& key, Args... args);

Now this is much easier to use, especially since C++17:

auto [ result, created ] = texture_cache.getOrCreate("my_texture.png"s, my_texture.png"s);

At a higher level, you've hard-coded the create function into the cache, which makes it less flexible. Rather than doing that, you can take a function object in getOrCreate(), like this:

auto [ tex1, created1 ] = texture_cache.getOrCreate("texture_1", [] { return load_texture_from_file("texture_1.png"); });
auto [ tex2, created2 ] = texture_cache.getOrCreate("texture_2", [] { return generate_procedural_texture(seed_value); });
// and so on

The signature of getOrCreate() now becomes:

template <typename Func>
std::tuple<std::shared_ptr<Val>, bool> getOrCreate(const Key& key, Func&& func);

and the creation line in the function is:

ret = std::make_shared<Val>(func());

Or even better, let the cache objects construct themselves:

template <typename Args...>
std::tuple<std::shared_ptr<Val>, bool> getOrCreate(const Key& key, Args&&... args)
{
    // ...
    ret = std::make_shared<Val>(std::forward<Args>(args)...);
    // ...
}

// used as:
auto [ tex1, created1 ] = texture_cache.getOrCreate("texture_1", from_file, "texture_1.png");
auto [ tex2, created2 ] = texture_cache.getOrCreate("texture_2", noise_texture, seed_value);
// assumes texture_cache holds "texture" objects that have the following constructors, or something similar:
// texture::texture(from_file_t, std::filesystem::path);
// texture::texture(noise_texture_t, std::uint_fast32_t);

There are multiple ways to do all this that are way more flexible than setting the create function and its signature when the cache is created.

This function also seems a little over-complex, using find() and then []. Your map values are weak pointers. There's really no need for all that dancing around to get-or-set them in the map. You can just do:

bool getOrCreate(const Key& key, std::shared_ptr<Val>& ret, Args... args)
{
    std::lock_guard<std::mutex> lock(*_mutex);
    auto& wkptr = _cache[key];
    ret = wkptr.lock();
    if (ret)
        return true;
    // Either the value wasn't in the map, or it was but it expired
    // (so it was effectively removed from the map). Either way,
    // make a new thing and store it in the cache.
    ret = std::make_shared<Val>(_creator(args...));
    wkptr = ret;
    return false;
}

Also, along with everything I mentioned above about the create function stuff, if you're going to make this function a template - either to take a create function object or an argument parameter pack to pass to the value type's constructor - you might as well take advantage of that and use perfect forwarding for the key as well.

Also-also, you use make_unique(), and that's good... but it's not that important. It is, however, important to use make_shared(). You should aim for that in the function's design.

inline void clear()
{
    std::lock_guard<std::mutex> lock(*_mutex);
    _cache.clear();
}

The inline keyword serves no purpose here. Functions defined in the class are inline by default. What would be handy here are const and noexcept. At least const, because locking might throw an error. Same goes for size().

There's another nasty little issue with having a clear() function, though. Your cache objects erase themselves from the cache when the last one goes out of scope, right? Consider this scenario:

Subsystem A creates a cache object with key "foo", and gets an instance.
Subsystem (any!) calls clear() on the cache. Subsystem A's "foo" instance is still valid, of course. It just doesn't have an entry in the cache anymore.
Subsystem B creates a new cache object with the key "foo", and gets an instance.
Subsystem A lets its "foo" instance expire... which triggers the deleter to erase "foo" from the cache.
Subsystem B asks for another "foo" object from the cache, expecting to get an alias of the one it got in step 3 because it knows no-one's called clear() since it got the "foo" object in step 3... instead it gets a whole new "foo".

In other words, the cache becomes unpredictable. Even if I'm holding an object I got from a specified key and I know for sure no one's cleared the cache since I got it, I still can't be sure I'll get the same object from the same key (or any object at all from that key).

std::map<Key, std::weak_ptr<Val>> _cache;

Is there any reason you're not using an unordered map here?

Summary

As you've noted, having cache objects remove themselves from the cache when they expire invites problems. There's no good reason to do that. Just leave the empty weak pointers in the cache, and cull them from time to time (or not; just leave them there and ignore them).
Defining the create function arguments at instantiation time, and the create function itself at construction time, is inflexible, and it complicates the class. It's much more flexible to be able define creation parameters when and where I need them. The class should be PtrCache<Key, Val>, maybe with an allocator option. Having Args... in there is clunky and restrictive.
The most flexible option is probably to let your cache objects' constructors do what constructors are for, and define getOrCreate() as a template function taking arguments that get perfect-forwarded to the constructor if and only if something actually gets constructed.
Use make_shared().
Don't manually redefine implicitly generated functions; just = default them.


DRYing out my action creators I've created a private function called _successResponse to handle successful responses from my promise.

The challenge was passing the dispatch function to the _successResponse which is being passed to the .then method as a callback.

I accomplished this by currying using .bind. I would love to get feedback on this approach.

import axios from 'axios';
import { browserHistory } from 'react-router';
import {
        AUTH_USER,
        AUTH_ERROR,
        UNAUTH_USER,
        FETCH_MESSAGE
    } from './types';

const ROOT_URL = 'http://localhost:3090';

export function signinUser({ email, password }){
    return function(dispatch){
        // Submit email/password to server
        axios.post(`${ROOT_URL}/signin`, { email, password })
            .then(_successResponse.bind(null, dispatch))
            .catch(() => {dispatch(authError('Bad Login Info'))})
    }
}

export function signupUser({email, password}){
    return function(dispatch){
        axios.post(`${ROOT_URL}/signup`, { email, password })
            .then(_successResponse.bind(null, dispatch))
            .catch(response => {dispatch(authError(response.response.data.error))});
    }
}

export function authError(error){
    return {
        type: AUTH_ERROR,
        payload: error
    }
}

export function signoutUser(){
    localStorage.removeItem('token');

    return { type: UNAUTH_USER }
}

export function fetchMessage(){
    return function(dispatch){
        axios.get(ROOT_URL, {
            headers: { authorization: localStorage.getItem('token') }
        })
            .then(response => {
                dispatch({
                    type: FETCH_MESSAGE,
                    payload: response.data.message
                })
            });
    }
}

// Private functions
function _successResponse(dispatch, response){
        // If request is valid...
        // - Update state to indicate user is authenticated
        dispatch({ type: AUTH_USER });
        // - Save the JWT token
        localStorage.setItem('token', response.data.token);
        // - redirect to the route '/feature'
        browserHistory.push('/feature');
}

Currying?

Photo hosted via unsplash, taken by Monika Grabkowska
Yes you used Function.bind() to fix the callback function as the first argument to the private function, and this is a good approach to avoid excess functions and reduce the number of lines. While the concept is similar, what this code uses is actually Partial Application. Read more about the differences in this article.

The first couple functions appear to catch errors, but fetchMessage() doesn't appear to. Maybe you have updated it since you posted it but if not, I would suggest adding error handling there too.

You could consider using async and await to reduce the .then() callbacks.


Project Euler Problem #50 Consecutive Prime Sums Problem: Which prime, below one-million, can be written as the sum of the most consecutive primes?

At the moment my code runs for over 200 seconds and was wondering how I can find a way to bring that down by a large amount. I have tried optimising it as much as I can and added an extra if statement, if(subSum < 1000000): to reduce the need to see if subSum is a prime. Here is my code at the moment:

def sieve(num):
    primes = []
    import math
    for i in range(num):
        primes.append(True)
    j = 1
    while(j<(math.sqrt(num)+1)):
        k = (j+1)*2
        while(k<(num+1)):
            primes[k-1] = False
            k+=(j+1)
        j+=1
    primes[0] = False
    prime = []
    for a in range(num):
        if(primes[a] == True):
            prime.append(a+1)
    return prime

primes = sieve(1000000)
count = 1
high = 0
length = len(primes)
while count <= length:
    i = 0
    while(i<(length-count)):
        sub = primes[i:i+count]
        subSum = sum(sub)
        if(subSum < 1000000):
            if(subSum in primes):
                if(subSum>high):
                    high = subSum
                break
            else:
                i+=1
        else:
            break
    count += 1
print(high)

Thanks for any help.

I'll try to focus on the performance of the code provided, and what can be done to improve it. One key observation that I found is that it is more efficient to start searching for larger consecutive sums, and move towards shorter until we find a valid candidate.

Then the obvious question is: where do we start? To find this, simply check the cumulative sums for the first n primes. As n increases, so does the sum. When the sum is above 1000000, it would not be possible to find a sum of n primes which is in the list of primes below 1000000. With this, it can be deduced that the sum can contain 546 elements at most. You should verify this on your own.

Another observation is that 2 is the only even prime. With the knowledge that odd + odd = even, even + odd = odd + even = odd, and even + even = even, we can deduce that if and only if 2 is a part of the consecutive sum, the sum must contain an even number of elements. Thus, we must only check the very first consecutive sum (the one starting with 2) when the sum has an even number of elements.

With all of this in place, the speed of the search is improved considerably.

n = 1000000
found = False
primes = sieve(n)
primes_set = set(primes)

for test_len in range(545, 1, -2):
    for i in range(0, len(primes)-test_len):
        s = sum(primes[i:i+test_len])
        if s in primes_set:
            print("found", test_len, s)
            found = True
            break
    if found:
        break

This is of course only a small snippet, and not the full program. For example, this does not test for even length consecutive sums, but that should be trivial with the information provided above. However, the execution time for the loop is less than a second, with the majority of the runtime now being used by the prime sieve. However, optimizing that is outside the scope of this answer.


Finding equivalent to % for the inner if clauses I have a static list of descriptions in a resource and I am filtering down the list based on the index positions of the text in the array

Here is my multi converter:

using OCLMEditor.Data.StudentInfo.Enums;
using System;
using System.Collections.Generic;
using System.Globalization;
using System.Windows.Data;

namespace OCLMEditor.ValueConverters
{
    public class StudentAssignmentTypeFilterConverter : IMultiValueConverter
    {
        public object Convert(object[] values, Type targetType, object parameter, CultureInfo culture)
        {
            StudentAssignmentType[] allTypes = values[0] as StudentAssignmentType[];
            List<StudentAssignmentType> filteredTypes = new List<StudentAssignmentType>();
            int iNumberOfClasses = values[1] != null ? (int)values[1] : 0;

            if (iNumberOfClasses == 3) // Main hall, Class 1 and Class 2
                return allTypes;

            if(iNumberOfClasses == 0)
            {
                // What to return? It should not get here as the combo is disabled if
                return allTypes;
            }

            for(int i = 0; i < allTypes.Length; i++)
            {
                if(iNumberOfClasses >= 1)
                {
                    if (i == 0 || i == 3 || i == 6 || i == 9 || i == 12 || i == 15 || i == 18)
                        filteredTypes.Add(allTypes[i]);
                }

                if(iNumberOfClasses >= 2)
                {
                    if (i == 1 || i == 4 || i == 7 || i == 10 || i == 13 || i == 16 || i == 19)
                        filteredTypes.Add(allTypes[i]);
                }

                if (iNumberOfClasses == 3)
                {
                    if (i == 2 || i == 5 || i == 8 || i == 11 || i == 14 || i == 17 || i == 20)
                        filteredTypes.Add(allTypes[i]);
                }
            }

            return filteredTypes;
        }

        public object[] ConvertBack(object value, Type[] targetTypes, object parameter, CultureInfo culture)
        {
            throw new NotSupportedException();
        }
    }
}

Do you think the if clauses inside can be simplified somehow?

Specifcally these three if clauses. Eg:

if(iNumberOfClasses >= 1)
{
    if (i == 0 || i == 3 || i == 6 || i == 9 || i == 12 || i == 15 || i == 18)
        filteredTypes.Add(allTypes[i]);
}

I know about using % to get the (modulus(?)) remainder of a division. So if my numbers were 1, 5, 10, 15, 20 I could use number % 5 == 0 to get all of them. But ican't see that I can do that here?

The answer of t3chb0t provides a solution in the right direction. My point of "critique" with that solution is the overdose on LinQ for a simple solution like checking if a collection of numbers contains a certain number.

The point of predefining the collection is a good idea. I would do it like this:

var classesForOne = new [] { 0, 3, 6, 9, 12, 15, 18 };
var classesForTwo = new [] { 1, 4, 7, 10, 13, 16, 19 };
var classesForThree = new [] { 2, 5, 8, 11, 14, 17, 20 };

Now you can check if the variable i is present in a collection. This can be done like this:

if(iNumberOfClasses >= 1)
{
    if (classesForOne.Contains(i))
        filteredTypes.Add(allTypes[i]);
}

Writing it this way, you can combine the two if statements in one simpler statement:

if(iNumberOfClasses >= 1 && classesForOne.Contains(i))
{
    filteredTypes.Add(allTypes[i]);
}

So the whole code with loop would now look like following:

var classesForOne = new [] { 0, 3, 6, 9, 12, 15, 18 };
var classesForTwo = new [] { 1, 4, 7, 10, 13, 16, 19 };
var classesForThree = new [] { 2, 5, 8, 11, 14, 17, 20 };

for(int i = 0; i < allTypes.Length; i++)
{
    if(iNumberOfClasses >= 1 && classesForOne.Contains(i))
        filteredTypes.Add(allTypes[i]);

    if(iNumberOfClasses >= 2 && classesForTwo.Contains(i))
        filteredTypes.Add(allTypes[i]);

    if(iNumberOfClasses == 3 && classesForThree.Contains(i))
        filteredTypes.Add(allTypes[i]);
}

Edit:

The mod implementation:

This edit is not mandatory, it is just provided as extra information to your secondary question about using the modulus. You CAN implement the modulus-way to achieve the same result. I'm not saying this is a better implementation, just showing how it can be done :) . Let's look at each of the three cases.

{ 0, 3, 6, 9, 12, 15, 18 }

If i is divisible by 3, meaning it has a remainder of 0, it is in the first collection of numbers.

{ 1, 4, 7, 10, 13, 16, 19 }

If i is divided by 3, remainder 1, it is in the second collection.

{ 2, 5, 8, 11, 14, 17, 20 }

And for the last, if i is divided by 3, remainder 2, it is in the third.

Of course using this method you're not validating if the number i is valid within the range. If for example i is 22 (remainder is 1), the result would say that it is present in the second collection. You'll have to build in an extra check for this. Here's the implementation:

for(int i = 0; i < allTypes.Length; i++)
{
    if(iNumberOfClasses >= 1 && (i <= 18 && i % 3 == 0))
        filteredTypes.Add(allTypes[i]);

    if(iNumberOfClasses >= 2 && (i <= 19 && i % 3 == 1))
        filteredTypes.Add(allTypes[i]);

    if(iNumberOfClasses == 3 && (i <= 20 && i % 3 == 2))
        filteredTypes.Add(allTypes[i]);
}

In this way, you don't need to predefine a collection, the counterpart is that you have to built an extra check to set the maximum allowed number. Hope this explains it. :)


Parsing complex string data I have a complex strings like the following:

$company=>Company(code->MyCompany)
$brand=>Brand(code->Brand_1, company->$company)

and then I prepare objects out of them as follows:

Macro [key=$company, type=Company, map={code=MyCompany}]
Macro [key=$brand, type=Brand, map={ company=$company, code=Brand_1}]

using the following code for parsing. Is there a better way?

public class Macro {

    private String key;

    private String type;

    private Map<String, String> map;
}

public static Macro getMacro(String str) {
    final Macro macro = new Macro();
    final Map<String, String> map = new HashMap<String, String>();
    final Matcher matcher = Pattern.compile("\\(([^)]+)\\)").matcher(str);
    while (matcher.find()) {
        final List<String> list = Arrays.asList(matcher.group(1).split(",")).stream()
                .collect(Collectors.toList());
        list.stream().forEach(a ->
        {
            final String[] ar = a.split("->");
            map.put(ar[0], ar[1]);
        });
    }
    str = str.replaceAll("\\(.*\\)", "");
    final String[] s = str.split("=>");
    macro.setMap(map);
    macro.setKey(s[0]);
    macro.setType(s[1]);
    return macro;
}

Pattern.compile("\\(([^)]+)\\)");

The Pattern is always the same so you may just as well make it a private static final constant. This will avoid having to recompile the pattern each time.

Arrays.asList(/*..*/).stream().collect(Collectors.toList());

Why this dance? Arrays.asList returns a List. You only do a stream on the return result.

    final List<String> list = Arrays.asList(matcher.group(1).split(","));
    list.stream().forEach(a ->
    {
        final String[] ar = a.split("->");
        map.put(ar[0], ar[1]);
    });

However a Pattern an return a Stream split according to the pattern directly:

Pattern COMMA_PATTERN = Pattern.compile("\\s*,\\s*");

COMMA_PATTERN.splitAsStream(matcher.group(3)).forEach(a ->
{
    //...
}

You can use a single match of a Pattern to find both the leading values and the string inside the parenthesis:

private static final Pattern FULL_PATTERN = Pattern.compile("^(\\$\\w+)\\s*=>\\s*(\\w+)\\s*\\(([^)]+)\\)$");
private static final Pattern KEY_VALUE_PATTERN = Pattern.compile("(\\w+)\\s*->\\s*([\\$\\w]+)");
private static final Pattern COMMA_PATTERN = Pattern.compile("\\s*,\\s*");

public static Macro getMacro(String str) {
    final Macro macro = new Macro();
    final Map<String, String> map = new HashMap<String, String>();
    final Matcher matcher = FULL_PATTERN.matcher(str);
    if(!matcher.matches()) {
        throw new IllegalArgumentException("...");
    }

    COMMA_PATTERN.splitAsStream(matcher.group(3)).forEach(a ->
    {
        final Matcher kvMatcher = KEY_VALUE_PATTERN.matcher(a);
        if(!kvMatcher.matches()){
             throw new IllegalArgumentException("...");
        }

        map.put(kvMatcher.group(1), kvMatcher.group(2));
    });
    macro.setMap(map);
    macro.setKey(matcher.group(1));
    macro.setType(matcher.group(2));
    return macro;

}

You'll note I added \\s* to various points in each pattern, This lets you ignore the whitespace that may surround the operators.

The \\w in a Pattern means a word character. If you only want alphanumeric +undescore then you'llneed to replace each instance of it with [\\p{Alnum}_]


Displaying a user's uploaded posts I'm working on prepared statements for my website and I'm wondering if it's possible to have multiple prepared statements one after another.  In this example, I have 2 select statements. I'm hoping that the question marks I have as placeholders won't be overwriting one another.  After that's been answered, I was wondering if this code was still open to SQL injection.

function getHomeUploads($conn) {
        $userName = $_GET["user"];
        $sqluserid = "SELECT * FROM users WHERE userName = ?;";
        $useridstmt = mysqli_stmt_init($conn);
        if (!mysqli_stmt_prepare($useridstmt, $sqluserid)) {
            echo "SQL statement failed";
        }
        else {
            mysqli_stmt_bind_param($useridstmt, "s", $userName);
            mysqli_stmt_execute($useridstmt);

            $useridresult = mysqli_stmt_get_result($useridstmt);
            while ($useridrow = mysqli_fetch_assoc($useridresult)) {

                if($useridrow['userid'] > 0) {
                    $userid = $useridrow['userid'];
                    $sqlusercontent = "SELECT * FROM posts WHERE hostid = ? AND userid = ? AND commentid = 0;";
                    $stmtusercontent = mysqli_stmt_init($conn);
                    if (!mysqli_stmt_prepare($stmtusercontent, $sqlusercontent)) {
                        echo "SQL statement failed";
                    }
                    else {
                        mysqli_stmt_bind_param($stmtusercontent, "ss", $userid, $userid);
                        mysqli_stmt_execute($stmtusercontent);
                        $usercontent = mysqli_stmt_get_result($stmtusercontent);
                        $postid = 1;

                        echo "
";
                        echo "
";
                        while ($row = mysqli_fetch_assoc($usercontent)) {
                            // Text fileid
                            if ($row['fileid'] == 1) {
                                echo "
";
                                echo "
";
                                echo $row['title']."
";
                                echo $row['date']."
";
                                echo $row['description']."
";
                                echo "
";
                                echo "
 •••
";
                                echo "
";
                            }
                            // Picture fileid
                            else if ($row['fileid'] == 2) {
                                $filename = "posts/".$userid."/".$postid."*";
                                $fileinfo = glob($filename);
                                $fileext = explode(".", $fileinfo[0]);
                                $fileactualext = $fileext[1];

                                echo "
";
                                echo "
";
                                echo $row['title']."
";
                                echo $row['date']."
";
                                echo "
";
                                echo $row['description']."
";
                                echo "
";
                                echo "
 •••
";
                                echo "
";
                            }
                            else {
                                echo '';
                            }

                            $postid++;
                        }
                        echo "
";
                        echo "
";
                    }
                }
            }
        }
    }
```

Passing the database connection as a variable is a good technique.
Validate user input as much as possible before considering executing any processes.  If the input is missing or invalid, don't do anything with the submission.

if (empty($_GET["user"])) { // add your validation here if possible
    echo "Missing/Invalid data submitted";
    // halt the script or redirect or whatever
}

Use object-oriented mysqli syntax because it is more brief and IMO cleaner to read.
Do not perform iterated queries.  You can easily condense you iterated queries into one simple query using an INNER JOIN.  Something like this should do (untested):

SELECT *
FROM users u
INNER JOIN posts p USING userid
WHERE u.user_name = ?
  AND p.hostid = p.userid
  AND p.commentid = 0

USING is a convenient shorthand keyword when the two joining columns are identical; otherwise use ON and declare the columns individually. MySQL ON vs USING? You should also replace * in the SELECT clause with the exact columns you need from either table.  (Never ask for more than you need.) With the above query, you will only need to bind $_GET"user"] once since there is now only one placeholder.
I don't typically endorse printing from the same function that does the processing.  If you are going to print nested html elements, you can improve readability by tabbing them.
There is a cleaner way to extract the file extension than picking up post-explosion debris.  How do I get (extract) a file extension in PHP?

$ext = pathinfo($filename, PATHINFO_EXTENSION);

If $row['fileid'] isn't 1 or 2, don't bother printing an empty string.


How best to keep track of an unknown number of objects? I have a manager class that will manage an unknown number of objects.  I'm currently storing the objects in a NSMutableArray object, and when I need to find one, I iterate through the array comparing pointers.

For example, I have an array called managedObjects.  I get a delegate method call:

- (void)someDelegateMethod:sender withResults:results {
    for (NSDictionary *managedObjectDict in managedObjects) {
        if (sender == [managedObjectDict objectForKey:@"DelegatedObject"]) {
            NSLog(@"%@",[managedObjectDict objectForKey:@"Value"]);
            break;
        }
    }
}

Now, Objective-C forin loops are handled in batches and are extremely fast... but if I were managing thousands of objects, this could potentially take a bit of time to find the right one.

How can I improve this efficiency?

I'm not an Objective-C guy but, I do know that if you used some sort of hash table structure and could define a hashcode for these objects, that you'd be able to pull them out much faster.

You said at times you "need to find one", I would assume you would know enough about it to be able to generate its hashcode and check your table for it in O(1) time.


A tiny switcher for arbitrary types in C++11 I was about to use the awful enum+map trick to switch on std::strings when I thought to see if I could let some variadic templates do the job for me and I started to play...

#include <string>
#include <iostream>
#include <functional>
#include <unordered_map>

template<typename Key = std::string, typename Fun = std::function<void(void)> >
class Switcher {
  std::unordered_map<Key,Fun> map;
  Fun fdefault;
 public:
  template<typename... O>
  Switcher(const Fun& f, const Key& k, const Fun& f2, const O&... o):
    Switcher(f2, o...) { map.emplace(k,f); }
  template<typename... O>
  Switcher(const Fun& f, const Key& k, const Key& k2, const O&... o):
    Switcher(f, k2, o...) { map.emplace(k,f); }

  Switcher(const Fun & f, const Key & k) { map.emplace(k,f); }
  Switcher(const Fun & f) { fdefault=f; }

  Switcher & operator()(const Key & k) {
    if (map.count(k)) map.at(k)();
    else if (fdefault) fdefault();
    return *this;
  }
};

int main() {
  std::string s1("hi");
  std::string s2("dear");
  Switcher<> switcher ( //whatever std::function here , all the keys here
                        [](){std::cout << "Hello ";  }, "hi",
                        [](){std::cout << "World ";  }, "dear", "buddy",
                        [](){std::cout << "Default ";}
                      );
  switcher(s1);
  switcher(s2);
  switcher("hi")("buddy")("?");
  return 0;
}

Of course is not the same as a switch statement, but it looks handy and for simple purposes it goes quite close, doesn't it?

While the idea is OK, this seems to add very little over a (unordered_)map of strings to functions (created directly). The client code is almost the same as your client code anyway:

int main() {
    typedef std::pair<std::string, std::function<void(void)>> pair;
    std::string s1("hi");
    std::string s2("dear");

    auto defaultFn = [](){ std::cout << "Default "; };

    std::map<std::string, std::function<void(void)>> switcher{
        pair{"hi", [](){ std::cout << "Hello "; } },
        pair{"dear", [](){ std::cout << "World "; } },
    }

    switcher[s1]();
    switcher[s2]();

    switcher["hi"](); // not supported: chaining calls ("buddy")("?");

    if(!switcher.count("buddy"))
        defaultFn();
    else
        switcher["buddy"]();
    return 0;
}

The code is a bit bigger, and some syntactic sugar is missing (stuff that you added in your example, like chaining calls and multiple keys per entry) but the code is semantically the same.

My point to adding this code: your class only makes sense if you use lots of switches (on strings) in your code, and usually in OOP, if you do have big switches in your code, you should extract them as a hierarchy of classes (or a table of functions) anyway.


Bad practices in this procedure to calculate player statistics? I recently self-taught myself MySQL as quickly as possible because it was needed for a project that I'm working on. Now that I've got some time I've been working on improving some thrown together SQL procedures. I've already done a few things, like logic improvements and removal of sub-queries, but I'm wondering if anyone can quickly skim through the procedure and point out anything specifically bad that I might have missed.

CREATE PROCEDURE `userstatsUpdate`(IN userauth VARCHAR(32))
BEGIN

DECLARE mapcount INT DEFAULT 0;
DECLARE stagecount INT DEFAULT 0;
DECLARE bonuscount INT DEFAULT 0;

DECLARE mapmult FLOAT DEFAULT 0;
DECLARE stagemult FLOAT DEFAULT 0;
DECLARE bonusmult FLOAT DEFAULT 0;

DECLARE pc FLOAT DEFAULT 0;

DECLARE stagerecs INT DEFAULT 0;
DECLARE bonusrecs INT DEFAULT 0;
DECLARE maprecs INT DEFAULT 0;
DECLARE tops INT DEFAULT 0;

DECLARE p_comps FLOAT DEFAULT 0;
DECLARE p_tops FLOAT DEFAULT 0;
DECLARE counter INT DEFAULT 0;
DECLARE ranknum INT DEFAULT 0;

DECLARE userID INTEGER DEFAULT 0;

SELECT playerID INTO userID FROM cs_players WHERE cs_players.userauth = userauth;

WHILE counter < 10 DO

    SET ranknum = (SELECT count(*) from cs_times INNER JOIN cs_maps ON cs_times.mapid=cs_maps.mapid WHERE stage = 0 AND rank = counter+1 AND playerid = userID AND active = 1);

    SET p_tops = p_tops + (((10-counter)+(10-counter+2)*30)*ranknum);
    SET counter = counter + 1;

END WHILE;

SELECT count(*),
COALESCE(
(count(*)*((difficulty+4)/5)),0)
AS points INTO mapcount, mapmult from cs_times INNER JOIN cs_maps ON cs_times.mapid = cs_maps.mapid WHERE stage = 0 AND playerid = userID AND cs_maps.active = 1;
SELECT count(*), COALESCE(
(count(*)*((difficulty+4)/5)),0)
 AS points INTO stagecount, stagemult  from cs_times INNER JOIN cs_maps ON cs_times.mapid = cs_maps.mapid WHERE stage > 0 AND cs_times.type = 0 AND playerid = userID AND cs_maps.active = 1;
SELECT count(*), COALESCE(
(count(*)*((difficulty+4)/5)),0)
AS points INTO bonuscount, bonusmult from cs_times INNER JOIN cs_maps ON cs_times.mapid = cs_maps.mapid WHERE stage > 0 AND cs_times.type = 1 AND playerid = userID AND cs_maps.active = 1;
SET maprecs = (SELECT count(*) from cs_times INNER JOIN cs_maps ON cs_times.mapid = cs_maps.mapid WHERE stage = 0 AND rank = 1 AND playerid = userID AND cs_maps.active = 1);
SET stagerecs = (SELECT count(*) from cs_times INNER JOIN cs_maps ON cs_times.mapid = cs_maps.mapid WHERE stage > 0 AND cs_times.type = 0 AND rank = 1 AND playerid = userID AND cs_maps.active = 1);
SET bonusrecs = (SELECT count(*) from cs_times INNER JOIN cs_maps ON cs_times.mapid = cs_maps.mapid WHERE stage > 0 AND cs_times.type = 1 AND rank = 1 AND playerid = userID AND cs_maps.active = 1);
SET tops = (SELECT count(*) from cs_times INNER JOIN cs_maps ON cs_times.mapid = cs_maps.mapid WHERE stage = 0 AND rank <= 10 AND rank > 0 AND playerid = userID AND cs_maps.active = 1);
SET @TotalStage = (SELECT sum(checkpoints) FROM cs_maps WHERE mtype = 0 AND active = 1);
SET @TotalBonus = (SELECT sum(cp_bonus) FROM cs_maps  WHERE active = 1);
SET @TotalMaps = (SELECT COUNT(*) FROM cs_maps WHERE active = 1);

SET @div1 = ((mapcount*2)+stagecount+(bonuscount*1.5));
SET @div2 = ((@TotalMaps*2)+@TotalStage+(@TotalBonus*1.5));

SET pc = @div1/@div2;

SET p_comps = (mapmult * 50) + (bonusmult * 20) + (stagemult * 5);

INSERT INTO cs_userstats(playerid, c_maps, c_stages, c_bonuses, wr_m, wr_s, wr_b, wr_top, p_top, p_comp, percomp) VALUES (userID, mapcount, stagecount, bonuscount, maprecs, stagerecs, bonusrecs, tops, p_tops, p_comps, pc)
ON DUPLICATE KEY UPDATE c_maps = mapcount, c_stages = stagecount, c_bonuses = bonuscount, wr_m = maprecs, wr_s = stagerecs, wr_b = bonusrecs, wr_top = tops, p_top = p_tops, p_comp = p_comps, percomp = pc;

END$$

the first thing that I see here is that it is going to be hard to read because you don't use standard ...  "notation" like indentation and new lines for each column you are selecting in a SELECT statement.  all of this is going to matter when you are going to be returning to the code to read it again or maintain it later.

where you have this:

WHERE stage = 0 AND playerid = userID

the playerid = userID part should really be in a join statement if possible.

some of your SET statements look very similar, you should look at these and see if you can't create a Temp Table with them, or even a CTE (I don't remember if this is possible with MySQL or not)


Filter out date records which don't fall within or span a date range I have records which have specific time spans. The need is to select records which adhere to the following three rules (read them as or conditions) concerning a target time span:

Record's Begin date can begin within the target time span and end date does not matter.
Record's End date falls with the target time span and the start date is inconsequential.
Record's dates actually span the target time.

So basically a query should return all records which start, end or span a target time frame.

Is there a more efficient way in Oracle 11g of doing the above rules than what I currently have stated in the SQL below?

I have created a test SQL Fiddle Test for the below schema and SQL

Schema

CREATE TABLE dates_test (
  id         NUMBER,
  hint       VarChar(20),
  BEGIN_DATE DATE,
  END_DATE DATE
);
-- If the span is Mar 6 to April 6 then 2, 3 and 4 are valid -
INSERT INTO dates_test VALUES (1, 'exclude before', TO_DATE('01-JAN-2014','DD-MON-YYYY'), TO_DATE('05-JAN-2014','DD-MON-YYYY'));
INSERT INTO dates_test VALUES (2, 'spans',          TO_DATE('05-MAR-2014','DD-MON-YYYY'), TO_DATE('08-SEP-2014','DD-MON-YYYY'));
INSERT INTO dates_test VALUES (3, 'Starts within',  TO_DATE('07-MAR-2014','DD-MON-YYYY'), TO_DATE('07-SEP-2014','DD-MON-YYYY'));
INSERT INTO dates_test VALUES (4, 'Ends within',    TO_DATE('01-JAN-2014','DD-MON-YYYY'), TO_DATE('07-MAR-2014','DD-MON-YYYY'));
INSERT INTO dates_test VALUES (5, 'exclude after',  TO_DATE('06-JUN-2014','DD-MON-YYYY'), TO_DATE('07-SEP-2014','DD-MON-YYYY'));
INSERT INTO dates_test VALUES (6, 'within OK',  TO_DATE('08-MAR-2014','DD-MON-YYYY'), TO_DATE('09-MAR-2014','DD-MON-YYYY'));

Here is the SQL which returns what I need but can it be improved?

select * from
dates_test t0
where
(  -- Starts Within range
  ( t0.BEGIN_DATE
    BETWEEN TO_DATE('03-06-2014', 'mm-dd-yyyy')
    AND     TO_DATE('04-06-2014', 'mm-dd-yyyy')
  )
OR  -- Ends within range
  (
    t0.END_DATE
    BETWEEN TO_DATE('03-06-2014', 'mm-dd-yyyy')
    AND     TO_DATE('04-06-2014', 'mm-dd-yyyy')
   )
OR    -- SPANS Range
   (
     t0.BEGIN_DATE < TO_DATE('03-06-2014', 'mm-dd-yyyy')
     AND
     t0.END_DATE > TO_DATE('04-06-2014', 'mm-dd-yyyy')
    )
);

Result:

| ID |          HINT |                     BEGIN_DATE |                         END_DATE |
|----|---------------|--------------------------------|----------------------------------|
|  2 |         spans |   March, 05 2014 00:00:00+0000 | September, 08 2014 00:00:00+0000 |
|  3 | Starts within |   March, 07 2014 00:00:00+0000 | September, 07 2014 00:00:00+0000 |
|  4 |   Ends within | January, 01 2014 00:00:00+0000 |     March, 07 2014 00:00:00+0000 |
|  5 |     within OK |   March, 08 2014 00:00:00+0000 |     March, 09 2014 00:00:00+0000 |

So, My first answer messed up (Can I blame it on incomplete test data?)

That's a good thing to note, that your should design your tests to cover the situations that are significant to you. Because your tests were incomplete, when I ran and modified your code, and got the 'same' results, I assumed the code was good. I never validated your test cases (my fault).

This re-work made me realize that your data would be better served with a negative test case. What you want to test for is exclusionary ranges. If you can assume your data is valid (it is, right), then you want to just test those cases where there is no intersection.

There is no intersection if the test date ends before the data's date, or the data's date ends before the test date. This simplifies the query to:

select *
from dates_test t0
where not (
      END_DATE < TO_DATE('03-06-2014', 'mm-dd-yyyy')
   or BEGIN_DATE > TO_DATE('04-06-2014', 'mm-dd-yyyy')

As was pointed out to me (very nicely, I may add), the above logic can be simplified further using 'basic' boolean logic to:

select *
from dates_test t0
where END_DATE >= TO_DATE('03-06-2014', 'mm-dd-yyyy')
  and BEGIN_DATE <= TO_DATE('04-06-2014', 'mm-dd-yyyy'))

which is my final answer (thanks @mjolka)

Using your (revised) SQL fiddle, the above query produces (note that your revised data has two records with id 5):

| ID |          HINT |                     BEGIN_DATE |                         END_DATE |
|----|---------------|--------------------------------|----------------------------------|
|  2 |         spans |   March, 05 2014 00:00:00+0000 | September, 08 2014 00:00:00+0000 |
|  3 | Starts within |   March, 07 2014 00:00:00+0000 | September, 07 2014 00:00:00+0000 |
|  4 |   Ends within | January, 01 2014 00:00:00+0000 |     March, 07 2014 00:00:00+0000 |
|  5 |     within OK |   March, 08 2014 00:00:00+0000 |     March, 09 2014 00:00:00+0000 |


Refactor a subroutine that divides date range into quarters. The subroutine below, GetDetailsQuarterly, accepts two dates. Then it writes out each quarter that falls within the date range. It also gives the first and last day of the quarter. I really don't like how the beginning and ending dates of each quarter are hardcoded. I don't believe this solution accounts for leap year as well. Could someone point me in the right direction to refactor this? Thanks in advance.

GetDetailQuarterly:

    Public Sub GetDetailsQuarterly(ByVal beginDate As Date, ByVal endDate As Date)

Console.WriteLine("{0}>>> date range is {1} - {2} {3}", Environment.NewLine, beginDate, endDate, Environment.NewLine)

Dim incrementingStartDate As Date = beginDate
Dim fy As New FiscalYear()

While incrementingStartDate <= endDate

    fy.qOneBegin = "1/1/" & incrementingStartDate.Year
    fy.qOneEnd = "3/31/" & incrementingStartDate.Year
    fy.qTwoBegin = "4/1/" & incrementingStartDate.Year
    fy.qTwoEnd = "6/30/" & incrementingStartDate.Year
    fy.qThreeBegin = "7/1/" & incrementingStartDate.Year
    fy.qThreeEnd = "9/30/" & incrementingStartDate.Year
    fy.qFourBegin = "10/1/" & incrementingStartDate.Year
    fy.qFourEnd = "12/31/" & incrementingStartDate.Year

    Select Case incrementingStartDate
        Case fy.qOneBegin To fy.qOneEnd
            fy.CurrentQuarter = 1
        Case fy.qTwoBegin To fy.qTwoEnd
            fy.CurrentQuarter = 2
        Case fy.qThreeBegin To fy.qThreeEnd
            fy.CurrentQuarter = 3
        Case fy.qFourBegin To fy.qFourEnd
            fy.CurrentQuarter = 4
        Case Else
            Console.WriteLine("out of range")
    End Select

    Console.WriteLine("your q{0}", fy.CurrentQuarter)
    fy.getCurrentQuarterRange()

    incrementingStartDate = incrementingStartDate.AddMonths(3)

End While
  End Sub

FiscalYear Class:

    Public Class FiscalYear

Public Property qOneBegin() As Date

Public Property qOneEnd() As Date

Public Property qTwoBegin() As Date

Public Property qTwoEnd() As Date

Public Property qThreeBegin() As Date

Public Property qThreeEnd() As Date

Public Property qFourBegin() As Date

Public Property qFourEnd() As Date

Public Property CurrentQuarter As Integer

Public Sub getCurrentQuarterRange()
    Select Case CurrentQuarter
        Case 1
            Console.WriteLine("s: {0}, e: {1}", qOneBegin, qOneEnd)
        Case 2
            Console.WriteLine("s: {0}, e: {1}", qTwoBegin, qTwoEnd)
        Case 3
            Console.WriteLine("s: {0}, e: {1}", qThreeBegin, qThreeEnd)
        Case 4
            Console.WriteLine("s: {0}, e: {1}", qFourBegin, qFourEnd)
        Case Else
            Console.WriteLine("out of range")
    End Select
End Sub
 End Class

I am a bit lost in terms of what you are actually trying to accomplish, but I can give some pointers.

First, leap years do not particularly impact quarters unless you truly have some unique rules. Under normal behaviors, February 29 will be in the same quarter as February 28, and would solidly fall inside quarter 1 in your given scenario.

Secondly, a quick way to figure out the quarter of a given date (if your quarter system starts at January 1) is the following formula

Dim currentQuarter as Integer = currentDateTime.Month \ 3 + 1

Similarly, you can perform operations to arrive at quarter beginning and ending dates.

Dim quarterStart = new DateTime(currentDateTime.Year, 3 * (currentQuarter - 1) + 1, 1)
Dim quarterEnd = quarterStart.AddMonths(3).AddSeconds(-1)

For example, using today as the base, you could test the code

Dim currentDateTime = DateTime.Now

Dim currentQuarter = currentDateTime.Month \ 3 + 1
Dim quarterStart = new DateTime(currentDateTime.Year, 3 * (currentQuarter - 1) + 1, 1)
Dim quarterEnd = quarterStart.AddMonths(3).AddSeconds(-1)

Console.WriteLine(currentQuarter)
Console.WriteLine(quarterStart)
Console.WriteLine(quarterEnd)

And see these results

2
4/1/2011 12:00:00 AM
6/30/2011 11:59:59 PM


Calculating the Nth number in the supertable of two numbers Below is the problem:

  Little Timmy is exceptionally good at math tables, so his teacher decided to make things a bit more interesting. His teacher gave him two numbers, A and B, and told him to merge the tables of A and B in order (ascending order), removing the duplicates and thus supertable of A and B, and asks Little Timmy the Nth number.  Given A, B and N, calculate the Nth number in the supertable of A and B.

Input

  First line contains number of test cases T . Each test case contains three integers A, B and N.

Output

  For each test case print the Nth number of the supertable.

Here is my code:

BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
    String line = br.readLine();
    int N = Integer.parseInt(line);

    SortedSet<Integer> sort=new TreeSet<>();
    for (int i = 0; i < N; i++) {
        String[] input=br.readLine().split(" ");
        if(input[0].equals(input[1])){
            System.out.println(Integer.parseInt(input[0])*Integer.parseInt(input[2]));
        }else{
        for(int i1=1;i1<Integer.parseInt(input[2]);i1++){
            sort.add(Integer.parseInt(input[0])*i1);
            if(sort.size()==Integer.parseInt(input[2]))
                break;
            sort.add(Integer.parseInt(input[1])*i1);
            if(sort.size()==Integer.parseInt(input[2]))
                break;
        }
        System.out.println(sort.toArray()[Integer.parseInt(input[2])-1]);
        }
    }

The problem is that my code is taking too long to run and I am not passing my test cases because of time constraints. How can I optimize this code so that it may run fast?

If I just look at the code, without looking at the algorithm, here's something you could optimize:

String[] input=br.readLine().split(" ");
if(input[0].equals(input[1])){
    System.out.println(Integer.parseInt(input[0])*Integer.parseInt(input[2]));
}else{
    for(int i1=1;i1<Integer.parseInt(input[2]);i1++){
        sort.add(Integer.parseInt(input[0])*i1);
        if(sort.size()==Integer.parseInt(input[2]))
            break;
        sort.add(Integer.parseInt(input[1])*i1);
        if(sort.size()==Integer.parseInt(input[2]))
            break;
    }
    System.out.println(sort.toArray()[Integer.parseInt(input[2])-1]);
}

What's up with all the parseInt? I have a feeling parseInt is very expensive.
So let's do that only once.

No matter what you do, you always need input[2] and input[0].
You only need input[1] once you reach the for loop, so I've moved the declaration near the for loop.

String[] input=br.readLine().split(" ");
int input0 = Integer.parseInt(input[0]);
int input2 = Integer.parseInt(input[2]);
if(input[0].equals(input[1])){
    System.out.println(input0*input2);
}else{
    int input1 = Integer.parseInt(input[1]);
    for(int i1=1;i1<input2;i1++){
        sort.add(input0*i1);
        if(sort.size()==input2)
            break;
        sort.add(input1*i1);
        if(sort.size()==input2)
            break;
    }
    System.out.println(sort.toArray()[input2-1]);
}

There's probably a greater optimization to be made on a algorithmic level, but this at least fixes one issue.


Basic request script I've just recently started running some wargames (Natas, OverTheWire) and thought I'd implement some of my scripting experience to solve them instead of doing it manually (though I did that first).

I've got this script:

import requests
from requests.auth import HTTPBasicAuth

def exploit(host, credentials):
    print(host)
    try:

        r = requests.get(host,
                         auth=HTTPBasicAuth(credentials[0],
                                            credentials[1]),
                         headers={'referer': 'http://natas5.natas.labs.overthewire.org/'}
                         )

        print([line for line in r.content.decode('ascii').split('\n') \
               if 'natas5' in line][0])

    except Exception as e:
        print(e)

def main():
    host = 'http://natas4.natas.labs.overthewire.org'
    credentials = ('natas4', 'Z9tkRkWmpt9Qr7XrR5jWRkgOU901swEZ')
    exploit(host, credentials)

if __name__ == '__main__':
    main()

I know it's a really basic script, but I'd really want to know if I'm following the right coding conventions and if there are ways of making a bit better. I've rarely used the requests library, so I'm still fairly new to it.

I would start with the following improvements:

better naming - for instance, r is not a good variable name. Remember that code is much more often read than written
avoid hardcoding constants - define them as proper Python constants (these are not really constants, but more like a guideline)
you can use next() built-in function instead of a list comprehension and then getting the first filtered item
you can unpack the credentials tuple into the HTTPBasicAuth arguments
avoid having a bare except clause - catch more specific exceptions if needed

Improved code:

import requests
from requests.auth import HTTPBasicAuth

REFERER = 'http://natas5.natas.labs.overthewire.org/'
KEYWORD = 'natas5'

def exploit(host, credentials):
    """TODO: docstring here"""
    try:
        response = requests.get(host,
                                auth=HTTPBasicAuth(*credentials),
                                headers={'referer': REFERER})

        response_lines = response.content.decode('ascii').split('\n')
        return next((line for line in response_lines if KEYWORD in line), 'Keyword not found')
    except requests.RequestException as e:
        print(e)

def main():
    host = 'http://natas4.natas.labs.overthewire.org'
    credentials = ('natas4', 'Z9tkRkWmpt9Qr7XrR5jWRkgOU901swEZ')
    print(exploit(host, credentials))

if __name__ == '__main__':
    main()


Function to check whether entire list is prime numbers I have a prime function which accepts a list of numbers and checks if all of the numbers in the list are prime.

Here is my code:

from math import sqrt
def primes(lista):
    return all(True if n == 2 else bool(all(n % x for x in list(range(3, int(sqrt(n)+1), 2))+ [2])) for n in lista)

Can I make this code more readable without making the actual function slower?

There are a few obvious improvements, which either make it more readable or even faster:

True if n == 2 else ... is equivalent to n == 2 or ...
bool(all(...)) is the same as all()
You could pull out the special case of 2 in all(n % x for x in list(range(3, int(sqrt(n)+1), 2))+ [2]) by doing (n % 2 and all(n % x for x in range(3, int(sqrt(n)+1), 2))). This has the advantage that the range does not get consumed into a list which makes this stop generating values as soon as a value is found not to be prime.

This gives:

from math import sqrt

def primes(lista):
    return all(n == 2 or (n % 2 and all(n % x for x in range(3, int(sqrt(n)+1), 2))) for n in lista)

But if you really want to make it more readable, you should factor out the prime check into another function:

from math import sqrt

def is_prime(n):
    """Check if `n` is prime.

    Uses exhaustive search to look for factors up to sqrt(n) + 1.
    """
    if n == 2:
        return True
    if n % 2 == 0:
        return False
    return all(n % x for x in range(3, int(sqrt(n) + 1), 2))

def all_primes(lista):
    """Check if all numbers in `lista` are primes."""
    return all(is_prime(n) for n in lista)

Another alternative is to compute a set of primes once and then just check if all of them are in this set:

primes_set = set(prime_sieve(100000))

def all_primes_set(lista):
    lista_set = set(lista)
    return len(primes_set & lista_set) == len(lista_set)

On my machine all of these are faster than your original function:

primes_list = list(prime_sieve(100000))

%timeit primes_op(primes_list)
77.3 ms ± 373 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

%timeit primes(primes_list)
67.8 ms ± 706 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

%timeit all_primes(primes_list)
70.9 ms ± 235 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

%timeit all_primes_set(primes_list)
823 µs ± 9.53 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

So if you know what the largest number is which you need to check, checking against a set that is computed only once is the fastest.


Express.js handler to update user fields I have an express handler which i thought was pretty simple, but CodeClimate flagged this method as having a Cognitive complexity of 6 (5 is the max by default without flagging something)

Curious how I could simplify this method to reduce Cognitive complexity.

function updateUser (req, res) {
  // Update any fields that were passed in.
  // Explicitly checking for undefined b/c passing null values should set them to null in the db
  if (req.body.firstName !== undefined) {
    req.user.firstName = req.body.firstName
  }

  if (req.body.lastName !== undefined) {
    req.user.lastName = req.body.lastName
  }

  if (req.body.email !== undefined) {
    req.user.email = req.body.email
  }

  if (req.body.phone !== undefined) {
    req.user.phone = req.body.phone
  }

  if (req.body.fax !== undefined) {
    req.user.fax = req.body.fax
  }

  if (req.body.notes !== undefined) {
    req.user.notes = req.body.notes
  }

  req.user.save()
    .then(user => {
      return res.status(HttpStatus.OK).send(user)
    })
    .catch(err => {
      req.log.error(err)
      return handleErr(res, HttpStatus.INTERNAL_SERVER_ERROR, err.message)
    })
}

Assignments can be contracted to something like below:

 req.user.firstName = req.body.firstName || req.user.firstName;
 req.user.lastName = req.body.lastName || req.user.lastName;
 req.user.email = req.body.email || req.user.email;
 req.user.phone = req.body.phone || req.user.phone;
 req.user.fax = req.body.fax || req.user.fax;
 req.user.notes = req.body.notes || req.user.notes;

I think it can further be reduced if properties are put in some array and iterated over (since source and destination property have the same name), but I would stick with this arguably more readable text.

I am not familiar with express.js, so the following might not be applicable (I usually work with Typescript in Angular).

Your request have both a body and some user property. It is recommended to clearly separate output from input in order to obtain pure functions as much as possible. Something like below:

 updateUserFromRequest(req, user) {

     // clone to avoid changing provided reference
     var ret = [... user];

     // assignment logic here using ret

     return ret;
 }


Adding an entry for a matched $module What would be the most efficient way to write the following code in Perl:

       my $index = 0;
       foreach ( @spec ) {
          if ( $module =~ m/$_/ ) {
         splice(@spec, $index, 0, $module);
             last;
          }
          $index++;
       }

This works fine but just seems a little wordy. The idea is that where I find a match for $module in the array I add an entry. I want to keep the array in a certain order and sorted.

You want "more efficient wordiness"??? I presume you're not asking for golfing, but for code that's more readable from conciseness.

@spec = map { $_ eq $module ? ($_, $_) : $_ } @spec;


Transpose of a matrix using Python 3.8 What is the transpose of a matrix:
In linear algebra, the transpose of a matrix is an operator which flips a matrix over its diagonal; that is, it switches the row and column indices of the matrix A by producing another matrix, often denoted by Aᵀ.
Code
dimension = int(input())
matrix = []
transpose = []
for row in range(dimension):
    entry = list(map(int,input().split()))
    matrix.append(entry)
for i in range(dimension):
    for j in range(dimension):
        transpose.append(matrix[j][i])
m = 0
n = dimension

for x in range(dimension+1):

    row = transpose[m:n]
    list1 = [str(item) for item in row]
    string2 = " ".join(list1)
    print(string2)
    m = n
    n = n + dimension

My question
What all modifications I can do to this code to make it better furthermore efficient?

You're converting the matrix elements to int and then back to string without doing anything with the ints, so that's just wasteful.
Anyway, the usual way to transpose is zip. Demo including mock input:
input = iter('''3
1 2 3
4 5 6
7 8 9'''.splitlines()).__next__

matrix = [input().split() for _ in range(int(input()))]

for column in zip(*matrix):
    print(*column)

Output:
1 4 7
2 5 8
3 6 9


Look over my code for PDO and MVC I'm building this website with PDO and Pattern MVC.  I'm currently working on the login page and would like to know if this structure is correct for MVC pattern.  I'd also like your opinions and advice on this.

For my login view I have:

<form action="test.php" name="loginform" method="post">
    <input type="text" name="login">
    <input type="password" name="password">
    <input type="submit" name="login_submit" value="Se connecter">
</form>

For my login controller I have:

if(isset($_POST['login_submit'])){
    if(!empty($_POST['login']) && !empty($_POST['password'])){
        $auth = new auth();
        $auth->setLogin($_POST['login']);
        $auth->setPassword(md5(sha1($_POST['password'])));
        if(($row = $auth->login('u_login,u_email,u_id_level', 'users', 'u_login', 'u_password'))){
            $_SESSION['back_office'] = array(
                'login' => $row->u_login,
                'email' => $row->u_email,
                'level' => $row->u_id_level
            );
        }else{
            message::showError('Compte non reconnu');
        }
    }else{
        message::showError('Veuillez remplir tous les champs');
    }
}

For my login model I have:

class auth{
    protected $rowUser;
    protected $login;
    protected $password;
    protected $email;

    public function setLogin($login){
        $this->login = $login;
    }
    public function setPassword($password){
        $this->password = $password;
    }
    /**
     * @return array $row
     */
    public function login($fields, $table, $col_login, $col_password){
        $query = Db::getInstance()->prepare('SELECT '.$fields.' FROM '.$table.' WHERE '.$col_login.' = :login AND '.$col_password.' = :password');
        $query->bindValue(':login',$this->login,PDO::PARAM_STR);
        $query->bindValue(':password',$this->password,PDO::PARAM_STR);
        $query->execute();
        if($query->rowCount() > 0){
            $row = $query->fetch();
            return $row;
        }else{
            return false;
        }
        $query->closeCursor();
    }
}

A part of code in auth::login() of method you should move to other class, for example 'db'. Because you class auth has too much responsibility. For example if you want to have dynamic define fields in class auth:

Db class:

class db {
    public function getItem($table, $fields = '*', $conditions = array()) {
        // you code ...
        return $row;
    }
}

Your model:

class auth {
    private $table = 'users';
    private $fields = 'u_login, u_email, u_id_level';
    private $fieldLogin = 'u_login';
    private $fieldPassword = 'u_password';

    public function setTable($value) { $this->table = $value; }
    public function setFields($value) { $this->fields = $value; }
    public function setFieldLogin($value) { $this->fieldLogin = $value; }
    public function setFieldPassword($value) { $this->fieldPassword = $value; }

    public function login() {
        $db = new db();
        return $db->getItem($this->table, $this->fields, array(
            $this->fieldLogin => $this->login,
            $this->fieldPassword => $this->password
        ));
    }
}

This code is more elastic and simpler will be change it.


Shortening code for string substitution I have this short function, it converts a human selected name like "Accelerometer" to "Acc" which a sensor can use:
private final String[] sensorNames = {"Acc", "Gyro", "Magn", "IMU9", "Temp"};

/**
 * Converts human representation of sensor names to movesense names. TODO: better code
 * @param sensorType Human representation of sensor name.
 * @return Short name of the sensor.
 */
private String findShortSensorName(String sensorType) {
    for (int i = 0; i < sensorNames.length; i++) {
        if (sensorType.substring(0,1).equals(sensorNames[i].substring(0,1))) {
            return sensorNames[i];
        }
    }
    return "";
}

Is there a way to make this more efficient or use less code? The return ""; is also bugging me. Thanks for your input!

It's hard to comment really well on this as your problem specification is more than a little vague.
Do you really want to do this based only on the first letter - is "AbominableSnowman" to be mapped to "Acc"?
Or do you know that the input sensor types are drawn from a specific set of values?
You could make your method (Java doesn't have functions) more succint, for example like this
private String findShortSensorName(String sensorType) {
  for (String sensorName : sensorNames) {
      if (sensorName.startsWith(sensorType.substring(0,1))) {
          return sensorName;
      }
  }
  return "";
}

However, the idiomatic approach for lookup in Java is usually to use a Map, and I'd have thought that if the value isn't found, throwing an Exception may be more appropriate than returning an empty string, though that will depend on your application. As your method doesn't look tied to a particular Java object, it can probably be "static", as well.
My version isn't shorter (it's considerably longer), but it's idiomatic (in my view) and easier to safely extend.
class BadSensorTypeException extends Exception {
// Thrown when a sensor type isn't found in the lookup table

  public BadSensorTypeException(String detailMessage) {
    super(detailMessage);
  }
 }

/**
 * lookup table mapping sensor types to short movesense names
 */
private static final Map<String, String> sensorNames = new HashMap<>();
static {
// static block to initialise the lookup table
  sensorNames.put("Accelerometr", "Acc");
  sensorNames.put("Gyroscope", "Gyro");
  // etc
}

/**
 * Converts human representation of sensor names to movesense names.
 * @param sensorType Human representation of sensor name.
 * @return Short name of the sensor.
 * @throws BadSensorTypeException if the sensor type isn't found
 */
private static String findShortSensorName(String sensorType) throws BadSensorTypeException {
  String shortName = sensorNames.get(sensorType);
  if (shortName != null) {
    return shortName;
  }
  throw new BadSensorTypeException(String.format("%s is not a valid sensor type", sensorType));
}


Verlet integration movement - doubt for on_key_press() methods Based on my understanding of Verlet integration I tryed to use it over my Euler method to move my character in a 2D space.
I will put only the neccessery code, but if anything else is needed I will edit my post.

This is my Character class (which I probably need to rename to Charactor)

class PlayerData(object):
    def __init__(self):
        self.applied_force = Vector(0, 0)
        self.mass = 70 /killograms
        self.position = Vector(15, 200)
        self.old_position = Vector(self.position.x, self.position.y)
        self.acceleration = Vector(self.applied_force.x / self.mass, 0)

    # These methods seems smelly
    def on_left_key_pressed(self):
        self.applied_force.x = -200
        self.old_position.x = self.position.x + 3
    def on_right_key_pressed(self):
        self.applied_force.x = 200
        self.old_position.x = self.position.x - 3
    def on_left_key_released(self):
        self.applied_force.x = 0
        self.old_position.x = self.position.x
    def on_right_key_released(self):
        self.applied_force.x = 0
        self.old_position.x = self.position.x
    def keys_up(self):
        self.applied_force = Vector(0, 0)
        self.old_position = Vector(self.position.x, self.position.y)

    def verlet(self, dt):
        self.acceleration = Vector(self.applied_force.x / self.mass, 0)
        temp = Vector(self.position.x, self.position.y)

        self.position.x += self.position.x - self.old_position.x + \
                           self.acceleration.x * dt * dt

        self.old_position = Vector(temp.x, temp.y)

    def update(self, dt):
        self.verlet(dt)
        // here be a collision detection method which I need to write
        self.rect.left = self.position.x
        self.rect.top = self.position.y

And this is the Event class which has 2 main methods - listener and handler

class EventManager(object):
    def __init__(self, player):
        self.player = player
        # few bools to indicate key states, the listener method toggles them

    def handler(self):
        ''' Calls Player on_x_key_pressed(), on_x_key_released()
        methods according to event bools set from the listener method.
        '''
        # Moving
        if self.left:
            self.player.on_left_key_pressed()
        elif self.right:
            self.player.on_right_key_pressed()
        else:
            self.player.on_keys_up()

I have jumping but I'm still trying to understand the right aproach to writing a proper verlet integration method.
I can controll how fast my object is moving by setting the self.old_position.x = self.position.x + 3 to self.old_position.x = self.position.x + 20 for example.
This also stops the movement, but I truly suspect that this is incorect or simply a very bad aproach.

EDIT: Redesigned the event handler method and shortened the bool names for simplicity.
I have no problems running the code, it is working as intented.
The thing I dont like is how my on_x_key_pressed() methods look, you can find them in PlayerData class.
I have the feeling that, to stop my character from continues move in some direction I have to set old_position.x = position.x.
Is this the right aproach to stop character movement?
And to control how fast the object is moving (the velocity), I have to manually set old_position to some value bigger/lower than position.x as seen in on_left_key_pressed() and on_right_key_pressed().
I wonder if there is better aproach to control the velocity?

def handler(self):
        '''
        Calls Player on_x_key_pressed(), on_x_key_released()
        methods according to event bools set from the listener method.
        '''
        # Moving
        if self.left_key_pressed and self.right_key_pressed:
            self.player.keys_up()

From this point, you know that both keys cannot be pressed at the same time, so you can continue with:

        elif self.left_key_pressed:
            self.player.on_left_key_pressed()
        elif self.right_key_pressed:
            self.player.on_right_key_pressed()
        else:
            self.player.keys_up()

It's up to you to see if you find it more easy to read.

I'm afraid I can't help much with the way you use verlet integration. Last comment: Try to avoid passing the key events around too much, or try to be more 'semantic', eg. send move_left to self.player instead of on_left_key_pressed.


Recording options selected from dropdown menu I am hoping someone can help me attain better performance and better ways of writing this script.  Any help would be awesome!

Overall Goal: I have a dropdown that allows someone to select an option from it.  It will add that record to the table below, and then the option will be removed.  If they delete that row in the table, it then adds that option back to the dropdown.

$(function() {
$('#addResource').on('change', function(){
var $selectBox = $(this),
    oID   = $selectBox.val(),
    oText = $selectBox.children(':selected').text(),
    url   = '/admin/contacts/json',
    oSize = $selectBox.children('option').size()
    noResources = '
No Resources.
';

//if select box out of options lets hide it
if((oSize-1) == 1) {
    $selectBox.hide().parent().append('
No More Contacts Available
');
}

// lets remove the selected option
$selectBox.children(':selected').remove();
$selectBox.children(':first').attr('selected','selected');

// lets get the contact to add to the table
$.post(url, { id: oID },function(data){
    var tableRow = "
<input type='hidden' value='"+data.id+"' name='resouces[]'>"+data.name+"	"+data.phone+"	"+data.email+"	$"+data.rate+"
",
        table = $("#resourcesTable tbody");

    table.find('tr.no-resources').remove();
    table.append(tableRow);

    cssClass = '.remove_'+data.id;
    table.on('click',cssClass, function(e){
        var $trash  = $(this),
            rowId  = $trash.attr('data-id');

        // open confirm modal window
        modalMsg('Are you sure you wish to delete '+data.name+' as a resource?','',function(result){
            // remove deleted row
            $(".resource_"+rowId).fadeOut(function(){
                var size = table.find('tr').size(),
                    name = $(this).find('.name').text();

                $selectBox.append("<option value='"+rowId+"'>"+name+"</option>"); // add option back to dropdown
                $(this).remove(); // remove the table row when deleted
                if((size-1) == 0 ){ // if no more talbe rows put back no resources message
                    table.append(noResources);
                }
            });

            // need to
        })
        e.preventDefault();
     });
    // need to add input fields to post to save to database
}, "json" );
}); // end on change
});

Your indentation is a little funky. I can understand not indenting after $(function () { (though I would do so myself), but the change handler's body really should be indented
You've got everything in one function, though it'd probably be more readable to separate things. For instance, addRecord, removeRecord (with further specialized functions for adding/removing a row, and adding/removing an option.
Use jQuery to build the HTML; don't type it out as a long string. Doing so is error-prone
I don't know how modalMsg works, but I imagine that the result you receive in the callback can have at least 2 different values. What happens if the user says "No, I don't want to delete that resource"? It looks like it gets removed anyway...
Moreover, the wording of that modal seems a bit severe considering you can just add the resource back in a moment later. The word "Delete" can scare the pants of people.
I have to wonder why your ajax call is a POST. I'm guessing you're updating something server-side when a record is added to the table, but in that case, shouldn't there also be a POST or DELETE call when the user removes something from the table?
You don't have any error handling, so if the ajax call should fail for any reason, the option is already gone, and the record isn't in the table either.
It'd be easy to add a little bit of caching, just in case someone changes their mind twice (adds, removes, adds again)
The hidden input should be in a td, rather than floating around inside a tr.
There's no need to create unique class names for every row. The idea of class names is that they can be applied to several elements. If you want to uniquely identify something, use an ID attribute - it's what they're for. However, in this case, we don't even need that, as we can use closures instead.
Rather than add and remove the no-resources row and the "No more contacts available" alert, it'd be easier to have these in the HTML already, and simply show/hide them as needed. The less HTML you have to build in JS, the better.
There's the issue of sorting the records, too. Not so much in the table, but rather in the dropdown. If I were to select the 3rd option out of, say, 10, and then change my mind, I'd expect it to behave like "undo": That's the option is back in the dropdown, as the 3rd option. But tackling this is a bit beyond the scope of this review.

I'd do something like this (assuming that the 1st option - the do-nothing option - has its value attribute set to an empty string). I haven't looked at error handling because I don't know how you'd prefer to do that.

Haven't tested this, but the general idea should be apparent

$(function () {
  var dropdown = $("#addResource"),
      table = $("#resourcesTable tbody"),
      noResourcesRow = table.find(".no-resources"),
      noContactsAlert = dropdown.parent().find(".alert"),
      url = "/admin/contacts/json",
      recordsCache = {};

  function fetchRecord(id) {
    if(recordsCache[id]) {
      addTableRow(records[id]); // use cache if possible
    } else {
      $.post(url, {id: id}, function (record) {
        recordsCache[id] = record; // add to cache
        addTableRow(record);
      });
    }
  }

  function addTableRow(record) {
    var row = $("

").addClass("resource").data("record", record),
        removeLink = $("", {
          "href": "#",
          "class": "remove-resource",
          "title": "Remove resource"
        }).append($("", { "class": "icon-trash" }));

    row.append($("	").text(record.name)).append($("<input></input>", {
      type: "hidden",
      value: record.id
    }));

    // this could be done with a loop too
    row.append($("	").text(record.phone));
    row.append($("	").text(record.email));
    row.append($("	").text(record.rate));

    row.append($("	").append(removeLink));

    removeLink.on("click", function (event) {
      event.preventDefault();
      modalMsg("Are you sure you wish to remove " + record.name + " as a resource?", "", function (result) {
        // FIXME: Add proper checking of the result
        if(...) {
          removeTableRow();
        }
      });
    });

    table.append(row);
  }

  function removeTableRow(row) {
    var record = row.data("record");
    row.fadeOut(function () {
      row.remove();
      if(!table.children.length) {
        noResourcesRow.show(); // or fadeIn() maybe
      }
      addOption(record.id, record.name); // add the option back in
    }
  }

  function addOption(id, name) {
    dropdown.append($("<option></option>", { value: id }).text(name));
    noContactsAlert.hide();
    dropdown.show();
  }

  function removeOption(option) {
    option.remove();
    if(dropdown.children.length === 1) { // includes the do-nothing option
      dropdown.hide();
      noContactsAlert.show();
    }
  }

  dropdown.on("change", function (event) {
    if(!this.value) {
      return; // ignore event if the do-nothing option is selected
    }

    fetchRecord(this.value);
    removeOption(dropdown.children(":selected"));
    this.value = ""; // set the do-nothing option
  });
});


recursive descent parser Here is a non-predictive recursive descent parser I wrote for the following grammar:

E : E1 '*' E
  | E1 '/' E
  | E1 '+' E
  | E1 '-' E
  | E1
  ;

E1 : NUM
   | '(' E ')'
   ;

It can be run against the token stream

func main() {
    // (m) * (m / (m))
    valid := Parse([]Token{
        {Value: "("},
        {Type: T_NUM},
        {Value: ")"},
        {Value: "*"},
        {Value: "("},
        {Type: T_NUM},
        {Value: "/"},
        {Value: "("},
        {Type: T_NUM},
        {Value: ")"},
        {Value: ")"},
    })

    println(valid)
}

It doesn't build an AST, it simply validates a token stream against the language.

// an enumeration of named token types. for
// this language, there is only one named
// token type (the number). all other token
// types are referred to by their string value
const (
    _ = iota
    T_NUM
)

// a token type refers to the terminal class of the
// token (eg T_NUM), whereas the value refers to
// its textual representation (eg "23")
type Token struct {
    Type  int
    Value string
}

// a slice of tokens that represents the input stack.
// the base pointer is passed to the production func
// that is matching against the stack.
type TokenStack []Token

// type definition for a production. given a stack and
// a base pointer, return the position at which the
// production ends. if no production is
// matched return NoMatch, or -1.
type Production func(TokenStack, int) int

// returned by a production func if no match  is found
// on the token stack at a given base pointer
const NoMatch = -1

// will return true if a given stack is valid for the
// language. a parse is valid when the return value from
// a production equals the length of the input stack
// (meaning the entire stack matches the production)
func Parse(s TokenStack) bool {
    p := E(s, 0)
    return M(p) && p == len(s)
}

// a production func for the rule:
//
//  E : E1 '*' E
//    | E1 '/' E
//    | E1 '+' E
//    | E1 '-' E
//    | E1
//    ;
func E(s TokenStack, p int) int {
    if p1 := Consecutive(s, p, E1, V("*"), E); M(p1) {
        return p1
    } else if p1 := Consecutive(s, p, E1, V("/"), E); M(p1) {
        return p1
    } else if p1 := Consecutive(s, p, E1, V("+"), E); M(p1) {
        return p1
    } else if p1 := Consecutive(s, p, E1, V("-"), E); M(p1) {
        return p1
    } else if p1 = E1(s, p); M(p1) {
        return p1
    }
    return NoMatch
}

// a production func for the rule:
//
//  E1 : T_NUM
//     | '(' E ')'
//     ;
func E1(s TokenStack, p int) int {
    if p1 := T(T_NUM)(s, p); M(p1) {
        return p1
    } else if p1 = Consecutive(s, p, V("("), E, V(")")); M(p1) {
        return p1
    }
    return NoMatch
}

// will try to match consecutive productions against
// a token stack, keeping track of the length of each
// match. if every consecutive rule matches, return
// the length of every match added to the base pointer
// originally passed to this function
func Consecutive(s TokenStack, p int, rules ...Production) int {
    var match_len = 0
    for _, rulei := range rules {
        if p1 := rulei(s, p+match_len); M(p1) {
            match_len += p1 - (p + match_len)
        } else {
            return NoMatch
        }
    }
    return p + match_len
}

// return a production func that will
// match a given token value
func V(v string) Production {
    return func(s TokenStack, p int) int {
        if len(s) > p && string(s[p].Value) == v {
            return p + 1
        }
        return NoMatch
    }
}

// return a production func that will
// match a given token type
func T(t int) Production {
    return func(s TokenStack, p int) int {
        if len(s) > p && s[p].Type == t {
            return p + 1
        }
        return NoMatch
    }
}

// a function to conveniently check the
// return value of a production for a match
func M(p int) bool {
    return p > NoMatch
}

Go fmt

First off, always go fmt your source code. The best thing about Go is that it always looks the same.

Godoc

Follow the godoc standard for comments. When commenting a function, start with the function name, capitalized, and end with a dot. Use proper grammar (like capital letter after punctuation). This makes automatically generated documentation look great, and further normalizes how Go code looks. TokenStack represents the input stack. is a good first sentance to start with.

(Actually, maybe TokenStack should be named InputStack, if that's what it really is?)

Const

Using iota to assign a single value like you are doing looks wierd. It's not that clear what you are doing.

const T_NUM = 1

or

const T_NUM = iota+1

or actually use the first value; make the nil value useful:

const (
    T_UNKNOWN = iota
    T_NUM
)

Usually, there is more than one constant. Since you're only using one, I'm wondering if it would be better to remove it as well. Every valid token is T_NUM, after all, so why have a constant for it in the first place?

Type

If you are going to use T_NUM as an enum, give it a type. Right now, T_NUM is just a constant integer. That means we can do weird things like T_NUM + T_NUM * 5. If we define a new type, such as type TokenType int and then make T_NUM a constant TokenType instead, we get a compililation error, since multiplying a TokenType by a number doesn't make sense. In many C-like languages, like C and C++, defining a new type like this wouldn't help, but in Go, types are real types, not type aliases.

Package main

You're starting the file with package main but there's no func main() in the source code. It looks like this is a library package, and in that case, the package name should be changed. The Go Blog has a great post about naming packages and things in packages.

NoMatch

It feels like NoMatch should be an error (for example `var NoMatch = errors.New("could not match TokenStack to known rules"). Right now it looks like C-code.

Exporting everything

Everything in your package is exported. You probably want some of the functions or types to be package-local. I'd aim for only exporting a single function: Parse. That way, it's super simple for someone to get started with the package. V and T should definitely not be exported as they stand, but if they have to be exported, they should probably be renamed.

Package testing only gets away with exporting T because that's almost the only thing it's exporting, and everytime you use it, you write testing.T. The name of the package helps explain what T is.

In-bound errors

Some of your functions return NoMatch on error, which is just -1. This is a common way of doing things in C or C++, but not in Go. It's usually better to return multiple values, the last one being an error, so that the caller can know how to check for errors without having to read the documentation first. Maybe all negative return values are errors, or just -1. What happens if the library updates and adds another possible return value; do we get subtile bugs? Is 0 an error? atoi is a good example. In C, there's no way of knowing if it encountered an error (or if it just parsed the string "0"). In Go, it returns (int, error).

if else if else if else if

The Go switch statement is very powerful and can be used instead of long if/elseif/elseif/else statements. If you omit the argument to switch it looks at the cases one by one until it finds one that is true, just like a long if/elseif/elseif statement does. In this case, however, since you need semicolons in your if-statements, switch does not work. Just a pointer for future code.


Check if two lists are permutations of the one another ( Improvement) Task: . Complete the following function that determines if two lists contain the same elements, but not necessarily in the same order. The function would return true if the first list contains 5, 1, 0, 2 and the
second list contains 0, 5, 2, 1. The function would return false if one list contains elements the other
does not or if the number of elements differ. This function could be used to determine if one list is a
permutation of another list. The function does not affect the contents of either list.

My code:

def permutation(a,b):
    if len(a) != len(b):
        return False
    n = len(a)
    count = 0
    for i in range(n):
        for j in range(n):
            if b[j] == a[i]:
                count += 1
    return count == n

def main():
    lst_1 = [1,2,3,4]
    lst_2 = [2,4,3,1]
    lst_3 = [2,4,4,5,4,5]
    lst_4 = [2,3,4,4,4,4]
    print(permutation(lst_1,lst_2))
    print(permutation(lst_2,lst_3))
    print(permutation(lst_3,lst_4))
main()

You can also do:

from collections import Counter

def compare_lists(list1, list2):
    return Counter(list1) == Counter(list2)

While list.sort / sorted has O(n log n) time complexity, constructing a Counter (which is a dict internally) is O(n). This is also an improvement over the solution in the question, which is O(n ^ 2).

There is also a more efficient memory allocation involved if you do not want to mutate list1 and list2. Constructing the sorted list has O(n) space complexity, while constructing the Counter is O(k), where k is the number of unique elements.

If you wanted to make this function scalable to an arbitrary number of lists, you could do:

from collections import Counter

def compare_lists(*lists):
    counters = map(Counter, lists)

    try:
        first_counter = next(counters)
    except StopIteration:
        return True

    return all(first_counter == counter for counter in counters)

Now compare_lists takes a variable number of arguments, and would return True if all of the arguments are permutations of each other.


MergeSort in Javascript I implemented the following mergesort algorithm in JavaScript, but I want to know your opinion about if is possible to do better than this.

function mergesort(list){
    if(list.length < 2){
        return list
    }else if(list.length > 1){
        let n = list.length
        let middle = n/2
        return merge(mergesort(list.slice(0, middle)), mergesort(list.slice(middle, n)))
    }
}

function merge(listA, listB){
    let list = []

    listA.push(Infinity)
    listB.push(Infinity)

    let i = 0
    let j = 0

    while(i < listA.length-1 || j < listB.length-1){
        if(listA[i] < listB[j]){
            list.push(listA[i])
            i++
        }else{
            list.push(listB[j])
            j++
        }
    }

    return list

}

Do you have any idea of how can I improve this?

I don't see a way to improve the algorithm, but here are some general coding tips:

A number can either be greater than 1, or less than 2. So you can use 'else' here instead.

if(list.length < 2){
    return list
}else if(list.length > 1){

Sometimes people create extra variables to improve readability, but here 'n' is less readable than list.length.

let n = list.length

A little bit of commenting can go a long way, for example if you could explain why you add 'Infinity' to the lists in a comment:

listA.push(Infinity)
listB.push(Infinity)


jQuery cleaner form submit function What's a better and more elegant way of writing this:

$('#footer form').submit(function(event) {
    event.preventDefault(event);
    $('#footer form .preload').spin(opts);
    $.post('contact.php', $(this).serialize(), function(response) {
        $('#footer form .preload div').remove();
        $('#footer form h5').remove();
        $('#footer form .error').html('');
        if (response == 1) {
            $('#footer form h3').after('
Your message has been sent!
');
            $('#footer form input[type=submit]').fadeOut();
        } else if (response == 0) {
            $('#footer form h3').after('
A problem occured. Please try again later.
');
            $('#footer form input[type=submit]').fadeOut();
        } else {
            var errors = $.parseJSON(response);
            $('label[for=name] .error').html(errors.name);
            $('label[for=email] .error').html(errors.email);
            $('label[for=message] .error').html(errors.message);
        }
    });
});

First thing I would do is cache some selectors, like the form:

$form = $('#footer form');

And then you can use it like:

$form.find('h3');


LeetCode: FloodFill C# https://leetcode.com/problems/flood-fill/

  An image is represented by a 2-D array of integers, each integer
  representing the pixel value of the image (from 0 to 65535).

  Given a coordinate (sr, sc) representing the starting pixel (row and
  column) of the flood fill, and a pixel value newColor, "flood fill"
  the image.

  To perform a "flood fill", consider the starting pixel, plus any
  pixels connected 4-directionally to the starting pixel of the same
  color as the starting pixel, plus any pixels connected 4-directionally
  to those pixels (also with the same color as the starting pixel), and
  so on. Replace the color of all of the aforementioned pixels with the
  newColor.

  At the end, return the modified image.

Example 1:
Input:
image = [[1,1,1],[1,1,0],[1,0,1]]
sr = 1, sc = 1, newColor = 2
Output: [[2,2,2],[2,2,0],[2,0,1]]
Explanation:
From the center of the image (with position (sr, sc) = (1, 1)), all pixels connected
by a path of the same color as the starting pixel are colored with the new color.
Note the bottom corner is not colored 2, because it is not 4-directionally connected
to the starting pixel.
Note:

The length of image and image[0] will be in the range [1, 50].
The given starting pixel will satisfy 0 <= sr < image.length and 0 <= sc < image[0].length.
The value of each color in image[i][j] and newColor will be an integer in [0, 65535].

Please review for performance and coding style.

using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace ArrayQuestions
{
    /// <summary>
    /// https://leetcode.com/problems/flood-fill/
    /// </summary>
    [TestClass]
    public class FloodFillTest
    {
        [TestMethod]
        public void ExampleTest()
        {
            int[][] image = { new[] { 1, 1, 1 }, new[] { 1, 1, 0 }, new[] { 1, 0, 1 } };
            int sr = 1;
            int sc = 1;
            int newColor = 2;
            int[][] expected = { new[] { 2, 2, 2 }, new[] { 2, 2, 0 }, new[] { 2, 0, 1 } };
            FloodFillDFS dfs = new FloodFillDFS();
            dfs.FloodFill(image, sr, sc, newColor);
            for (int i = 0; i < 3; i++)
            {
                CollectionAssert.AreEqual(expected[i],image[i] );
            }

        }
    }

    public class FloodFillDFS
    {
        public int[][] FloodFill(int[][] image, int sr, int sc, int newColor)
        {
            int oldColor = image[sr][sc];
            DFS(image, sr, sc, newColor, oldColor);
            return image;
        }

        //make sure to check first the corner cases
        private void DFS(int[][] image, int sr, int sc, int newColor, int oldColor)
        {
            if (sr < 0 || sc < 0 || sr >= image.Length || sc >= image[0].Length || image[sr][sc] == newColor || image[sr][sc] != oldColor)
            {
                return;
            }

            image[sr][sc] = newColor;
            DFS(image, sr - 1, sc, newColor, oldColor);
            DFS(image, sr + 1, sc, newColor, oldColor);
            DFS(image, sr, sc - 1, newColor, oldColor);
            DFS(image, sr, sc + 1, newColor, oldColor);
        }
    }
}

I would split the if to 3 variables and give each a name : out of image,  isVisited and isOriginalColor or something similar.

I don't know what sr means, I would use row and col.

FloodFill return image  but dfs change the original image, It can confuse the user of the class.