How should I read coordinates from a text file? I have a text file with lines that look like this:

Robot(479.30432416307934|98.90610653676828)
Robot(186.42081184420528|213.11277688981409)
Robot(86.80794277768825|412.1359734884495)

or, more general:

Robot(DOUBLE|DOUBLE)

How should I parse it in Java?

I have written the following (tested and working) code:

private static List<Position> getPositions(File myFile)
        throws FileNotFoundException, IOException {
    List<Position> result = new ArrayList();
    BufferedReader br = new BufferedReader(new FileReader(myFile));
    try {
        String line = br.readLine();
        while (line != null) {
            String coordinates = line.substring(6, line.length() - 1);
            int splitpoint = coordinates.indexOf('|');
            double x = Double.parseDouble(coordinates.substring(0,
                    splitpoint));
            double y = Double.parseDouble(coordinates
                    .substring(splitpoint + 1));
            result.add(new Position(x, y));
            line = br.readLine();
        }
    } finally {
        br.close();
    }
    return result;
}

It looks a too complicated for such a simple task for me. I mean, with the RegEx

Robot\(\d+\.\d+|\d+\)

I can match a line. According to Regextester my regex is correct. But how can I get the interesting part (the two doubles) from the regex?

Result after answer

/**
 * Read a file with positions of roboters.
 * @param file the file with positions
 * @return the list with roboter positions
 * @throws FileNotFoundException
 * @throws IOException
 */
private static List<Position> getPositions(final File file)
        throws FileNotFoundException, IOException {
    if (file == null || !file.canRead()) {
        throw new IllegalArgumentException("file not readable: " + file);
    }

    final Scanner s = new Scanner(file).useDelimiter("Robot\\(|\\||\\)\n?");
    final List<Position> positions = new ArrayList<Position>();
    while (s.hasNext()) {
        positions.add(new Position(s.nextDouble(), s.nextDouble()));
        s.nextLine();
    }

    return positions;
}

As this is posted on codereview, let us take a look at the whole function.

private static List<Position> getPositions(File myFile)
        throws FileNotFoundException, IOException {

I would suggest to do a check for the file (and rename it to file, I do not think there is a herFile, hisFile and so on):

    if (file == null || !file.canRead())
        throw new IllegalArgumentException("file not readable: " + file);

Which could make error handling a bit easier. We will see later.

    List<Position> result = new ArrayList();
    BufferedReader br = new BufferedReader(new FileReader(myFile));
    try {
        String line = br.readLine();
        while (line != null) {
        ...
            line = br.readLine();

I would keep the typical pattern, so everyone can recognize it:

        String line;
        while ((line = bufferedReader.readLine()) != null) {

            String coordinates = line.substring(6, line.length() - 1);
            int splitpoint = coordinates.indexOf('|');
            double x = Double.parseDouble(coordinates.substring(0, splitpoint));
            double y = Double.parseDouble(coordinates.substring(splitpoint + 1));
            result.add(new Position(x, y));
            line = br.readLine();
        }

For me, this is nearly fine. Just to a split("|") and you are done in the easy way:

            final String[] coordinatesArray = line.substring(6, line.length() - 1).split("|");
            final double x = Double.parseDouble(coordinatesArray[0]);
            final double y = Double.parseDouble(coordinatesArray[1]);
            result.add(new Position(x, y));

And I suggest to add an example in a comment, to help the next reader.

    } finally {
        br.close();
    }

If you have Java7, you could change this to try-with statement:

    try (final BufferedReader bufferedReader = new BufferedReader(new FileReader(file))) {
        ...
    }

And you could catch the error. The most problematic ones are cought by the file checks. Everything else is probably something you can not do anything about.
Then it is fine to catch it, show it and continue:

    } catch (NumberFormatException | IOException e) {
        e.printStackTrace(); // handle the exception or rethrow it. It could be safe to do nothing here
    }

If you put everything together, it could look like this:

private static List<Position> getPositions(final File file) {
    if (file == null || !file.canRead())
        throw new IllegalArgumentException("file not readable: " + file);

    final List<Position> result = new ArrayList<>();
    try (final BufferedReader bufferedReader = new BufferedReader(new FileReader(file))) {
        String line;
        while ((line = bufferedReader.readLine()) != null) {
            // example: Robot(479.30432416307934|98.90610653676828)
            final String[] coordinatesArray = line.substring(6, line.length() - 1).split("|");
            final double x = Double.parseDouble(coordinatesArray[0]);
            final double y = Double.parseDouble(coordinatesArray[1]);
            result.add(new Position(x, y));
        }
    } catch (NumberFormatException | IOException e) {
        e.printStackTrace(); // handle the exception or rethrow it. It could be safe to do nothing here
    }
    return result;
}

If you try the way with the scanner, you shoudl just tell him which characters to ignore. Please do not try a complex regular expression. They are unreadable.
What we have there?

  Robot(479.30432416307934|98.90610653676828)

Well, everything which is not a number or a point. To save some typing, we do not place every character alone, we put them together as \p{Alpha} (any alphabetic character): [\p{Alpha}()|\]. And we want this to happen for one or more times: [\p{Alpha}()|]+.
And we should not forget about multiple lines, then there is a newline: [\p{Alpha}()|\\n]+.

It is still mostly readable if you have a basic understanding of regular expressions, so we are fine in this case. You could also use [a-zA-Z()|\\n]+ which would be more common outside the java world.

The complete function could look like this:

private static List<Position> getPositions(final File file) {
    if (file == null || !file.canRead())
        throw new IllegalArgumentException("file not readable: " + file);

    final List<Position> result = new ArrayList<>();

    try (Scanner scanner = new Scanner(file)) {
        // example: Robot(479.30432416307934|98.90610653676828)
        scanner.useDelimiter("[\p{Alpha}()|\\n]+");
        while (scanner.hasNext())
            result.add(new Position(scanner.nextDouble(), scanner.nextDouble()));
    } catch (final FileNotFoundException e1) {
        e1.printStackTrace(); // handle the exception or rethrow it. It could be safe to do nothing here
    }
    return result;
}


Templated renderer class A class that matches the rendered objects shader and render function(sets shader resources) based on its typeid hash.

#include <unordered_map>

#include <glm/glm.hpp>

#include "gl_func.h"
#include "gl_buffer.h"
#include "shader.h"
#include "log.h"

class Renderer
{
private:

    typedef void (*RenderFunctionPointer)();

    typedef std::unordered_map< size_t, Shader* >::iterator ShaderIterator;
    typedef std::unordered_map< size_t, RenderFunctionPointer >::iterator
        FunctionIterator;

    std::unordered_map< size_t, Shader* > Shaders;
    std::unordered_map< size_t, RenderFunctionPointer > RenderFunctions;

    glm::mat4 ProjectionMatrix;
    glm::mat4 ViewMatrix;

public:

    // Sets the given matrices for all shaders
    void SetProjectionMatrix( glm::mat4 p );
    void SetViewMatrix( glm::mat4 v );

    /*! \brief Tells the renderer how to render object of type T
     *  \param shd Shader to use for rendering.
     *  \param f Pointer to a function that takes a shader and a T& argument. */
    template< typename T >
    void AddObjectType( Shader* shd, void (*f)(Shader*, T& ) )
    {
        size_t type = typeid( T ).hash_code();
        Shaders[type] = shd;
        RenderFunctions[type] = (RenderFunctionPointer)f;
    }

    /*! \brief Renders the given object.
     *  The object type has to be registered before this call.
     *  \param obj Object to Render */
    template< typename T >
    void Render( T& obj )
    {
        size_t type = typeid( T ).hash_code();

        ShaderIterator i = Shaders.find( type );

        if( i == Shaders.end() )
        {
            LOG_WARNING() << "Couldn't find shader for object: "
                << typeid( T ).name();
            return;
        }

        Shader* shd = i->second;
        shd->SetModelMatrix( obj.GetTransform() );
        shd->Bind();

        FunctionIterator f = RenderFunctions.find( type );
        if( f == RenderFunctions.end() )
        {
            LOG_WARNING() << "Couldn't find renderfunction for object: "
                << typeid( T ).name();
            return;
        }

        RenderFunctionPointer rp = f->second;
        reinterpret_cast< void(*)(Shader* shd, T& t) >(rp)( shd, obj );
    }
};

Here is some usage to clarify why is a reinterpret_cast necessary(this is not code to be reviewed, just rough example of usage):

// Init
DeferredRenderer->AddObjectType<Asset>(AssetShader.Get(),
   RenderFunctions::Deferred::RenderAsset );

// Somewhere passing a Asset to external C code( lua )
void* genericRenderable = MyAsset;
PassToLua( genericRenderable );

// Lua code calling entity system.
SetComponent( DeferredRenderable, genericRenderable );

// The render system processing entities
static_cast<Renderable*>(genericRenderable)->Render( DeferredRenderer );

// This calls
void Asset::Render( Renderer* r ) // where r is DeferredRenderer
{
    r->Render( *this );
}

Another approach, the first one to come to my mind actually, would be using a std::type_index as key in your unordered_map. That's exactly what type_index was designed for, and it is more specific about its purpose than a raw size_t integer.
BTW, size_t is a member of namespace std in C++, so if you are going to use it, make sure to properly qualify the type as std::size_t. The C++ standard does not require it to also be available in the global namespace. You should also include <cstddef> somewhere (the header that defines std::size_t). At the moment you rely on some other standard header that imports this dependency.
I really don't like this:

void (*f)(Shader*, T& )

reinterpret_cast< void(*)(Shader* shd, T& t) >(rp)( shd, obj );

First issue here is the duplicate typing of this function's signature. If you introduce a typo in any of the declarations, that could very well crash de program, because it would be using a different signature from the actual function. This can be improved with a local typedef. Second is the type cast from RenderFunctionPointer to the actual pointer type. It looks dangerous. Third, std::function would be a much better and type-safe option.

According to feedback from a comment, this approach was taken to account for different functions for different types of objects. The template type T can be anything, however the map has to store a known type.

Since you are already paying for the price of an indirect function call, then perhaps a cleaner approach would be to use polymorphism. Make the object type inherit from a Renderable interface that has a onRender(Shader & shd) virtual method. That would also remove the need to store function pointers into the renderer, cleaning up the interface as well. Instead of telling the renderer how to draw an object, let the object know how to draw itself.
You don't need these typedefs:

typedef std::unordered_map< size_t, Shader* >::iterator ShaderIterator;
typedef std::unordered_map< size_t, RenderFunctionPointer >::iterator FunctionIterator;

If your sole objective is to reduce typing in places like this:

ShaderIterator i = Shaders.find( type );

That is what auto is for (C++11).
I see that you have a macro wrapping your log system (LOG_WARNING()). However, even if you mute your log by changing an internal verbosity parameter, the log calls would still be evaluated. I'm assuming this will be used for software like video games, where you probably don't need the overhead of logging on a released title. So I would suggest that you change you logging macros a little bit to avoid evaluating the log calls if you ever wish to mute the log system permanently (such as in a release build):

#define LOG_WARNING(msg) do { MyLog::getInstance() << msg << "\n"; } while (0)

That's one possibility. Then if you need to disable it, just make the macro a no-op:

#define LOG_WARNING(msg)

Now when using the log, a small change:

LOG_WARNING("Couldn't find shader for object: " << typeid( T ).name());

For the muted/disabled log, the logging calls would not be evaluated at all, incurring no runtime cost.
A final comment: The ownership of the Shader instances added to the renderer via AddObjectType() if quite unclear. Who is supposed to free those pointers? The renderer? The caller? No one? In such situations, the best solution is almost always to use a smart pointer. Perhaps a std::shared_ptr would be adequate in this case, if ownership is meant to be shared. Otherwise, a std::unique_ptr would be the best course of action.


Relative prime numbers Two integers a and b are relatively prime if and only if there are no integers:

  x > 1, y > 0, z > 0 such that a = xy and b = xz.

I wrote a program that determines how many positive integers less than n are relatively prime to n.  But my program works too slowly because the number is sometimes too big.

My program should work for n <= 1000000000.

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

int main()
{
    long long int n;
    int cntr = 0, cntr2 = 0;
    cin >> n;
    if (!n) return 0;
    vector <int> num;
    for (int i = 2; i < n; i++)
    {
        if (n % i != 0)
        {
            if (num.size()>0)
            {
                for (int j = 0; j < num.size(); j++)
                {
                    if (i % num[j] != 0)
                        cntr2++;
                }
                if (cntr2 == num.size())
                    cntr++;
                cntr2 = 0;
            }
            else
                cntr++;
        }
        else
            num.push_back(i);
    }
    cout << cntr + 1 << endl;
}

In order to determine whether two numbers are co-prime (relatively prime), you can check whether their gcd (greatest common divisor) is greater than 1.
The gcd can be calculated by Euclid's  algorithm:

unsigned int gcd (unsigned int a, unsigned int b)
    {
      unsigned int x;
      while (b)
        {
          x = a % b;
          a = b;
          b = x;
        }
      return a;
    }

if gcd(x,y) > 1: the numbers are not co-prime.

If the code is supposed to run on a platform with slow integer division, you can use the so called binary gcd instead.


Bare bone implementation of Dijkstra I am trying to implement Dijkstra without writing lot of code for the Graph class itself. I have implemented it from Wikipedia. However, I am not fond of the way I have handled the minimum distance calculation. I would like to have some improvement on that.

graph = {'s': {'v':1,'w':4 },
         'v': {'w':2,'t':6},
         'w': {'t':3},
         't': {'': None}
         }
def dj(graph, start):
    Q = []# queue to hold all the vertices
    dist = {} #dictionary to hold all the distance
    prev = {} #dictionary to hold all the previous node visited

    for key in graph.keys():
        dist[key] = 1000
        prev[key] = ""
        Q.append(key)

    dist[start] = 0

    #I had to have another distance data structure dst
    # to store all the distance covered
    # I am not fond of it
    dst = []

    while Q:
        # Here I get the minimum distance
        u = min(dist, key = dist.get)
        # remove the min one from Q
        Q.remove(u)
        for v in graph[u]:
            #I also want to improvise this condition block
            #It is because of 't': {'': None}
            if graph[u][v] is None:
                continue
            alt = dist[u] + graph[u][v]
            if alt < dist[v]:
                dist[v] = alt
                prev[v] = u
        #I have to discard the minimum key from dist dictionary
        d = (dist.pop(u))
        # I then have to append the discarded one to dst dictionary
        # Redundant work
        dst.append(d)

    return dst,prev

print(dj(graph,'s'))

for key in graph.keys():
        dist[key] = 1000

Why 1000? There are two values which are easy to defend here: positive infinity (which is the value that an algorithms textbook would use), or half the maximum value of the datatype (to avoid overflow when adding two weights). Even the second one of those requires a comment to justify it.

        prev[key] = ""

Why not just leave it undefined?

        Q.append(key)

What is the purpose of Q? It just seems to duplicate dist.keys().

    #I had to have another distance data structure dst
    # to store all the distance covered

Why? It's not used anywhere that I can see.

        # Here I get the minimum distance
        u = min(dist, key = dist.get)

Hopefully you can see that this is quite inefficient, and that's the motivation for using more sophisticated data structures.

            #I also want to improvise this condition block
            #It is because of 't': {'': None}
            if graph[u][v] is None:
                continue

Why is it not just 't': {}?


Sorting an assortment of strings and integers together, while keeping them separate I put a solution to this coding problem together. The problem is this:

Create a function that takes an array, finds the most often repeated element(s) within it and returns it/them in an array. The function should work for both integers and strings mixed together within the input list (e.g. [1, 1, "a"]).
If there is a tie for highest occurrence, return both.
Separate integers and strings in the result.
If returning multiple elements, sort result alphabetically with numbers coming before strings.

This is the solution I came up with:
def highest_occurrence(arr)

  # Separate the unique values into individual sub-arrays
  x = rand(2**32).to_s(16)
  result = arr.sort do |a, b|
    a = a.to_s + x if a.is_a?(Numeric)
    b = b.to_s + x if b.is_a?(Numeric)
    a <=> b
  end.chunk_while {|a, b| a == b }.to_a

  # Get an array of all of the individual values with the max size,
  # Sort them by integers first, strings second
  result = result.select do |a2|
    a2.size == result.max_by(&:size).size
  end.map(&:uniq).flatten.sort_by { |v| v.class.to_s }

end

It passes these tests:
p highest_occurrence(["a","a","b","b"]) == ["a","b"]
p highest_occurrence([1,"a","b","b"]) == ["b"]
p highest_occurrence([1,2,2,3,3,3,4,4,4,4]) == [4]
p highest_occurrence(["ab","ab","b"]) == ["ab"]
p highest_occurrence(["ab","ab","b","bb","b"]) == ["ab","b"]
p highest_occurrence([3,3,3,4,4,4,4,2,3,6,7,6,7,6,7,6,"a","a","a","a"]) == [3,4,6,"a"]
p highest_occurrence([2,2,"2","2",4,4]) == [2,4,"2"]

I'd like to know whether there are better ways to solve some of the specific problems in this exercise. In particular, the requirement to sort strings and integers together without being able to convert the integers to strings in the sort block was an interesting one. I managed this by appending a random hex value (the same value) to each integer during the sort process. This seems a bit hackish, and I have the feeling it could be improved upon.
I would also appreciate any other suggestions for how to do a cleaner job.

Your test cases should include lexical sort of integers; that is, [9,11] must return [11,9].  (Your implementation does pass this test since you're converting everything to a string).

As you suspected, mangling the input is hacky.  This is better accomplished with a multi-criteria sort.  This technique maps each individual value to an array of sort criteria.  Ruby will compare the arrays only until it finds unequal elements; this means you can mix integers and strings in the second field, so long as the first field distinguishes them.

For our problem, the first field will be 0 for integers else 1.  The second field is the value itself.

Although this approach will never compare integers to strings—if the first criteria distinguishes the two elements, the comparison is done—the second criteria must be a string anyway, to satisfy the "sort alphabetically" requirement.

It's not necessary to sort the entire array or store a full copy of it.  Instead, count duplicates in a hash table.  Traverse the values of the hash to find a maximum.  Traverse again to extract the corresponding keys.

def highest_occurrence(arr)
    return arr if arr.size <= 1
    count = Hash.new(0)
    arr.each { |k| count[k] += 1 }
    max = count.max_by{|k,n| n}[1]
    return count.select { |k,n| n==max }.keys.sort_by { |k| [ k.is_a?(Integer)?0:1, k.to_s ] }
end


'Sort By' Function for Rebol I'm looking for a more consise way to sort a block of objects. Where you can sort a block of blocks based on the numeric position of the values to compare, there's no such shorthand for comparing objects by a given field:

test-data: reduce [
    object [name: "A" value: 1]
    object [name: "B" value: 3]
    object [name: "C" value: 2]
]

One solution would be to extend the native SORT function to accept a WORD! argument corresponding to the object field. However, that would require an update to the language and might take a little while to be accepted.

I've sketched out the following SORT-BY function that would take either a word (for the object field) or a block (an expression that is applied to the object):

sort-by: func [series [block!] comparator [block! word!]][
    forskip series 2 [
        insert series either word? comparator [
            all [
                in series/1 :comparator
                get in series/1 :comparator
            ]
        ][
            use [object] compose [
                object: first series
                (comparator)
            ]
        ]
    ]

    sort/skip series 2

    head forall series [remove series]
]

It's not ideal—it iterates through the block pulling the respective value from each object and adding it to the block; sorts the block; and removes the value again.

sort-by test-data 'value
sort-by test-data 'name
sort-by test-data [object/value]
sort-by test-data [sine 50 * object/value]

Any thoughts on the approach, particularly economy? Or even the need for such a function...

Obviously is lacking a descending/ascending switch—implied in future revision.

Any reason you can't use the SORTs /COMPARE refinement here?

Here is a working example of sort-by that uses it:

sort-by: function [
    series     [block!]
    comparator [block! word!]
  ][
    sort/compare series func [`a `b] either word? comparator [
        [`a/:comparator < `b/:comparator]
    ][
        sortie: function [by] [
            s: to-paren copy/deep comparator
            forall s [
                if all [path? s/1  s/1/1 = 'object] [s/1/1: by]
            ]
            s
        ]

        compose [(sortie '`a) < (sortie '`b)]
    ]
]


__str__ method for a tic-tac-toe board I am making my first object-oriented program in python and I am having trouble with the __str__ part of Board() class. I want it to be a visual representation of the tic-tac-toe board. I have achieved this but the code looks ridiculous and I know there must be a better way! Here is my project so far:

class Space(object):

    def __init__(self, state="x"):
        self.state = state

    state_names = ['empty', 'x', 'o']

    def __str__(self):

        return str(self.state)

    def change_state(self, string):
        if string == 'x':
            self.state = 'x'
        else:
            self.state = 'o'

class Board(object):

    def __init__(self):
        self.spaces = []
        for num in range(0,9):
            space = Space()
            self.spaces.append(space)
        board = self.spaces

    def __str__(self):
        res = []
        for space in self.spaces:
            res.append(str(space))
        return '   |   |'  + '\n ' + res[6] + ' | ' + res[7] + ' | ' + res[8] + '\n   |   |' + '\n-----------' + '\n   |   |' + '\n ' + res[3] + ' | ' + res[4] + ' | ' + res[5] + '\n   |   |' + '\n-----------' + '\n   |   |' + '\n ' + res[0] + ' | ' + res[1] + ' | ' + res[2] + '\n   |   |'

the default output should (and does) look like this:

   |   |
 x | x | x
   |   |
-----------
   |   |
 x | x | x
   |   |
-----------
   |   |
 x | x | x
   |   |

You can hugely reduce your __str__ using loops, e.g. using join() and a couple of generator expressions:

def __str__(self):
    return '\n-----------\n'.join('   |   |\n '+' | '.join(str(self.spaces[x]) for x in [0+y, 1+y, 2+y])+'\n   |   |' for y in [6, 3, 0])

Output:

   |   |
 x | x | x
   |   |
-----------
   |   |
 x | x | x
   |   |
-----------
   |   |
 x | x | x
   |   |


Test that all Python keywords are mentioned in a dictionary's values I have a dict of categories of Python keywords, with lists of keywords as the values:

Keywords_33=[('File_2', ['with', 'as']),
             ('Module_2', ['from', 'import']),
             ('Constant_3', {'bool': ['False', 'True'],
                             'none': ['None']}),
             ('Operator_4', {'boolean_operation': {'or', 'and', 'not'},
                             'comparison': {'is'}}),
             ('Container_operation_2', ['in', 'del']),
             ('Klass_1', ['class']),
             ('Function_7',['lambda', 'def', 'pass',
                            'global', 'nonlocal',
                            'return', 'yield']),
             ('Repetition_4', ['while', 'for', 'continue', 'break']),
             ('Condition_3', ['if', 'elif', 'else']),
             ('Debug_2', ['assert', 'raise']),
             ('Exception_3', ['try', 'except', 'finally'])]

I want to ensure that every keyword is in a category.  This code does that:

from keyword import kwlist
s = str(Keywords_33)
intersection_with_kwlist = [keyword for keyword in kwlist if keyword not in s]
print(intersection_with_kwlist)

Expected (and actual) output:

[]

This indicates that no keywords are missing from Keywords_33.

Can I make this code more elegant?

I'd personally use a generic function to flatten your datatype.

If an item in your collection in collections.Iterable then you will want to recurse through it.
If it's not you will want to yield it.
If it's a str or bytes you'll want to yield it, however this can change. And so you'll want to pass a tuple of these wanted types.
You probably want to only go through dictionary values as opposed to keys. And so you should pass a function to handle dictionaries.

And so I'd make:

def flatten(obj, dict_handle=dict.keys, wanted=(str, bytes)):
    def inner(obj):
        if isinstance(obj, dict):
            obj = dict_handle(obj)
        for el in obj:
            if isinstance(el, collections.Iterable) and not isinstance(el, wanted):
                yield from inner(el)
            else:
                yield el
    return inner(obj)

From this your code becomes:

from keyword import kwlist
import collections

unknown = set(kwlist) - set(flatten(Keywords_33, dict_handle=dict.values))
print(unknown)

If you don't want to remove 'File_2' from kwlist, then you can always change Keywords_33 to a dictionary:

unknown = set(kwlist) - set(flatten(dict(Keywords_33), dict_handle=dict.values))
print(unknown)


Aggregating search results from multiple databases, in smaller work units I have a large function that performs a search over a data set given a set of parameters. This function goes through multiple databases which all hold data that would comprise one final entry from the search.

def perform(self):
    #Go through each year and each DB in that year
    for year, yearInfo in data["preprocessDBInfo"].items():
        termsToCases = {}
        for dbName, dbInfo in yearInfo["dbs"].items():
            #Relevant to search?
            relevantTerms = self.search.ofDB(dbName)
            if len(relevantTerms) == 0:
                continue #Not a relevant database

            #Open the database and get cases
            fp = os.path.join(prefs["rootPath"], dbInfo["filePath"])
            caseDB = caseDB(fp)

            cases = caseDB.getCases(stubs=True,search=relevantTerms)
            termsToCases.update(cases)

        self.foundCases = self.foundCases.union(self.resolve(termsToCases))

Anyone who wants to call this function would have to wait for the entire search to complete which can take a couple of minutes depending on the search complexity.

Now I need to move this task to a separate thread of execution and be able to periodically check it and more drastically pause or kill it. In order to do this I need to break it up so that perform only does small work units so the caller of it has much more control over execution. I have come up with the following to solve this

def performGenerator(self):
    #Go through each year and each DB in that year
    for year, yearInfo in data["preprocessDBInfo"].items():
        for dbName, dbInfo in yearInfo["dbs"].items():
            #Relevant to search?
            relevantTerms = self.search.ofDB(dbName)
            if len(relevantTerms) == 0:
                continue #Not a relevant database

            #Open the database and get cases
            fp = os.path.join(prefs["rootPath"], dbInfo["filePath"])
            caseDB = caseDB(fp)
            cases = caseDB.getCases(stubs=True,search=relevantTerms)
            yield cases

Is there any sort of name for a function like this, a function that originally was large and autonomous but now only does tiny work units? (disregarding the fact that it's a generator in Python)
Is there any sort of pattern or methodology for breaking up a large subtask list this? Python's generator interface seems like it would be the best fit for my needs but I don't know what I would do if I didn't have such a high level interface (such as if I were programming in C)

You could split up your code further by separating out the lines where you actually get the cases to yield. It's only a few lines that need just a couple parameters to run and can return a simple variable, making them a prime candidate for refactoring into a function:

def performGenerator(self):
    #Go through each year and each DB in that year
    for year, yearInfo in data["preprocessDBInfo"].items():
        for dbName, dbInfo in yearInfo["dbs"].items():
            #Relevant to search?
            relevantTerms = self.search.ofDB(dbName)
            if len(relevantTerms) == 0:
                continue #Not a relevant database

            yield get_cases(relevantTerms, dbInfo["filePath"])

def get_cases(relevantTerms, filepath)
    fp = os.path.join(prefs["rootPath"], filepath)
    caseDB = caseDB(fp)
    return caseDB.getCases(stubs=True, search=relevantTerms)

This line is confusing me:

caseDB = caseDB(fp)

This isn't intended to modify caseDB, is it? As far as I can tell, it would modify it by replacing the name in your first case, but not the second since you're only modifying it inside the local scope each time the generator is called. It is a very bad idea to match names like this, because it makes it unclear whether or not they're supposed to be the same thing, or caseDB is an instance of a caseDB object. If this reuse of the name is necessary, it's a very confusing pattern that ought to be explicitly explained rather than just used without comment.

You shouldn't test for an empty collection using len(var) == 0. You're wasting time calculating the full length of something just to check if it's zero. Instead, use Python's truthiness. Any variable can be evaluated as a boolean in Python. For collections, this means that it's False if the collection is empty, and True otherwise. This means your test could be:

relevantTerms = self.search.ofDB(dbName)
if not relevantTerms:
    continue  # Not a relevant database

You don't actually use year, so you should throw it away. It's Pythonic to use an underscore for the name of a throwaway value, like this:

for _, yearInfo in data["preprocessDBInfo"].items():

This signals clearer that the value isn't used, which makes your code clearer to read.


Return quarters of current year I build this simple function, trying to use PEP 8 Style, something that I discovered recently thanks to this previous question.
The function works, but I was wondering if there is a better way to do it.
import datetime

def quarter_values_builder():
    """A function that returns an array
    with all the actual year quarters
    """
    now = datetime.datetime.now()
    year = now.year
    year_str = str(year)
    quarter_values = ["31/03/","30/06/","30/09/","31/12/"]
    quarter_values_now = [x+year_str for x in quarter_values]

    return quarter_values_now

current_quarters_string = quarter_values_builder()
print(current_quarters_string)

Without seeing the rest of your code it's difficult to say how this is used, but I think your function has conflated logic and formatting, and formatting is being done too soon. You should hold onto real date information right up until the edges of your program when that's possible. The following demonstrates one way of keeping dates and moving them to the current year.
from datetime import date, timedelta
from typing import Iterable, Tuple

QUARTER_DATES = (
    date(2000, 3, 31),
    date(2000, 6, 30),
    date(2000, 9, 30),
    date(2000, 12, 31),
)

def get_current_quarters() -> Iterable[date]:
    year = date.today().year
    for quarter in QUARTER_DATES:
        yield quarter.replace(year=year)

print('\n'.join(d.strftime('%d/%m/%Y') for d in get_current_quarters()))

dd-mm-YYYY is also an ambiguous and non-sortable format. Your usage (which you have not shown) will either need this to be "machine-readable" - e.g. for logs, REST payloads, etc. - or human-readable, e.g. for a UI. If you need machine readability, the ISO yyyy-mm-dd format is strongly preferred. If you need human readability, rather than hard-coding dd/mm/YYYY, your code should obey the current locale (not shown above).


A Get method for an API, with three modes Is this good way to create a get API method with different service methods?
Based on the scope parameter, it decides what service method in the API to call.

Enum:

    enum scopeValue
    {
          None,
          GetAll,
          GetWithDetails
    };

API:

   public class WorkItemController : BaseController
    {
        private IWorkItemService workItemService;

        public WorkItemController(IWorkItemService workItemService)
        {
            this.workItemService = workItemService;
        }

        [HttpGet]
        public async Task<IEnumerable<WorkItemDto>> Get(int id, string scope = "None")
        {
            scopeValue choice;

            Enum.TryParse(scope, out choice);

            if (choice == scopeValue.None)
            {
                return await workItemService.Get(id);
            }
            else if (choice == scopeValue.GetAll)
            {
                return await workItemService.GetAll();
            }
            else if (choice == scopeValue.GetWithDetails)
            {
                return await workItemService.GetAllWithDetails();
            }
        }
    }

Let's think about what your possible URIs would look like.

http://example.com/WorkItem?id=5
http://example.com/WorkItem?id=5&scope=GetAll

That's not how I'd expect the API to work. Typically, calling the resource without an id will return all of them.

// returns all work items
http://example.com/WorkItem

// returns work item by id
http://example.com/WorkItem/5

And then, to get the details for an item, we add an action to the end of the URI.

// returns details for specified item
http://example.com/WorkItem/details/5

To get the last part, you'll need to wire up a custom route, but it's not that difficult to do. You can then overload the Get methods to implement this. It's a much cleaner API to work with and, honestly, results in a much cleaner & maintainable controller.

I take it back, you won't need a custom route. MVC comes "out of the box" with a {controller}/{action}/{id} route, so for the Detail route, all you need to do is create the proper method.

[HttpGet]
public async Task<IEnumerable<WorkItemDto>> Details()

And

[HttpGet]
public async Task<IEnumerable<WorkItemDto>> Details(int id)

Will give you routes that look like this.

http://example.com/WorkItem/Details
http://example.com/WorkItem/Details/5


How can this async url response checker be cleaner/faster? with open('things.txt') as things:
    urls = [url.strip().lower() for url in things]

async def is_site_404(s, url):
    async with s.head(f"https://example.com/{url}") as r1:
        if r1.status == 404:
            print('hello i am working')

async def create_tasks(urls):
    tasks = []
    async with aiohttp.ClientSession() as s:
        for url in urls:
            if len(url) >= 5 and len(url) < 16 and url.isalnum():
                task = asyncio.create_task(is_site_404(s, url))
                tasks.append(task)
        return await asyncio.gather(*tasks)

while True:
    asyncio.get_event_loop().run_until_complete(create_tasks(urls))

Hi, this is a basic asynchronous url response checker that I created and it's pretty fast, but I'm wondering if there is any way to get more requests per second with the base of this code. I have it designed so that it runs forever and just prints whenever there is a 404 in this example basically. I'm pretty new to python and coding in general and I would like some guidance/advice from anyone who has more experience with this kind of thing.. maybe there is an aiohttp alternative I should use that's faster? ANY advice is greatly appreciated.

Overall, your code looks pretty decent. The functions are doing what they should be (create_task shouldn't be running the tasks as well), coroutines are gathered after aggregation.
I'd suggest a few things to make it more readable (and maintainable)
if __name__ block
Put script execution content inside the if __name__ == "__main__" block. Read more about why on stack overflow.
Variable naming
While you follow the PEP-8 convention on variable naming, the names still could use a rework, for eg. session instead of just s.
URL or path
URL refers to "Uniform Resource Locator", which is of the form:
scheme:[//authority]path[?query][#fragment]

You are dealing with only the path here, scheme and authority sections have been fixed as https://example.com/. This is again naming convenience.
Gather vs create
You are creating as well as gathering tasks in create_tasks function.
Type hinting
New in python-3.x is the type hinting feature. I suggest using it whenever possible.

Rewrite
import asyncio
import aiohttp

HOST = "https://example.com"
THINGS_FILE = "things.txt"

def validate_path(path: str) -> bool:
    return 5 <= len(path) < 16 and path.isalnum()

async def check_404(session: aiohttp.ClientSession, path: str):
    async with session.head(f"{HOST}/{path}") as response:
        if response.status == 404:
            print("hello i am working")

async def execute_requests(paths: list[str]):
    async with aiohttp.ClientSession() as session:
        tasks = []
        for path in paths:
            if validate_path(path):
                task = asyncio.create_task(check_404(session, path))
                tasks.append(task)
        return await asyncio.gather(*tasks)

def main():
    with open(THINGS_FILE) as things:
        paths = [line.strip().lower() for line in things]
    while True:
        asyncio.get_event_loop().run_until_complete(execute_requests(paths))

if __name__ == "__main__":
    main()

This can be further rewritten depending on the data being read from file, using a map/filter to only iterate over validated paths in file etc. The above is mostly a suggestion.


Leetcode 1275. Find Winner on a Tic Tac Toe Game I'm working on one easy question (Leetcode 1275. Find Winner on a Tic Tac Toe Game) and am wondering how I can make the code even cleaner.

class Solution:
    def tictactoe(self, moves: List[List[int]]) -> str:
        from collections import Counter

        def is_win(pos: List[List[int]]) -> bool:
            pos = set(tuple(i) for i in pos)
            d1, d2 = set([(0, 0), (1, 1), (2, 2)]), set([(0, 2), (1, 1), (2, 0)])
            x = Counter(i for i, _ in pos)
            y = Counter(i for _, i in pos)
            d1 -= set(pos)
            d2 -= set(pos)
            return (3 in x.values()) or (3 in y.values()) or not d1 or not d2

        a, b = moves[::2], moves[1::2]
        A, B = is_win(a), is_win(b)
        if A:
            return 'A'
        elif B:
            return 'B'
        elif not A and not B and (len(moves) == 9):
            return 'Draw'
        return 'Pending'

The second half of your code is inefficient, as it computers is_win(b) unnecessarily. It can be written as:

    if is_win(moves[::2]):
        return 'A'
    elif is_win(moves[1::2]):
        return 'B'
    elif len(moves) == 9:
        return 'Draw'
    else:
        return 'Pending'

This removes the redundant not a and not b, the redundant parentheses and the redundant computation of is_win in case A has won already.

When I saw is_win for the first time, I didn't understand it.

Maybe my chances had been better if you renamed d1 to diag1.
You could also write diag1 and diag2 on separate lines, to align their coordinates vertically.
You could also inline the diag1 and diag2 so that you don't need a name for them at all.

I'm thinking of:

def is_win(pos: List[List[int]]) -> bool:
    pos = set(tuple(i) for i in pos)
    horiz = 3 in Counter(r for r, _ in pos).values()
    verti = 3 in Counter(c for _, c in pos).values()
    diag1 = not (set([(0, 0), (1, 1), (2, 2)]) - pos)
    diag2 = not (set([(0, 2), (1, 1), (2, 0)]) - pos)
    return horiz or verti or diag1 or diag2

To avoid unnecessary computations here as well, you can let your IDE inline the variables except for pos.

Instead of not (set1 - set2), the expression set1.issubset(set2) is clearer because it avoids the confusing not:

    diag1 = {(0, 0), (1, 1), (2, 2)}.issubset(pos)
    # or:
    diag1 = {(0, 0), (1, 1), (2, 2)} <= pos

Or you could make checking the diagonals similar to horizontal and vertical:

    diag1 = 3 in Counter(r - c for r, c in pos).values()
    diag2 = 3 in Counter(r + c for r, c in pos).values()


d3.js dougnut pie chart legend toggling I'm developing a legend toggling d3.js pie chart application using this jsFiddle as my latest version.

I am aiming to get a streamlined working example where the legend can toggle the slices, trying to deactivate all slices - resorts in a reset which reactivates all the slices. Splitting up presentation and application layer logic.

Tweening needs improvement too - as the slices pop into existence then re-tween smoothly.

How do I improve/fix the various bugs in this code base?

onLegendClick: function(dt, i){
                    //_toggle rectangle in legend

var completeData = jQuery.extend(true, [], methods.currentDataSet);

                    newDataSet = completeData;
                    if(methods.manipulatedData){
                        newDataSet = methods.manipulatedData;
                    }

                    d3.selectAll('rect')
                    .data([dt], function(d) {
                        return d.data.label;
                    })
                    .style("fill-opacity", function(d, j) {
                        var isActive = Math.abs(1-d3.select(this).style("fill-opacity"));
                        if(isActive){
                            newDataSet[j].total = completeData[j].total;
                        }else{
                            newDataSet[j].total = 0;
                        }

                        return isActive;
                    });

                    //animate slices
                    methods.animateSlices(newDataSet);

                    //stash manipulated data
                    methods.manipulatedData = newDataSet;

                }

CodeReview is more about reviewing code than fixing various bugs, here goes my once over:

Use a beautifier, either TidyUp in jsfiddle or jsbeautifier.org, your code is badly indented and is hard to follow because of it
Use more meaningful names:

onLegendClick: function(dt, i) { <- I am guessing dt is data, i could be anything
.style("fill-opacity", function(d, j) <- Same here

Commenting is hard, only comment what is not obvious and the general purpose of code blocks

This is too obvious:

//animate slices
methods.animateSlices(newDataSet);

This is not, but there is no comment:

var isActive = Math.abs(1 - d3.select(this).style("fill-opacity"));

Not sure if this is a bug, or something I dont grasp, why are you referring to completeData even though you might be running of methods.manipulatedData, if this is correct then you should have a comment there

 newDataSet[j].total = completeData[j].total;

This could use shortcut syntax:

newDataSet = completeData;
if(methods.manipulatedData) {
  newDataSet = methods.manipulatedData;
}

becomes then

newDataSet = methods.manipulatedData || completeData;

This could also use a short cut syntax:

if(isActive) {
  newDataSet[j].total = completeData[j].total;
} else {
  newDataSet[j].total = 0;
}

becomes then

newDataSet[j].total = isActive ? completeData[j].total : 0;

Do not pollute the global namespace, newDataSet is used without a var declaration

All in all, I think you could have pasted more code to review, we tend to not review the code in the jsfiddle, only what you pasted. My counter proposal would look like this:

onLegendClick: function(dt, i) {

  var completeData = jQuery.extend(true, [], methods.currentDataSet),
      newDataSet = methods.manipulatedData || completeData;

  d3.selectAll('rect')
    .data([dt], function(d) {
      return d.data.label;
    })
    .style("fill-opacity", function(d, j) {
      //Witty comment on how this works
      var isActive = Math.abs(1 - d3.select(this).style("fill-opacity"));
      //Witty comment as to why I still need completeData
      newDataSet[j].total = isActive ? completeData[j].total : 0;
      return isActive;
    });

  methods.animateSlices(newDataSet);
  methods.manipulatedData = newDataSet;
}


Laravel photo uploader class I'm trying to separate logic from the controller, so I created this Upload class. It is not a big class, just a class so that the controller doesn't know about the logic and stuff like that.

<?php
namespace Dnianas\Uploads;

class Photo
{

    /**
     * The image filename.
     * @var string
     */
    protected $filename;

    /**
     * The image extension.
     * @var string
     */
    protected $extension;

    /**
     * The image hashed value that we hashed using SHA1.
     * @var string
     */
    protected $hashedName;

    /**
     * The filename with the hashed name and the extension.
     * @var string
     */
    public $path;

    public function makeProfilePicture($input)
    {
        $this->fileName     = $input->getClientOriginalName();
        $this->extension    = $input->getClientOriginalExtension();
        $this->hashedName   = sha1(time() . $this->fileName);
        $this->path         = $this->hashedName . '.' .$this->extension;

        // Resize the image
        $image = \Image::make($input);
        $image->fit(300, 300);

        // Set the destination
        $destination = photos_path() . '/' . $this->path;

        // Save the image
        $image->save($destination);

        // Return the object
        return $this;
    }

    public function makeCoverPhoto($input)
    {
        $this->fileName     = $input->getClientOriginalName();
        $this->extension    = $input->getClientOriginalExtension();
        $this->hashedName   = sha1(time() . $this->fileName);
        $this->path         = $this->hashedName . '.' .$this->extension;

        // Resize the image
        $image = \Image::make($input);
        $image->fit(900, 350);

        // Set the destination
        $destination = photos_path() . '/cover-' . $this->path;

        // Save the image
        $image->save($destination);

        return $this;
    }

}

As you can see, the first 4 lines from each method is exactly the same. How can I improve this?

EDIT: I've followed the constructor method. I added the constructor.

Here is the final class:

<?php
namespace Dnianas\Uploads;

use Symfony\Component\HttpFoundation\File\UploadedFile;

class Photo
{

    /**
     * The image filename.
     * @var string
     */
    protected $filename;

    /**
     * The image extension.
     * @var string
     */
    protected $extension;

    /**
     * The image hashed value that we hashed using SHA1.
     * @var string
     */
    protected $hashedName;

    /**
     * The filename with the hashed name and the extension.
     * @var string
     */
    public $path;

    /**
     * The input from the user, Usually it's the file.
     * @var object
     */
    public $input;

    public function __construct(UploadedFile $input)
    {
        $this->input        = $input;
        $this->fileName     = $this->input->getClientOriginalName();
        $this->extension    = $this->input->getClientOriginalExtension();
        $this->hashedName   = sha1(time() . $this->fileName);
        $this->path         = $this->hashedName . '.' .$this->extension;
    }

    public function makeProfilePicture()
    {
        // Resize the image
        $image = \Image::make($this->input);
        $image->fit(300, 300);

        // Set the destination
        $destination = photos_path() . '/' . $this->path;

        // Save the image
        $image->save($destination);

        // Return the object
        return $this;
    }

    public function makeCoverPhoto()
    {
        // Resize the image
        $image = \Image::make($this->input);
        $image->fit(900, 350);

        // Set the destination
        $destination = photos_path() . '/cover-' . $this->path;

        // Save the image
        $image->save($destination);

        return $this;
    }

}

And in the controllers I do this:

$profilePicture = (new Photo($input))->makeProfilePicture();

Add a constructor. I'm pretty sure you can type hint your dependency too, so long as you know what you're working with, you can adjust to your needs.

use Symfony\Component\HttpFoundation\File\UploadedFile;

class Photo {

    protected $fileName;
    protected $extension;
    protected $hashedName;
    protected $path;

    public function __construct(UploadedFile $input)
    {
        $this->fileName     = $input->getClientOriginalName();
        $this->extension    = $input->getClientOriginalExtension();
        $this->hashedName   = sha1(time() . $this->fileName);
        $this->path         = $this->hashedName . '.' .$this->extension;
    }

    // methods...

}

Please also note in your code you wrote protected $filename; then referenced back to that in your methods as $this->fileName - make sure you get the n or N consistent.

You can always make a getter for the path attribute instead of making it public keep it protected.

public function getPath()
{
    return $this->path;
}

Hope that helps!


Method to randomise a string's case I'm an amateur at programming and have created the following method with which I pass a lowercase string with the idea it is then returned either in it's original lowercase format, Capitalised or UPPERCASE. I'm not sure this is the best way or is truly "random" but does work up to a point but would like pointers on any improvements or different ways to achieve this.
        //==============================================================================
        // Randomise string case
        //==============================================================================
        private string randomCase (string word)
        {
            string changed;
            Random r = new Random(Guid.NewGuid().GetHashCode());
            int n = r.Next(1, 29);

            if (n >= 1 && n <= 9)
            {
                // First letter capatalised
                changed = char.ToUpper(word[0]) + word.Substring(1);
                return changed;
            }
            else if (n >= 10 && n <= 19)
            {
                // Word capatalised
                changed = word.ToUpper();
                return changed;
            }
            else if (n >= 20 && n <= 29)
            {
                // Left as lower case
                return word;
            }

            return null;
        }

This entire method should be static, as it clearly does not require an object
new Random(Guid.NewGuid().GetHashCode()) is needlessly complicated. new Random() is initialized based on time by default
A simpler range from [0,2] can be used.
The changed variable can be entirely replaced by a switch and return structure

private static string Capitalized(string word) {
    return char.ToUpper(word[0]) + word.Substring(1);
}

private static string RandomCase(string word)
{
    int choice = new Random().Next(3);
    switch(choice) {
        case 0: return Capitalized(word);
        case 1: return word.ToUpper();
        case 2: default: return word;
    }
}


Synchronization with remote web service I am writing a method to synchronize the local Core Data entities with a remote web service (in this case, Parse.com).

To update changed or created objects, I fetch all where the updatedAt date property is larger than the last local sync date. So far, so good.

But I am struggling with the deletion of local objects that have been removed on the server. My current approach is the following. This however seems quite expensive to me (\$O(r*l)\$ in the worst case, if I recall correctly). Is there any way to reduce the two loops?

PFQuery *idQuery = [PFQuery queryWithClassName:@"Product"];
[idQuery selectKeys:@[]];
NSArray *allRemote = [idQuery findObjects];
NSArray *allRemoteIds = [allRemote valueForKey:@"objectId"];

NSArray *allLocal = [Product all];
for(Product *p in allLocal) {
    BOOL shouldDelete = YES;
    for(NSString *remoteId in allRemoteIds) {
        if([remoteId isEqualToString:p.productId]) {
            shouldDelete = NO;
            break;
        }
    }

    if(shouldDelete) {
        [p delete];
    }
}

You can use NSPredicate:

NSArray* productsToDelete = [allLocal filteredArrayUsingPredicate:[NSPredicate predicateWithFormat:@"NOT (productId IN %@)", allRemoteIds]];
for(Product* product in productsToDelete) {
    [product delete];
}

This will for sure improve readability of your code. I'm not sure about performance, but I would assume that Apple's implementation of filteredArrayUsingPredicate should be fast enough. If you really want to know it, do some profiling.

You can also read great article about performance of collections in Objective-C:

http://www.objc.io/issue-7/collections.html


C++ OpenGL Debug Utility Edit: A follow up post can be found here.

So I've started a c++ project, coming from Java / C# there are many obvious differences.

Below is an example of a class I've been working on:

.h

#pragma once

#include <vector>

#include <GL/glew.h>
#include <glm/glm.hpp>

class GLDebug {
private:

    struct Line {
        glm::vec3 p0, p1;
    };

private:

    std::vector<Line> m_lines;

public:

    GLDebug();
    ~GLDebug();

public:
    void drawLine(const glm::vec3& p0, const glm::vec3& p1);

    void onRender(const glm::mat4& projMatrix, const glm::mat4& viewMatrix);
};

.cpp

#include "GLDebug.h"

#include <glm/gtc/type_ptr.hpp>

GLDebug::GLDebug() { }

GLDebug::~GLDebug() { }

void GLDebug::drawLine(const glm::vec3& p0, const glm::vec3& p1) {
    m_lines.push_back({ p0, p1 });
}

void GLDebug::onRender(const glm::mat4& projMatrix, const glm::mat4& viewMatrix) {
    glMatrixMode(GL_PROJECTION);
    glLoadMatrixf(glm::value_ptr(projMatrix));

    glMatrixMode(GL_MODELVIEW);
    glLoadMatrixf(glm::value_ptr(viewMatrix));

    glBegin(GL_LINES);

    for (auto& line : m_lines) {
        glVertex3fv(glm::value_ptr(line.p0));
        glVertex3fv(glm::value_ptr(line.p1));
    }

    glEnd();

    m_lines.clear();
}

I've written this basic OpenGL debug utility class based on one I wrote in Java previously. Now it seems fine from what I can tell, but with my limited C++ knowledge I'm wondering if anything I'm doing is redundant / unnecessary and/or performance impacting.

One other thing, I know I can use compiler directives such as
#ifdef _DEBUG etc. If I wanted this class to only exist if compiling in debug mode, Obviously I can't wrap the whole class in the ifdef as that would break other code that is calling functions in the GLDebug class. So I was thinking of wrapping the functions internals instead. Is that a bad approach, I could use them everywhere else as well but that seems kinda bloated and less manageable.

Just a couple points (also well done properly qualifying namespaces, a rare thing to see).

#pragma once is non-standard. Meaning it will probably work but is not guaranteed to. Bjarne himself recommends against using it.
Not following your multiple private and public approach. Usually you have a section for each without repeating the keywords.
It makes more sense to order your interface from public to private as people using it don't want to look at the internals of your class, they want to know what methods they can use.
You don't do anything in either your constructor or your destructor. In this case it makes sense to let the compiler take care of them.
Avoid declaring more than one variable per line


Usage flexibility of Deck class Previous review of this project:

Deck of cards program using a card value map

Based on the previous feedback I've received, my program could become more flexible (it formerly created a standard 52-card deck by itself).  Now, the user can add Cards to a Deck, and Card is no longer hidden.  I like this approach better (also easier to work with), but I'm sure it can still be improved.  Regarding its use, I have a few questions:

Although accessors aren't always recommended, would they still be necessary for allowing the driver to know each Card value/rank/suit (unless they're maintained elsewhere)?  For instance, Blackjack requires card numerical values for determining a bust.
Is the exception-handling in deal() effective (when called on an empty Deck), although it'll still need to return a Card?  This is assuming that the user will not always call empty() before calling deal().
Is this particular approach too flexible?  If so, what could be done instead?

Deck.h

#ifndef DECK_H
#define DECK_H

#include <iostream>
#include <cstdint>
#include <vector>

class Card
{
private:
    unsigned value;
    char rank;
    char suit;

public:
    Card(unsigned v, char r, char s) : value(v), rank(r), suit(s) {}
    friend std::ostream& operator<<(std::ostream &out, const Card &card)
        {return out << '[' << card.rank << card.suit << ']';}
};

class Deck
{
private:
    std::vector<Card> cards;
    int topCardPos;

public:
    Deck();
    void addCard(unsigned value, char rank, char suit);
    void shuffle();
    Card deal();
    std::size_t size() const {return topCardPos+1;}
    bool empty() const {return topCardPos == -1;}
    friend std::ostream& operator<<(std::ostream &out, const Deck &deck);
};

#endif

Deck.cpp

#include "Deck.h"
#include <algorithm>
#include <stdexcept>

Deck::Deck() : topCardPos(-1) {}

void Deck::shuffle()
{
    topCardPos = cards.size()-1;
    std::random_shuffle(cards.begin(), cards.end());
}

void Deck::addCard(unsigned value, char rank, char suit)
{
    cards.push_back(Card(value, rank, suit));
    topCardPos++;
}

Card Deck::deal()
{
    try
    {
        topCardPos--;
        return cards.at(topCardPos+1);
    }
    catch (const std::out_of_range &oor)
    {
        std::cerr << "\nDECK DEAL ERROR: " << oor.what() << "\n";
        Card blankCard(0, '*', '*');
        return blankCard;
    }
}

std::ostream& operator<<(std::ostream &out, const Deck &deck)
{
    for (unsigned i = 0; i < deck.size(); ++i)
    {
        out << "\n" << deck.cards[i];
    }

    return out;
}

(Possible driver)

Deck deck;

std::array<char, 13> RANKS = {'A','2','3','4','5','6','7','8','9','T','J','Q','K'};
std::array<char, 4> SUITS = {'H','D','C','S'};

for (unsigned rank = 0; rank < RANKS.size(); ++rank)
{
    unsigned value = (rank < 10) ? rank+1 : 10;

    for (unsigned suit = 0; suit < SUITS.size(); ++suit)
    {
        deck.addCard(value, RANKS[rank], SUITS[suit]);
    }
}

1: Yes, your Card class should expose its rank and suit through a public interface. (See the previous thread and @user225770's answer for value.) It should at least give read-only access, but write access may not be necessary. (It is probably better to create a copy than to alter a card.)

The simplest indication of this is your output stream operator overload. The fact that it needs access to these members is a good indication that other client code (i.e. code using this class) will need access as well.

2: It depends, but in either case it should be modified a bit. Exceptions are used for exceptional circumstances, scenarios that you don't expect to run into a lot. If it's rare that your deck runs out of cards, then throwing an exception is fine. However, you shouldn't throw an exception for your own function. Let the exception propagate to the calling code, and let them decide how to handle the exception.

However, if it's a common occurrence that your deck runs out of cards, you should use another approach. What immediately comes to mind is to simply reduce the chance of this happening by checking empty() before calling deal(). You can verify this behavior by adding an assert at the beginning of the function.

(What you are technically doing is defining a precondition for your function. In short, this means that your function requires and assumes that the deck is not empty when deal() is called.)

3: No, I don't think this solution is over-engineered. However, a more interesting question is "How can I tell if I have over-engineered my solution?". Why, thank you for asking! This leads me to my next, and perhaps most important point:

Start doing unit tests. You are writing good code. The best way for you to improve now is by doing test-driven development. By writing unit tests, several of your questions will answer itself. For example: Should my class expose this member through its interface? Unit tests will answer this question for you by letting you know what you need access to to test and use your code in unit tests.

Test-driven development will let you know if your code is over-engineered. You write a test that works as a requirement to your code. Do you need to do X to make the test pass? Then implement it. Don't you? Then You Ain't Gonna Need It (yet).


Extracting the first two components of a version number What's the most pythonic way to extract a substring by delimiter position?

i.e. something like slicing a string, but where the index is based on the delimiter position, rather than the character position in the string.

I have a version string "1.2.3.dev0" and I want to extract the first two components "1.2".

Currently I'm using:

>>> delimiter = "."
>>> version = "1.2.3.dev0"
>>> short_version = delimiter.join((version.split(delimiter)[0:2]))
>>> print(short_version)
1.2

Is this the simplest way to achieve this?

I can't think of a simpler way of doing the splitting.

The only thing that comes to mind is replacing [0:2] by [:2] to take the first two components of the version number, since the first index in the slice notation defaults to the beginning of a sequence object.

However, instead of using a string, you could use a namedtuple with integer elements to represent a version number, which would allow you to do comparisons more easily:

>>> from collections import namedtuple
>>> VersionNumber = namedtuple('VersionNumber', 'major minor')
>>> version_str = '1.2.3.dev0'
>>> version = VersionNumber(*map(int, version_str.split('.')[:2]))
>>> print(version)
VersionNumber(major=1, minor=2)
>>> if version.major < 2:  # don't care about minor version
...     print('Need at least version 2.')

You could easily convert it back to a string:

>>> '.'.join(map(str, version))
'1.2'


Vue.js search functionality In my BlogList.vue component I made a search input field:
<input type="text" placeholder="Enter key word  ..." v-model="search">

And a computed property:
computed: {
    getfilteredData() {
     return this.blogs.filter(blog =>
        blog.name.toLowerCase().includes(
          this.search.toLowerCase()
        ) ||
        blog.category.toLowerCase().includes(
          this.search.toLowerCase()
        )
      )
    }
  },

The user can search on multiple values, blog.name and blog.category, which updates the list accordingly.
But can I write the above JavaScript (computed property) in a cleaner way?

One improvement would be factoring out the repeated call to this.search.toLowerCase() and storing the result in a block-scoped constant so it:

doesn't need to be calculated on every call to .includes() for each iteration of filter()
can decrease the length of each line where that value is used

That may be a small optimization but it is in line with the D.R.Y. principle
getfilteredData() {
  const lowerTerm = this.search.toLowerCase()
  return this.blogs.filter(blog =>
    blog.name.toLowerCase().includes(lowerTerm) ||
    blog.category.toLowerCase().includes(lowerTerm)
  )
},

Additionally, it could further be simplified by using Object destructuring to eliminate the need for the identifier blog with the callback to the filter method:
getfilteredData() {
  const lowerTerm = this.search.toLowerCase()
  return this.blogs.filter({name, category} =>
   name.toLowerCase().includes(lowerTerm) ||
   category.toLowerCase().includes(lowerTerm)
  )
},


First program: a simple calculator I'm reading this very good book, C# Player's Guide, to learn C#. I then plan to make video games with Unity 3D after.

In one of the chapters, he asks us to make a (really) simple calculator as a console application using the switch statement:

  The program that we’ll make is going to be a simple calculator. We’re
  going to ask the user to type in two numbers and then type in a math
  operation to perform on the two numbers.

  Use a switch statement to handle the different operations in different
  ways. Allow the user to type in ’+’ for addition, ’-’ for subtraction,
  ’*’ for multiplication, ’/’ for division, and ’%’ for remainder.

What do you think of my first program? Is it well-formatted and well-written?

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace SimpleCalculator
{
  class Program
  {
    static void Main(string[] args)
    {
        //use for the string operation choice
        int operation = 0;
        //stock the result of the operation
        double result = 0;

        //Ask user first number
        Console.WriteLine("Type you first number :");
        string stringFirstNumber = Console.ReadLine();
        double firstNumber = Convert.ToDouble(stringFirstNumber);

        //Ask user second number
        Console.WriteLine("Type you second number :");
        string stringSecondNumber = Console.ReadLine();
        double secondNumber = Convert.ToDouble(stringSecondNumber);

        //Ask user operation to use
        Console.WriteLine("Enter the operation + (addition), - (soustraction), * (multiplication), / (division), ^ (exposant) or % (reste) :");
        string stringOperation = Console.ReadLine();

        // Convert string choice to integral
        if (stringOperation == "+" || stringOperation == "addition")
        {
            operation = 1;
        }
        else if (stringOperation == "-" || stringOperation == "soustraction")
        {
            operation = 2;
        }
        else if (stringOperation == "*" || stringOperation == "multiplication")
        {
            operation = 3;
        }
        else if (stringOperation == "/" || stringOperation == "division")
        {
            operation = 4;
        }
        else if (stringOperation == "^" || stringOperation == "exposant")
        {
            operation = 5;
        }
        else if (stringOperation == "%" || stringOperation == "reste")
        {
            operation = 6;
        }

        //Do someting depending on the operation choose
        switch (operation)
        {
            case 1:
                result = firstNumber + secondNumber;
                break;

            case 2:
                result = firstNumber - secondNumber;
                break;

            case 3:
                result = firstNumber * secondNumber;
                break;

            case 4:
                result = firstNumber / secondNumber;
                break;

            case 5:
                result = Math.Pow(firstNumber, secondNumber);
                break;

            case 6:
                result = firstNumber % secondNumber;
                break;
        }
        Console.WriteLine("\nResult of " + firstNumber + " " + stringOperation + " " + secondNumber + " = " + result + ".");
        Console.ReadKey();
    }
  }
}

First of all you are not verifying the input of your program. You also don't need the strings stringFirstNumber  stringSecondNumber you can just put this all into a method that will check if the input is in correct format and also return the value something like this :

    private static double SetNumber(string outputText)
    {
        double parse;
        Console.Write(outputText);
        string tempInput = Console.ReadLine();
        while (!double.TryParse(tempInput, out parse))
        {
            Console.WriteLine("Incorrect input !");
            Console.Write(outputText);
            tempInput = Console.ReadLine();
        }
        return double.Parse(tempInput);
    }

and use it like this :

        double firstNumber = SetNumber("Type you first number : ");
        double secondNumber = SetNumber("Type you second number: ");

Now, about the operations.. I'm not sure if you wanted the sign % to work as percentage because if that's the case it wont. This is called modulus you can read more about it here https://msdn.microsoft.com/en-us/library/0w4e0fzs.aspx if you want to have percentages you must implement this yourself. Anyway I would recommend creating an array of strings that holds all your currently available operations and store them there something like this :

private static readonly string[] operations = { "+", "-", "*", "/", "^", "%" };

Note that we are creating this variable outside of the static void Main(). Here we are seeing a few modifiers which might be new to you so let me quickly go over them.

private means we are keeping this variable only in our current class (Program)
static means we are having just 1 instance of this variable.
readonly means we wont modify those variables it's similar to const but it gives us a little bit more flexibility of where we can declare the value of the variable.

If the modifiers confuse you just remove them :

static string[] operations = { "+", "-", "*", "/", "^", "%" };

You need the static one since we are using this variable in static methods.

Let's move on. Once we have our operations declared in a string array we can easily check if the user is entering a correct input by creating the following function :

    private static bool IsValidOperation(string input)
    {
        return operations.Contains(input);
    }

Well we have the checker method which uses LINQ.Contains() this extension checks if a specific collection contains a specific value. Now we just need a method that will return a string value to our stringOperation

    private static string SetOperation(string outputText)
    {
        Console.Write(outputText);
        string tempInput = Console.ReadLine();
        while (!IsValidOperation(tempInput))
        {
            Console.WriteLine("Incorrect input !");
            Console.Write(outputText);
            tempInput = Console.ReadLine();
        }
        return tempInput;
    }

and use like this

string stringOperation =
          SetOperation(
               "Enter the operation + (addition), - (soustraction), * (multiplication), / (division), ^ (exposant) or % (reste) :");

Now notice that we are using only string's. I removed the integer idea that you had initially. You can still implement it pretty easily working around with the indexes of the string array, but I don't this is necessary. Also one thing to mention is that we are again taking a parameter in our method SetOperation well we could've avoided that since we have just 1 operation to be entered so we could've just do

Console.Write("Enter the operation + (addition), - (soustraction), * (multiplication), / (division), ^ (exposant) or % (reste) :");

Instead of Console.Write(outputText);. But I used the same parameters in the previous method too so it's good to keep some consistency if you are still learning. We are almost done now all that's left is the big mess with if/else if statements and a switch case too .. So as I said earlier using integer and string is useless so I went just for a string. Combining both the switch case and the if/else if statements into this :

        switch (stringOperation)
        {
            case "+":
            case "addition":
                result = firstNumber + secondNumber;
                break;
            case "-":
            case "soustraction":
                result = firstNumber - secondNumber;
                break;
            case "*":
            case "multiplication":
                result = firstNumber*secondNumber;
                break;
            case "/":
            case "division":
                result = firstNumber/secondNumber;
                break;
            case "^":
            case "exposant":
                result = Math.Pow(firstNumber, secondNumber);
                break;
            case "%":
            case "reste":
                result = firstNumber%secondNumber;
                break;
        }

I prefer the switch case version here because it's more readable but you can go with if statements too

        if (stringOperation == "+" || stringOperation == "addition")
        {
            result = firstNumber + secondNumber;
        }
        else if (stringOperation == "-" || stringOperation == "soustraction")
        {
            result = firstNumber - secondNumber;
        }
        else if (stringOperation == "*" || stringOperation == "multiplication")
        {
            result = firstNumber*secondNumber;
        }
        else if (stringOperation == "/" || stringOperation == "division")
        {
            result = firstNumber/secondNumber;
        }
        else if (stringOperation == "^" || stringOperation == "exposant")
        {
            result = Math.Pow(firstNumber, secondNumber);
        }
        else if (stringOperation == "%" || stringOperation == "reste")
        {
            result = firstNumber%secondNumber;
        }

Notice that we don't have else/default at the end in case of wrong input, because we already made sure the nasty user wont be able to proceed unless he sticks to our rules.

And lastly I edited this line

Console.WriteLine("\nResult of " + firstNumber + " " + stringOperation + " " + secondNumber + " = " + result + ".");

into a formated string which also looks a lot more readable :

Console.WriteLine("Result of {0} {1} {2} = {3}", firstNumber, stringOperation, secondNumber, result);

Here's the full code if you got confused where to put something :

    private static readonly string[] operations = { "+", "-", "*", "/", "^", "%" };
    static void Main(string[] args)
    {
        double result = 0;
        double firstNumber = SetNumber("Type you first number : ");
        double secondNumber = SetNumber("Type you second number: ");

        string stringOperation =
            SetOperation(
                "Enter the operation + (addition), - (soustraction), * (multiplication), / (division), ^ (exposant) or % (reste) :");

        switch (stringOperation)
        {
            case "+":
            case "addition":
                result = firstNumber + secondNumber;
                break;
            case "-":
            case "soustraction":
                result = firstNumber - secondNumber;
                break;
            case "*":
            case "multiplication":
                result = firstNumber*secondNumber;
                break;
            case "/":
            case "division":
                result = firstNumber/secondNumber;
                break;
            case "^":
            case "exposant":
                result = Math.Pow(firstNumber, secondNumber);
                break;
            case "%":
            case "reste":
                result = firstNumber%secondNumber;
                break;
        }
        Console.WriteLine("Result of {0} {1} {2} = {3}", firstNumber, stringOperation, secondNumber, result);
        Console.ReadKey();
    }

    private static double SetNumber(string outputText)
    {
        double parse;
        Console.Write(outputText);
        string tempInput = Console.ReadLine();
        while (!double.TryParse(tempInput, out parse))
        {
            Console.WriteLine("Incorrect input !");
            Console.Write(outputText);
            tempInput = Console.ReadLine();
        }
        return double.Parse(tempInput);
    }

    private static bool IsValidOperation(string input)
    {
        return operations.Contains(input);
    }

    private static string SetOperation(string outputText)
    {
        Console.Write(outputText);
        string tempInput = Console.ReadLine();
        while (!IsValidOperation(tempInput))
        {
            Console.WriteLine("Incorrect input !");
            Console.Write(outputText);
            tempInput = Console.ReadLine();
        }
        return tempInput;
    }


Find the smallest positive number missing from an unsorted array this is the original question
https://www.geeksforgeeks.org/find-the-smallest-positive-number-missing-from-an-unsorted-array/
You are given an unsorted array with both positive and negative elements. You have to find the smallest positive number missing from the array in O(n) time using constant extra space. You can modify the original array.

Input: {2, 3, 7, 6, 8, -1, -10, 15}   Output: 1
Input:  { 2, 3, -7, 6, 8, 1, -10, 15 }  Output: 4
Input: {1, 1, 0, -1, -2}                Output: 2

using System;
using System.Collections.Generic;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace ArrayQuestions
{

[TestClass]
public class FindTheSmallestNonnegativeNumberNoyInArray
{
    [TestMethod]
    public void FindTheSmallestNonnegativeNumberNoyInArrayTest()
    {
        int[] array = {2, 3, 7, 6, 8, -1, -10, 15};
        int result = FindSmallestPositiveHash(array);
        int expected = 1;
        Assert.AreEqual(expected, result);
    }

   [TestMethod]
    public void FindTheSmallestNonnegativeNumberNoyInArrayTest2()
    {
        int[] array = { 2, 3, -7, 6, 8, 1, -10, 15 };
        int result = FindSmallestPositiveHash(array);
        int expected = 4;
        Assert.AreEqual(expected, result);
    }
    private int FindSmallestPositiveHash(int[] array)
    {
        HashSet<int> set = new HashSet<int>();
        int maxPositive = 0;
        for (int i = 0; i < array.Length; i++)
        {
            if (!set.Contains(array[i]))
            {
                maxPositive = Math.Max(maxPositive, array[i]);
                set.Add(array[i]);
            }
        }
        for (int i = 1; i < maxPositive; i++)
        {
            if (!set.Contains(i))
            {
                return i;
            }
        }
        return maxPositive + 1;
    }
}
}

I know there is a better solution, I tried to implement the hash based solution please comment or runtime and performance.
Thanks

There are a few things you can do to simplify your code:

There's no need to keep track of the maximum positive integer if you remove the condition from the second for loop: for (int i = 1; ; i++).
That means you don't need to check whether the hash already contains the given number: just add it right away.
If you don't mind a small performance hit you can use Linq's ToHashSet instead: var set = array.ToHashSet(); (Edit: or new HashSet<int>(array); if you're not using .NET Framework 4.7.2).
Edit: Alternately, if you expect a lot of negative values in your inputs, not adding those to the set can result in a fair speed improvement - I'm seeing a 30% improvement for inputs with 50% negative values.

The first three changes don't really speed things up, and the last one is fairly input-specific. For a significant and more reliable performance improvement, replace the hash with an array of booleans:

var presence = new bool[array.Length + 1];
foreach (var value in array)
{
    if (value > 0 && value < presence.Length)
    {
        presence[value] = true;
    }
}

for (int i = 1; i < presence.Length; i++)
{
    if (!presence[i])
    {
        return i;
    }
}
return presence.Length;

The maximum possible return value is array.Length + 1, so values larger than that can safely be ignored (just like negative values) because they won't affect the result. The input [1, 2, 3] produces 4. Replacing any of these numbers with a larger one will create a gap: [1, 99, 3] produces 2. Whether or not 99 is present is irrelevant: what matters is that 2 is not present.

For large inputs this is about twice as fast, for small inputs it can be more than 10x faster. Hash set lookups are fast, but there is some overhead involved, so they won't beat array lookups.

Edit: The proposed solution on their website is (re)using the input array as a lookup table. It's moving all positive numbers to the front and then marking the presence of numbers by making the value at that index negative - somewhat similar to the above approach. I guess the first step allows them to simplify the rest of the algorithm, but it does make things slower. That first step can be removed with careful swapping and negating, making it run faster, but it's still a bit slower as the array of boolean approach - and more difficult to understand.


Progress 4GL - Luhn algorithm My implementation in Progress 4GL of the Luhn Algorithm. Any suggestions on improving it?

FUNCTION fnLuhnAlgorithm RETURNS LOGICAL
  (INPUT pcNumber AS CHARACTER):
/*------------------------------------------------------------------------------
  Purpose:  Applies Luhn Algorithm to check a Number
    Notes:  Returns True/False Validation based on check digit

    From the rightmost digit, which is the check digit, moving left, double the value
    of every second digit;
    if product of this doubling operation is greater than 9 (e.g., 7 * 2 = 14).
        Sum the digits of the products (e.g., 10: 1 + 0 = 1, 14: 1 + 4 = 5) together
    Compute the sum of the digits.
    Multiply by 9.
    The last digit, is the check digit.
------------------------------------------------------------------------------*/
    DEFINE VARIABLE cNum        AS CHARACTER NO-UNDO.
    DEFINE VARIABLE iCheck      AS INTEGER   NO-UNDO.
    DEFINE VARIABLE iLength     AS INTEGER   NO-UNDO.
    DEFINE VARIABLE iLoopCnt    AS INTEGER   NO-UNDO.
    DEFINE VARIABLE iNum        AS INTEGER   NO-UNDO.
    DEFINE VARIABLE iNum1       AS INTEGER   NO-UNDO.
    DEFINE VARIABLE iNum2       AS INTEGER   NO-UNDO.
    DEFINE VARIABLE iTestLength AS INTEGER   NO-UNDO.

    ASSIGN
      iLength     = LENGTH(pcNumber)
      iTestLength = iLength - 1
        iCheck = 1. /* 1 for the check digit we skip */

    DO iLoopCnt = iTestLength TO 1 BY -1:
      ASSIGN
          iNum = INTEGER(SUBSTR(pcNumber,iLoopCnt,1))
          iCheck = iCheck + 1.

      IF iCheck MODULO 2 = 1 THEN
          ASSIGN iNum1 = iNum1 + iNum.
      ELSE
      DO:
          ASSIGN iNum2 = iNum * 2.
          IF iNum2 < 10 THEN
              ASSIGN iNum1 = iNum1 + iNum2.
          ELSE
              ASSIGN
                cNum  = STRING(iNum2)
                iNum1 = iNum1 + INTEGER(SUBSTR(cNum,1,1)) + INTEGER(SUBSTR(cNum,2,1)).
      END.
    END.

    ASSIGN
        iNum2 = iNum1 * 9
        iNum = iNum2 MODULO 10.

    IF iNum = INTEGER(SUBSTR(pcNumber,iLength,1)) THEN
      RETURN TRUE.
    ELSE
      RETURN FALSE.

END FUNCTION. /* fnLuhnAlgorithm  */

You are kind of overdoing it a little in my opinion, there are too many variables that don't really add much value, also I would change the parameter to be DECIMAL, to avoid calls using not numbers that would cause run time errors.

My implementation would look like this:

FUNCTION fnLuhnAlgorithm RETURNS LOGICAL
  (INPUT pcNumber AS DECIMAL):
/*------------------------------------------------------------------------------
  Purpose:  Applies Luhn Algorithm to check a Number
    Notes:  Returns True/False Validation based on check digit

    From the rightmost digit, which is the check digit, moving left, double the value
    of every second digit;
    if product of this doubling operation is greater than 9 (e.g., 7 * 2 = 14).
        Sum the digits of the products (e.g., 10: 1 + 0 = 1, 14: 1 + 4 = 5) together
    Compute the sum of the digits.
    Multiply by 9.
    The last digit, is the check digit.
------------------------------------------------------------------------------*/
    DEFINE VARIABLE cNumber     AS CHARACTER NO-UNDO INITIAL "".
    DEFINE VARIABLE iDigit      AS INTEGER   NO-UNDO INITIAL 0.
    DEFINE VARIABLE iSum        AS INTEGER   NO-UNDO INITIAL 0.
    DEFINE VARIABLE iLoopCnt    AS INTEGER   NO-UNDO INITIAL 0.

    cNumber = STRING(pcNumber).

    DO iLoopCnt = LENGTH(cNumber) - 1 TO 1 BY -1:

      iDigit = INTEGER(SUBSTR(cNumber,iLoopCnt,1)).

      IF iLoopCnt MODULO 2 = LENGTH(cNumber) MODULO 2 THEN
          iSum = iSum + iDigit.
      ELSE
          iSum = iSum + INTEGER(SUBSTR(STRING(iDigit * 2,"99"),1,1))
                      + INTEGER(SUBSTR(STRING(iDigit * 2,"99"),2,1)).

    END.

    IF ((iSum * 9) MODULO 10) = INTEGER(SUBSTR(cNumber,LENGTH(cNumber),1)) THEN
      RETURN TRUE.
    ELSE
      RETURN FALSE.

END FUNCTION. /* fnLuhnAlgorithm  */


Generate a series of random integers between 1 and 10 Is there a better way of generating a series of unique and random integers between 1 and 10 in JavaScript (ES6)?

const ran = [];

while (ran.length < 10) {
  x = Math.floor((Math.random() * 10) + 1);
  if (!ran.includes(x)) {
    ran.push(x);
  }
}
console.log(ran);

Your code could potentially run forever - in theory, it could randomly pick the same number again and again and again. Probably won't, but the point is that the code may take a long time to run.

A simpler approach is to generate the numbers 1-10, and then shuffle them, Fisher-Yates style. Something like:

var numbers = new Array(10),
    shuffled = [];

// fill one array with the numbers 1-10
numbers = numbers.fill(1).map((_, i) => i + 1);

// shuffle by taking a random element from one array
// and pushing it to the other array
while (numbers.length) {
  let idx = numbers.length * Math.random() | 0; // floor trick
  shuffled.push(numbers[idx]);
  numbers.splice(idx, 1);
}

console.log(shuffled);

(The "floor trick" works by using bitwise OR. Now, bitwise OR zero does actually do anything, but all JavaScript bitwise operations interpret their operands to 32-bit integers, so it's like casting to an int, discarding decimals - i.e. it floors the number.)


ThreadPoolExecutor submit refactor Currently I need to download thousand of images, I can use 2 functions to download the images: LoadUrl or LoadUrlProxy.

Each function has different parameters:

LoadUrl(url, timeout)
LoadUrlProxy(url, proxy_client, proxy_params)

As I don't want to duplicate the code, I'm looking to simplify the way I'm passing a function reference and its parameters, not sure If below is the best way or I can improve this code?

     # func(Function): Function reference which defines how to download images
      func = LoadUrl
      if use_proxy:
        proxy_client = CreateProxyClient()
        proxy_params = GetProxyParams()
        func = LoadUrlProxy
      images = []

      with futures.ThreadPoolExecutor(max_workers=_MAX_WORKERS) as executor:
        # Start the load operations and mark each future with its URL
        if use_proxy:
          future_to_url = {
              executor.submit(func, image, proxy_client, proxy_params): image
              for image in image_list
          }
        else:
          future_to_url = {
              executor.submit(func, image, _EXECUTOR_TIMEOUT): image
              for image in image_list
          }
        for future in futures.as_completed(future_to_url):
          image = future_to_url[future]
          try:
            images.append((image.master_id, future.result()))
          except Exception as exc:
            logging.exception('%r Generated an exception: %s', image.url, exc)

Why not use functools.partial to simplify further and avoid the if use_proxy a second time?

Something along the lines of

if use_proxy:
    func = functools.partial(
            LoadUrlProxy,
            proxy_client=CreateProxyClient(),
            proxy_params=GetProxyParams())
else:
    func = functools.partial(LoadUrl, timeout=_EXECUTOR_TIMEOUT)

with futures.ThreadPoolExecutor(max_workers=_MAX_WORKERS) as executor:
    future_to_url = {
            executor.submit(func, image): image
            for image in image_list
    }

    for future in futures.as_completed(future_to_url):
        image = future_to_url[future]
        try:
            images.append((image.master_id, future.result()))
        except Exception as exc:
            logging.exception('%r Generated an exception: %s', image.url, exc)


Terrain map generator with JavaScript and HTML5/CSS I'm trying to generate maps (as in terrain maps) using JavaScript only, but currently performance is pretty bad at best.

What my script currently does is:

Generate a square base grid for the map and print it
For each cell, check the adjacent cell's terrain type and select the terrain for the current cell
Print the results

How can I improve performance? (I'm aware that the current result looks nothing like an actual map, but one thing at a time.)

I've hosted my current script here.

(base and terrain properties are CSS classes)

$(document).ready(function () {
    var x = 50;
    var y = 50;
    var worldMap = [];

    worldMap = genesis(x, y, worldMap);
    worldMap = terraform(x, y, worldMap);
});

function genesis(x, y, worldMap) {
    for (i = 0; i < x; i++) {
        worldMap[i] = [];
        for (var j = 0; j < y; j++) {
            worldMap[i][j] = {
                xCoord: i,
                yCoord: j,
                base: "cell",
                terrain: "baren"
            };
            //console.log(worldMap[i][j]);
        }
    }

    var world = $(".world");

    for (i = 0; i < x; i++) {
        var row = document.createElement("div");
        row.className = "row";

        for (j = 0; j < y; j++) {
            var cell = document.createElement("div");
            cell.className = worldMap[i][j].base + " " + worldMap[i][j].terrain;
            cell.setAttribute('data-row', i);
            cell.setAttribute('data-col', j);
            row.appendChild(cell);
        }

        world.append(row);
    }

    return worldMap;
}

function selectTerrain(worldMap, cell) {
    var terrain = ["river", "river", "forest", "forest", "plain", "plain", "plain", "desert", "mountain"];

    var i = cell.xCoord;
    var j = cell.yCoord;

    console.log(i + " " + j);

    for (var a = -1; a < 2; a++) {
        for (var b = -1; b < 2; b++) {
            var m = parseInt(i) + parseInt(a);
            var n = parseInt(j) + parseInt(b);

            if (typeof worldMap[m] != "undefined") {
                if (typeof worldMap[m][n] != "undefined") {
                    var adj = worldMap[m][n];
                    console.log(adj);
                    console.log("Is defined");
                    if (adj.terrain != "baren") {
                        console.log("Is not baren");
                        for (z = 0; z < 4; z++) {
                            terrain.push(adj.terrain);
                        }
                        console.log(terrain);
                    }
                }
            }

        }
    }

    var selectedIndex = Math.floor(Math.random() * (terrain.length - 0)) + 0;
    var selectedTerrain = terrain[selectedIndex];

    return selectedTerrain;
}

function terraform(x, y, worldMap) {
    for (var i = (x-1); i-- ;) {
        for (var j = (y-1); j--; ) {
            var mappingCell = worldMap[i][j];
            console.log(mappingCell);
            var selectedTerrain = selectTerrain(worldMap, mappingCell);

            worldMap[i][j].terrain = selectedTerrain;

            console.log(mappingCell.terrain);

            var cell = $("[data-row=" + i + "][data-col=" + j + "]");
            cell[0].className = mappingCell.base + " " + mappingCell.terrain;
        }
    }

    return worldMap;
}

That map looks super cool! The performance isn't THAT bad, considering you're generating 2500 DIV elements :)

What you could do is generate one Canvas instead of so many DIVS. The GPU can render a canvas super-fast. That will also be helpful if you start adding more elements to your game.

There is no reason to use jQuery, so you can get rid of that too :)


Determining the maximum value that can be obtained from a loot A thief finds much more loot than his bag can fit. We have to find the most valuable combination of items assuming that any fraction of a loot item can be put into his bag.

For example has a bag which can fit at most 50 Kg of items. He has 3 items to choose from: the first item has a total value of $60 for 20 Kg, the second item has a total value of $100 for 50 Kg and the last item has a total value of $120 for 30 Kg.

So, if the thief takes the most of the item that costs more per unit Kg, he/she can make a better profit out of his/her thievery. In that case, he/she would take 30 Kgs of the third item, and 20 Kgs of the first item, resulting in a total of 50 Kgs (full capacity) with the total value of $180.

// THIS IS AN EXAMPLE OF THE FRACTIONAL KNAPSACK PROBLEM

#include<cstdio>
#include<iostream>
#include<vector>
using namespace std;
double get_max_index(vector<int>, vector<int>);
double get_max_value(vector<int>, vector<int>, int);

int main() {

    int num_items, bag_capacity;
    cout << "Enter the number of items: " << endl;
    cin >> num_items;
    cout << "Enter the total capacity that the bag can support: " << endl;
    cin >> bag_capacity;

    vector<int> values;
    vector<int> weights;

    for (int i = 0; i < num_items; i++)
    {
        int buff_val = 0, buff_wgt = 0;
        cout << "Enter the value and weight of Item " << i + 1 << ": " << endl;
        cin >> buff_val >> buff_wgt;
        values.push_back(buff_val);
        weights.push_back(buff_wgt);
    }

    cout.precision(10);
    cout << "The maximum loot value that can be acquired is " << fixed << get_max_value(values, weights, bag_capacity) << "." << endl;          // Always display 'precision' number of digits

    return 0;
}

double get_max_index(vector<int> vals, vector<int> wgts){
    int max_index = 0;
    double max_val_per_wgt = 0;
    for (int i = 0; i < wgts.size(); i++)
    {
        if (wgts[i] != 0 && (double) vals[i] / wgts[i] > max_val_per_wgt)
        {
            max_val_per_wgt = (double) vals[i] / wgts[i];
            max_index = i;
        }
    }
    return max_index;
}

double get_max_value(vector<int> vals, vector<int> wgts, int capacity){

    double max_val = 0.0;
    for(int i = 0; i < wgts.size(); i++) {
        if (capacity == 0)
        {
            return max_val;             // There's no space left in the bag to carry
        }
        int max_value_index = get_max_index(vals, wgts);     // See which item has the best value per weight index
        double taken = capacity > wgts[max_value_index] ? wgts[max_value_index] : capacity;     // get the minimum of the item's weight and capacity left in the bag
        max_val += taken * (double) vals[max_value_index] / wgts[max_value_index];      // calculate value for the item's weight taken
        capacity -= taken;          // reduce capacity of the bag by the amount of weight of item taken
        wgts[max_value_index] -= taken;         // reduce the item's weight by the amount taken
    }
    return max_val;
}

Please review this code in terms of complexity or in other areas that might result in this code failing. Also, looking forward to criticism and if you decide to post criticism, kindly include relevant details that a not-so-experienced programmer might find helpful in solving the problem.

The algorithm
Your algorithm needs O(n) space (for storing all items), and O(n * k) time (for selecting the best ones) (n = items to choose from, k = items chosen, bounded by n).
By choosing as you go, you can get that down to O(k') space and O(n * k') time (k' = maximum items chosen at any time, at least k, bounded by n).
Take a look at std::push_heap, std::tuple and lambdas for implementing the new algorithm.
Avoid casting
Casting is error-prone, as it circumvents the protections of the type-system. Thus, use the most restricted cast you can, and don't cast at all if reasonably possible. In your case, why not multiply with 1.0 instead?
Floating-point is hard
You are calculating the specific value (value per weight) of your items for comparison purposes. Luckily, a double has enough precision that you are extremely unlikely to suffer from rounding-errors when dividing two 32 bit numbers. Still, instead of comparing 1.0 * a_value / a_weight < 1.0 * b_value / b_weight you could compare 1LL * a_value * b_weight < 1LL * b_value * a_weight, avoiding division and floating-point.
All those useless copies
While copying small trivial types is generally the right approach, a std::vector is neither small nor trivial; Copying it is rather expensive. If you only need to read it, use a constant reference or preferably a C++2a std::span for increased flexibility.
Gracefully handle all kinds of invalid input
No need to assume malice, you are assured your load of garbage anyway.
Just the code

I don't see where you use anything from <cstdio>, so don't include it.

Never import a namespace wholesale which isn't designed for it. Especially std is huge and ever-changing, which can silently change the meaning of your code even between minor revisions of your toolchain, let alone using a different one.
It cannot be guaranteed to break noisily.

return 0; is implicit for main(). Make of that what you will.