Optimizing a raycasting/voxelspace algorithm in Pygame In the past three days, I've been developing a simple raycasting algorithm. I've made the renderer work as I'd like to. However, it is extremely slow (5 frames per second) because of two things (as far as I know).

Finding the values of a pixel independently many times, and often it lands on the same pixel.
The large amount of times the code is run.

I'm aiming for speed but the only programming language I have experience in is Python. What are the optimizations I can make to this code? Which other libraries can I use that are faster than Pygame or PIL for this use?
Also, how do I make the pixel coordinates loop (-1 = 1023) without if statements to prevent the game crashing if near the boundary?

from PIL import Image
import pygame

cdef enum resolution:
  resX=720
  resY=540
  qresX=180
cdef extern from "math.h":
  double sin(double x)
  double cos(double x)

screen = pygame.display.set_mode((resX,resY))
pygame.display.set_caption('VoxelSpace')

im=Image.open("data/height.png")
disp=im.load()
col = Image.open("data/colour.png")
rgb_im = col.convert('RGB')
rgb=rgb_im.load()

cdef unsigned char uu
cdef unsigned char height
cdef unsigned char prevHeight
cdef unsigned char heightBuffer
cdef float t
cdef float foga
cdef unsigned char fogb
cdef int x
cdef int y
cdef unsigned char r
cdef unsigned char g
cdef unsigned char b
cdef int pX
cdef int pY
cdef float pAngle
cdef unsigned char u
cdef int v
cdef unsigned char h

def Render(pX,pY,pAngle):
  screen.fill((100,100,255))
  heightBuffer=disp[pX,pY]
  for v from -qresX <= v < qresX by 1:
    prevHeight=0
    for u from 1 <= u < 124 by 1:
      if u<30:
        uu=u
      elif u<60:
        uu=u*2-45
      else:
        uu=u*4-180
      foga=u/60.0+1
      fogb=u/2

      t=v/float(qresX)+pAngle
      x=int(sin(t)*-uu+pX)
      y=int(cos(t)*-uu+pY)
      height=(float(disp[x,y]-heightBuffer-10)/uu*101+135)*2
      if height>prevHeight:
        r,g,b=rgb[x,y]
        pygame.draw.line(screen,(r/foga+fogb,g/foga+fogb,b/foga+u) , (2*(qresX+v),resY-prevHeight),(2*(qresX+v),resY-height),2)
        prevHeight=height
  pygame.display.flip()

I am compiling this code as a .so (via cython) and running it from another .py. Is it faster if I simply use PyPy instead?

Below is the code I have come up with. It basically follows the code used for the web demo in the github repo you had linked to. I will add some explanation to this answer on how I had made some basic optimizations to better fit cython's memoryview model as well as potential room for improvement later, but figured I should post the code now as it might be another few days before I can get back to it.

cimport libc.math as c_math
from libc.stdint cimport *
import math
import numpy as np
from PIL import Image

import os
os.environ['PYGAME_HIDE_SUPPORT_PROMPT'] = "hide"
import pygame
import sys
import time

ctypedef struct Point:
    float x
    float y

ctypedef struct Color:
    uint8_t r
    uint8_t g
    uint8_t b

cdef class Camera:
    cdef:
        public int x
        public int y
        public int height
        public int angle
        public int horizon
        public int distance

    def __init__(self, int x, int y, int height, int angle, int horizon, int distance):
        self.x = x
        self.y = y
        self.height = height
        self.angle = angle
        self.horizon = horizon
        self.distance = distance

cdef class Map:
    cdef:
        int width
        int height
        int shift
        const uint8_t[:, :, :] color_data
        const uint8_t[:, :] height_data

    def __init__(self, int width=1024, int height=1024, int shift=10):
        self.width = width
        self.height = height
        self.shift = shift
        self.color_data = None
        self.height_data = None

    def load_color_data(self, str color_path):
        cdef object image
        image = Image.open(color_path).convert("RGB")
        self.color_data = np.asarray(image)

    def load_height_data(self, str height_path):
        cdef object image
        image = Image.open(height_path).convert("L")
        self.height_data = np.asarray(image)

cdef class Window:
    cdef:
        int width
        int height
        str title
        object screen
        object clock
        int32_t[:] hidden_y
        uint8_t[:, :, :] output
        Color background_color
        Camera camera
        Map map

    def __init__(self, int width, int height, str title):
        self.width = width
        self.height = height
        self.screen = pygame.display.set_mode((self.width, self.height))
        self.title = title
        pygame.display.set_caption(self.title)
        self.hidden_y = np.zeros(self.width, dtype=np.int32)
        self.output = np.zeros((self.width, self.height, 3), dtype=np.uint8)
        self.clock = pygame.time.Clock()

    def set_background_color(self, uint8_t r, uint8_t g, uint8_t b):
        self.background_color.r = r
        self.background_color.g = g
        self.background_color.b = b

    def set_camera(self, Camera camera):
        self.camera = camera

    def set_map(self, Map map):
        self.map = map

    cdef void draw_background(self):
        cdef int x, y
        for x in range(self.width):
            for y in range(self.height):
                self.output[x, y, 0] = self.background_color.r
                self.output[x, y, 1] = self.background_color.g
                self.output[x, y, 2] = self.background_color.b

    cdef void draw_vertical_line(self, int x, int y_top, int y_bottom, Color *color):
        cdef int y

        if y_top < 0:
            y_top = 0
        if y_top > y_bottom:
            return
        for y in range(y_top, y_bottom):
            self.output[x, y, 0] = color.r
            self.output[x, y, 1] = color.g
            self.output[x, y, 2] = color.b

    cdef display(self):
        surf = pygame.surfarray.make_surface(np.asarray(self.output))
        self.screen.blit(surf, (0, 0))
        pygame.display.flip()
        pygame.display.set_caption("{0}: {1} fps".format(self.title, <int>self.clock.get_fps()))

    def render(self):
        cdef:
            int map_width_period = self.map.width - 1
            int map_height_period = self.map.height - 1
            float s = c_math.sin(self.camera.angle)
            float c = c_math.cos(self.camera.angle)

            float z = 1.0
            float delta_z = 1.0
            float inv_z
            Point left, right, delta
            int i
            int map_x
            int map_y
            int height_on_screen
            Color color

        for i in range(self.width):
            self.hidden_y[i] = self.height
        self.draw_background()
        while z < self.camera.distance:
            left = Point(
                (-c * z) - (s * z),
                (s * z) - (c * z),
            )
            right = Point(
                (c * z) - (s * z),
                (-s * z) - (c * z),
            )
            delta = Point(
                (right.x - left.x) / self.width,
                (right.y - left.y) / self.width,
            )
            left.x += self.camera.x
            left.y += self.camera.y

            inv_z = 1.0 / z * 240
            for i in range(self.width):
                map_x = <int>c_math.floor(left.x) & map_height_period
                map_y = <int>c_math.floor(left.y) & map_width_period
                height_on_screen = <int>((self.camera.height - self.map.height_data[map_x, map_y]) * inv_z + self.camera.horizon)
                color.r = self.map.color_data[map_x, map_y, 0]
                color.g = self.map.color_data[map_x, map_y, 1]
                color.b = self.map.color_data[map_x, map_y, 2]

                self.draw_vertical_line(i, height_on_screen, self.hidden_y[i], &color)
                if height_on_screen < self.hidden_y[i]:
                    self.hidden_y[i] = height_on_screen
                left.x += delta.x
                left.y += delta.y

            delta_z += 0.005
            z += delta_z

        self.display()
        self.clock.tick(60)#60 fps

pygame.init()

window = Window(width=800, height=600, title="VoxelSpace")
window.set_background_color(144, 144, 224)

map = Map()
map.load_color_data("./images/C1W.png")
map.load_height_data("./images/D1.png")
window.set_map(map)

camera = Camera(x=512, y=800, height=78, angle=0, horizon=100, distance=800)
window.set_camera(camera)

while True:
    #win.handle_input()
    for event in pygame.event.get():
        if event.type == pygame.QUIT:
            pygame.quit()
            sys.exit()
        if event.type == pygame.KEYDOWN:
            if event.key == pygame.K_UP:
                print("up")

    camera.y -= 1
    window.render()


Store the newest 100 numbers. (delete old ones) The task is to store the newest ~100 numbers and calculate their sum.

I would like to optimize for time-performance.

Usage: In the context of sequence alignment, after creating a 2D scoring-matrix between 2 sequences, starting at the bottom right corner, I want to search for the optimal alignment path. Here I adapt previous alignment algorithms to search for the optimal alignment at a fixed alignment length. For this, the sum of the previous path isn't sufficient, but instead I want to save the values of the previous path into each cell.

storage = []
new_number = 1

def store_numbers(storage, new_number, storage_size = 100):
    storage.append(new_number)
    if len(storage)>storage_size:
        del storage[0]
    return storage, sum(storage)

#test
for x in range(100):
    print(store_numbers(storage, x, storage_size = 10)

Ideas: Avoid the function? Avoid append() - del() and instead overwrite? Instead of list use Numpy array or something else?

You could make your life way easier if you used the deque data structure.

from collections import deque

d = deque(maxlen=2)
d.append(1)
d.append(2)
d.append(3)

sum(d)  # gives 5, which is the sum of the last two inserted elements.

With this, you call the sum function only when you really need to, which will improve your performance if you don't need the sum every time (like when you're first populating the container).

Also, deque probably has faster length checking internally than using len onto a list.


Reasonable ActiveSupport dependency and define_method usage? I'm writing a results wrapper for a gem which wraps a Thesaurus API. Basically, when you lookup a word in the thesaurus, the results come back in approximately this JSON form:

{
  'noun' => {
    'syn' => ['array', 'of', 'synonyms'],
    'rel' => ['array', 'of', 'related terms']
   },
   'verb' => {
     'syn' => ['synonyms', 'of', 'verb', 'word form'],
     'ant' => ['antonyms', 'of', 'word']
   }
}

I would like to be able to access the results in the following ways:

Get an array of synonyms across all parts of speech. eg.

results.synonyms
# => ['array', 'of', 'synonyms', 'synonyms', 'of', 'verb', 'word form']

Indifferently (writing results['noun'] all the time is a PITA):

results[:noun]

Here is my code and questions:

Is it acceptable to create a dependance on ActiveSupport just so I can access the results indifferently, or should I just suck it up? THe only other AR method I'm using in the gem is blank?.
Is the usage of define_method too magical? Should I just define the convenience methods normally instead?

require "active_support/core_ext/hash/indifferent_access"

module Dinosaurus
  class Results < HashWithIndifferentAccess

    def initialize(data = {})
      super(data)
    end

    # Define some convenience methods.
    { 'synonyms' => :syn,
      'antonyms' => :ant,
      'similar_terms' => :sim,
      'related_terms' => :rel
    }.each do |name, key|
      define_method name do
        grouped(key)
      end
    end

  private

    def grouped(key)
      group = []
      self.each do |pos, type|
        group += type[key] if type[key]
      end
      group
    end
  end
end

1) I believe that depending on popular and stable libraries is not a problem. Though I prefer more opinionated approach: to symbolize_keys!.

2) You define_method looks clear enough, IMHO

3) You may simplify grouped like this:

def grouped_by(key)
  self.values.flat_map { |type| type[key] }.compact!
end


Class naming conventions I have the following scenario:

The (web-) application is built with Java/Spring. I have several REST-endpoints which deliver data prepared to be consumed by a frontend by datatables.net. Each of this endpoint has a somehow similar signature (sEcho, iDisplayStart, iDisplayLength etc.) which are standardparameters necessary to work with datatables. Some of the controllers need additional Information, which are provided in extra variables.

So I made a DatatablesOptions class for the general usecase and if I need additional fields I extend the DatatablesOptions class.

My question is now about the naming of the extended classes.

There are two approaches:

I have a package for each of the endpoints, where all additional classes for a topic are stored: e.g. "CustomerComplaints". In such a package you would find the derived class which is used for the additional fields exactly for the usecase of "customer complaints". Hence it is in the "CustomerComplaints"-package the name of e.g. "ViewOptions" would be enough to say what it is used for: »This object is used for the options of a view in the context of "CustomerComplaints"«.

So my code in the Controller looks like this:

@Controller
@RequestMapping("/")
public class CustomerComplaints {

[...]
    @RequestMapping(value="/customercomplaints/datatable", method= RequestMethod.GET)
    @ResponseBody
    public AjaxTableModel getCustomerComplaintsForDataTable(@RequestParam (value="sEcho")int echo,
                                            @RequestParam (value="iDisplayStart")int startLine,
                                            @RequestParam (value="iDisplayLength")int linesPerPage,
                                            @RequestParam (value="iSortCol_0")int columnToSort,
                                            @RequestParam (value="sSearch", required = false) String searchString,
                                            @RequestParam (value="statusfilter")FilterOptions filterOptions,
                                            @RequestParam (value="sSortDir_0")String sortingDirection
[...]
ViewOptions o = new ViewOptions()
            .setLinesPerPage(linesPerPage)
            .setStartLine(startLine)
            .setSortColumn(getSortingKeyForColumnNumber(columnToSort))
            .setSortingDirection(sortingDirection)
            .setFilterOptions(filterOptions)
            .setSearchString(searchString);

The job of the ViewOptions-class is in every controller the same and for the understanding of the code neglectable.

So a further specification via the classes name is not necessary. It seems to me like some kind of "Hungarian Notation". The name CustomerComplaintsViewOptions provides not more Information in this context. And the package makes it unambigous enough. Every smart IDE should resolve the definition for that.
The second approach, which is more texteditor-friendly, is to make it easy to find the definition of this class and name it CustomerComplaintsViewOptions. So a coworker sees from the variable name, where to look, when he wants to know more.

What kind of naming is better style?

Actually #1 is called smurf naming (here is a question about it on Programmers.SE).

One thing to consider: Do you have to work with more than one ViewOptions at the same time (or at the same source file)? If that's true having a more specific name would help. Two possible advantages of smurfier names:

There aren't multiple editor tabs with the same filename.
You don't have to use full package names to declare two ViewOptions in the same source file.

  So I made a DatatablesOptions class for the general usecase and if
  I need additional fields I extend the DatatablesOptions class.

Check Effective Java, Second Edition, Item 16: Favor composition over inheritance if you don't know already.
Try to format the code in a way which avoid unnecessary horizontal scrolling. It would make it esasier to read.
@RequestParam (value="iDisplayStart")int startLine,
@RequestParam (value="iDisplayLength")int linesPerPage,
@RequestParam (value="iSortCol_0")int columnToSort,
@RequestParam (value="sSearch", required = false) String     searchString,

Some of the parameters have a space between the closing parentheses (...false) String) , some of them doesn't (...th")int linesPerPage). You can find another inconsistencies too. It should be consistent. Modern IDEs have autoformat, they do a really good job here, use them.


Routing System request syntax I'm working on writing a routing system for a PHP project. I'm trying to write it in a way that it can easily be expanded later on via plugins.
I have 2 ideas for how to go about doing it. I've already created working versions of both, now it's just down to which one people find easier to use.
1:
class GeneralRouter extends Router {

    public function RootRoute (Request $request, Response $response){
        $request->requiresGet();
        $response->html('home');
        $response->send();
    }

    public function PageRoute (Request $request, Response $response){
        $request->requiresGet();
        $response->html('Hi, looking at \"' . $request->parameters["page"]. '\"');
        $response->send();
    }

    public function routes (){
        return array(
            '/' => 'RootRoute',
            '/:page/' => 'PageRoute'
        );
    }
}

2:
class GeneralRouter extends Router {

    public function init (){
        $this->get('/', function(Request $req, Response $res){
            $response->html('
home
');
            $response->send();
        });

        $this->get('/:page/', function(Request $req, Response $res){
            $response->html('
Hi, looking at \"' . $req->parameters["page"]. '\"
');
            $response->send();
        });
    }
}

#1 is similar to the way I've seen other PHP projects do their routing classes (CraftCMS for example), however I really like the ExpressJS-like format of #2. Which would be easier for expansion and/or is the "correct" way of doing it?

The first example is a violation of the open/closed-principle. You shouldn't need to extend the class in order to change the routes. There doesn't seem to be any other way to use the class without extending it.

class Router {
    protected $routes = [];

    protected function getRoute(Request $req) {
        // ...
    }

    public function handle(Request $req, Response $res) {
        $fn = $this->getRoute($req);
        call_user_func($fn, $req, $res);
    }

    public function setRoutes(array $routes)
    {
        $this->routes = $routes;
    }

    public function addRoute($method, $path, $fn) {
        $this->routes["$method $path"] = $fn;
    }
}

Instead of setting method as values for the routes, use a callable. This can be a function, method, closure or invokable object.

A user may still choose to extend it and initialize in the constructor if he really wants to.

class GeneralRouter extends Router {

    public function RootRoute (Request $request, Response $response){
       // ...
    }

    public function PageRoute (Request $request, Response $response){
       // ...
    }

    public function __construct() {
        $this->setRoutes([
            'GET /' => [self::class, 'RootRoute']
            'GET /:page/' => [self::class, 'RootRoute']
        ]);
    }
}

Adding a get, post and delete method to the Router class would just add micro framework syntax for those who like it, but it wouldn't fundamentally change the router.

Last, do not invent your own custom Request and Response objects. Use PSR-7 to ensure interoperability with libraries and test suites. There is Zend Diactorors and Jasny HTTP message (developed by me).


Class for removing Non Alpha/Numeric Character String and Counting Words Yesterday I had help via Stack Overflow on a program I written some code for, and needed to know if it needed to be improved.

After reviewing the responses, I restructured the code into the following...

using System;

namespace Language
{
    public static class Grammar
    {
        /// <summary>
        /// Returns a string value with non alpha/numeric characters removed.
        /// </summary>
        /// <param name="Sentence"></param>
        public static string RemoveNonAlphaNumeric(string Sentence)
        {
            string[] Removed = { " ", ".", "!", "?", "@", "%", "&", "^", "$", "#", "*", "~" };
            string[] words = Sentence.Split(Removed, StringSplitOptions.RemoveEmptyEntries);

            return string.Join(" ", words);
        }

        /// <summary>
        /// Returns the integer value of the number of words in a sentence.
        /// </summary>
        /// <param name="Sentence"></param>
        /// <returns></returns>
        public static int GetWordCount(string Sentence)
        {
            string[] Removed = { " " };
            string[] Words = Sentence.Split(Removed, StringSplitOptions.RemoveEmptyEntries);

            return Words.Length;
        }
    }
}

Initally GetWordCount() contained a foreach() loop which was basically counting the number of words in the array. I took the suggestion of someone removed this. Then replaced the return CountWords which was initially a variable declared at 0, with return words.Length. I also took out Stream as a string parameter as someone  suggested that it may cause confusion.

Here's how the functions can be called from the main()...

using System;
using Language;

namespace ULESConMain
{
    class Program
    {
        static void Main(string[] args)
        {

            string OldSentence = "@#The dirty dog, was &&walking proudly!";

            // output using just Console.WriteLine() without assigning to a varaiable //

            Console.WriteLine($"{Grammar.RemoveNonAlphaNumeric(OldSentence)}");
            Console.WriteLine($"The total number of words in the sentence = {Grammar.GetWordCount(OldSentence)}");

            // Output using Console.WriteLine() using returned values //

            string NewSentence1 = Grammar.RemoveNonAlphaNumeric(OldSentence);
            int WordCount = Grammar.GetWordCount(NewSentence1);

            Console.WriteLine();

            Console.WriteLine(NewSentence1);
            Console.WriteLine($"The total number of words in the sentence = {WordCount}");

            // Prompt to return control to the Operating System and exit the program.
            Console.WriteLine("\nPress the ENTRER key to continue...");
            Console.ReadKey();  // Get user input to return to the Operating System.
        }
    }
}

If anyone has any ideas or if you think this is good enough since it's working as expected. Please let me know.

The function RemoveNonAlphaNumeric promises to remove non-alphanumeric characters. It does this in an incomplete way.

The comma is not removed.
The apostrophe is not removed.
The em-dash — is not removed.

There's probably a function Character.IsDigit and Character.isLetter that is more appropriate. In the end, whether it is appropriate or not depends on what the code is supposed to do at all. What do you need it for, why do you want to remove non-alphanumeric characters in the first place?


Getting user's current location I am trying to get user's current location base on the accuracy and less battery consumption.

Please review my code to help determine if I am going in right direction.

Location location = null;
Criteria criteria = new Criteria();
criteria.setAccuracy(Criteria.ACCURACY_FINE);
criteria.setAltitudeRequired(false);
criteria.setBearingRequired(false);
criteria.setCostAllowed(true);
criteria.setPowerRequirement(Criteria.POWER_
try {
    String provider = locationManager.getBestProvider(criteria, false);
    locationManager.requestLocationUpdates(provider, 2000, 5,locationListener);
    location = locationManager.getLastKnownLocation(provider);
    lati = Double.toString(location.getLatitude());
    longi = Double.toString(location.getLongitude());

} catch (Exception ex) {
    ex.printStackTrace();
}

The last know location could be null. So you need a further check for this, then ask for a quick update:

   if(location == null){
            String provider = locationManager.getBestProvider(criteria, true);
            if (provider != null){
                locationManager.requestLocationUpdates(provider, 0, 0, singeUpdateListener, context.getMainLooper());
            }else {
                Log.e("TAG", No location set");
            }
    }

class variable:

protected LocationListener singeUpdateListener = new LocationListener() {
        public void onLocationChanged(Location location) {
            String lati = Double.toString(location.getLatitude());
            String longi = Double.toString(location.getLongitude());
            locationManager.removeUpdates(singeUpdateListener);
        }

        public void onStatusChanged(String provider, int status, Bundle extras) {}
        public void onProviderEnabled(String provider) {}
        public void onProviderDisabled(String provider) {}
      };


Generating an ordinal number for the day of the month I am relatively new to programming and came across an if statement that I believe to be a bit verbose. I have tested my code and it appears to work without any issues, but I am wanting to know if there is a more efficient means or better shorthand of an if statement checking "day".

NSDate *now = [[NSDate alloc] init];
NSLog(@"The date is %@", now);

double seconds = [now timeIntervalSince1970];
NSLog(@"It has been %f seconds since the start of 1970.", seconds);

NSCalendar *cal = [NSCalendar currentCalendar];
NSLog(@"My calendar is %@", [cal calendarIdentifier]);

unsigned long day = [cal ordinalityOfUnit:NSCalendarUnitDay
                         inUnit:NSCalendarUnitMonth
                         forDate:now];

if (day == 1 || day == 21 || day == 31) {
        NSLog(@"Today is the %lust day of the month", day);
    } else if (day == 2 || day == 22) {
        NSLog(@"Today is the %lund day of the month", day);
    } else if (day == 3 || day == 23) {
        NSLog(@"Today is the %lurd day of the month", day);
    } else {
        NSLog(@"Today is the %luth day of the month", day);
    }

The problem of formatting an ordinal number is a general one that applies to more than just 31 days of a month.  You also have repetition of much of the string @"Today is the %lu?? day of the month".  Therefore, I suggest defining a separate function for handling this problem.  Taking inspiration from NSNumberFormatter and 'th' 'st' 'nd' 'rd' (ordinal) number endings, I would write:

NSString* const ordinalNumberSuffix(NSUInteger num) {
    if ((num / 10) % 10 == 1) {
        return @"th";       // Special cases 11th, 12th, 13th
    }
    switch (num % 10) {
        case 1:  return @"st";
        case 2:  return @"nd";
        case 3:  return @"rd";
        default: return @"th";
    }
}

Then you can call

NSLog(@"Today is the %lu%@ day of the month", day, ordinalNumberSuffix(day));


Persistent, dictionary-like object ensurance This is a part of a project I'm working on. The codebase is Python 2.7 and I don't have an option to switch to Python 3.

Code:

import shelve
def get_document_info():
    token_shelf = False

    try:
        token_shelf = shelve.open(os.path.join(current_dir, 'document_info.shelve'))
        token_mapping = token_shelf['token_mapping']

        if token_mapping:
            return token_mapping
        else:
            return False

    except Exception, e:
        pass

    finally:
        if token_shelf:
            token_shelf.close()

Shelve is a persistent, dictionary-like object. More info can be found at https://docs.python.org/2/library/shelve.html

I need to ensure that token_shelf is closed at the end. In the beginning of the function, what do I define token_shelf as? If I don't define something there, PyCharm will throw an error saying that the token_shelf in finally block does not exist.

What's the ideal way to write this piece of code? How can it be made more elegant and Pythonic?

PS: This code is working as expected right now.

Ideally you would want to use Python's with ... as syntax, which creates a context that calls the exit method of whatever object you create. For this, the shelve.Shelf classes would need to implement the dunder methods __enter__ and __exit__ (which they don't).

At this point you basically have three options:

Inherit from them and implement it yourself:

class Shelf(shelve.Shelf):

    def __new__(cls, file_name):
        return shelve.open(file_name)

    def __enter__(self):
        return self

    def __exit__(self, *args):
        self.close()

Or just write a simple wrapper class like this:

class Shelf(object):

    def __init__(self, file_name):
        self.file_name = file_name

    def __enter__(self):
        self.obj = shelve.open(self.file_name)
        return self.obj

    def __exit__(self, *args):
        self.obj.close()

Both can be used like this:

import os
import shelve

def get_document_info(current_dir):
    shelf_path = os.path.join(current_dir, 'document_info.shelve')
    with Shelf(shelf_path) as token_shelf:
        return token_shelf.get('token_mapping')

When you use try..except you should always guard against as specific exceptions as possible (so that unexpected exceptions can still rise to the top). In this case this would probably be a KeyError. Here I used the get method instead, which will return None if the key is not found.

I also removed the return False, because you should check for falsiness in the calling code. In other words, if you are doing if token_mapping is False or  if token_mapping == False in the calling code, you are doing it wrong. Just have the if not token_mapping/if token_mapping there.

Use Python's contextlib.closing, which already implements something like option 2:

from contextlib import closing
import os
import shelve

def get_document_info(current_dir):
    shelf_path = os.path.join(current_dir, 'document_info.shelve')
    with closing(shelve.open(shelf_path)) as token_shelf:
        return token_shelf.get('token_mapping')


JavaScript and callback: apply an action on each element of a callback input I'm quite new to js. I even don't know how to call this problem: apply an action on each element of a callback input, but the elements are actually an output..

I crash a lot on this situation: I have a nested callback where first I load a json array and then I perform an action on each element of this array.

$.getJSON(DATA_URL+'/groups', function( groups ) {
    groups.forEach( function(g){
        appendDropdownItem(g);
    } );
});

I want to do it in one line. Not because I think one line is cool but because I think that this nested callback only gets the code dirty and affect readability.

In a pythonic approach I imagine somethings like a comprehension:

[appendDropDownItem(g) for g in loadAjax(DATA_URL+'/groups')]

So.. how to make it one liner/more readable?

First you can realize that function(x){return f(x)} == f, so you can reduce one line there:

$.getJSON(DATA_URL +'/groups', function(groups){
  groups.forEach(appendDropdownItem)
})

It is harder to go further with these builtin functions. But if you have a curried forEach, with the arguments in the right order, such as the one in Rambda, or in Essentialjs (shameless plug), then you can reduce it even more:

$.getJSON(DATA_URL +'/groups', forEach(appendDropdownItem))

There you got a one readable one-liner.

You can implement your own curried forEach in any case:

var forEach = function(f) {
  return function(xs) {
    return xs.forEach(f)
  }
}

But these libraries include many other helpers that provide a more functional workflow that leads to these one-liners often by means of currying and composition.


Change the limit from CakePHP 3 Paginator by click in link inside an I'm using the CakePHP 3.0 to paginate some content, but I can't find a way to change the limit (user on browser choose the limit of content displayed on screen) using Paginator built-in method (like sort built-in), to do this I use javascript to change limit in URL.

Qustion: Exist any way to do this in cakephp style ?

Controller action that uses paginate

public $helpers = [
    'Paginator' => ['templates' => 'paginator-templates']
];

public function search()
{
    if($this->request->is('get'))
    {
        $search = $this->request->query['search'];
        @$limit = $this->request->query['limit'] ?: 3;

        $this->paginate = [
            'conditions' => ['product_name LIKE' => '%' . $search . '%'],
            'limit' => $limit,
            'contain' => ['Medias' => function($q){
                return $q->select(['path', 'product_id'])
                    ->where(['media_type_id' => 3]);
            }]
        ];

        $products = $this->paginate($this->Products);
     }
}

Select with pagination limits:

<select name="products-view" class="form-control products-view inline-pagination-menu" id="products-view">
    <option value="3">3</option>
    <option value="6">6</option>
    <option value="9">9</option>
</select>

Javascript responsible to change limit in URL

$('#products-view').change(function(){
    changeLimitPagination(this);
});

function changeLimitPagination(option){
    location.search = $.query.set('limit', option.value).toString();
}

I will admit that I am not really familiar with CakePHP so I am not sure how to answer "Exist any way to do this in cakephp style?" - did you ever find an answer for that?

I am intrigued by the following line:

@$limit = $this->request->query['limit'] ?: 3;

shouldn't the error control operator be prepended before the right hand side of the assignment operator?

$limit = @$this->request->query['limit'] ?: 3;

I haven't tried the error control operator on that side... maybe it still works the same...

Looking at the JavaScript code, it appears that the option parameter actually corresponds to the select list

$('#products-view').change(function(){
    changeLimitPagination(this);
});

function changeLimitPagination(option){
    console.log(' option tagName: ', option.tagName, ' id: ', option.id);
}
<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>
<select name="products-view" class="form-control products-view inline-pagination-menu" id="products-view">
    <option value="3">3</option>
    <option value="6">6</option>
    <option value="9">9</option>
</select>

So a better name would be something like productsList or limitList. Also, that doesn't really need to be a parameter - it could be stored in a variable outside the function. While you might still need to use jQuery for other code, regular JavaScript code could be used to select the element by id attribute with document.getElementById() and then add the change handler via EventTarget.addEventHandler(). That way, changeLimitPagination could simply be added as the callback to the change handler instead of an extra anonymous function that has the sole purpose of calling changeLimitPagination.

var productsView = document.getElementById('products-view');
productsView.addEventListener('change', changeLimitPagination);

function changeLimitPagination(){
    console.log('changeLimitPagintation() - value: ', productsView.value);
    //location.search = $.query.set('limit', option.value).toString();
}
<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>
<select name="products-view" class="form-control products-view inline-pagination-menu" id="products-view">
    <option value="3">3</option>
    <option value="6">6</option>
    <option value="9">9</option>
</select>


Insert array items into jQuery / JavaScript variable This code exists in my view; it makes use of a model passed through from the controller.

$("#email").emailautocomplete({
domains: [@foreach (var organisation in Model.Organisations)
             {
                @Html.Raw("\"") @organisation.EmailDomain
                @Html.Raw("\", \r\n")
             }
                @Html.Raw("]")
});

Which renders as:

$("#email").emailautocomplete({
domains: ["domain1.gov.uk",
"domain2.com",
"domain3.gov.uk",
"domain4.gov.uk",
]
});

This achieves what I want but I feel the razor syntax is messy and wonder if there is a cleaner way of passing the email domain strings into the JavaScript, to achieve the same rendered result.

I understand an ajax call is an option however I don't want to call the database again to get the list of organisations as the data has already been retrieved by the controller and passed into the view, and is used elsewhere in the view to populate a combo box.

I just see three issues:

You're making the view harder to read and you're missing a chance to reuse your code.
You're not properly escaping e-mail address (see RFC5322 and Wikipedia). If you don't then you're open to Code Injection attack (see later).
Some (older) browsers won't accept the trailing comma after the last array item (see MDN).

To solve the first issue you may write an extension method to build a JavaScript array string from an IEnumerable<T>:

static string AsJsArrayString<T>(this IEnumerable<T> items)
{
    var array = new StringBuilder();
    array.Append('[');
    array.Append(String.Join(",", items.Select(x => $"'{x}'")));
    array.Append(']');

    return array.ToString();
}

This code could be optimized and make more terse but I guess you understand the point:

$("#email").emailautocomplete({
    domains: @Html.Raw(Model.Organisations.Select(x => x.EmailDomain).AsJsArrayString())
});

This function may also be reused elsewhere.

Now we can look at second issue. E-mail addresses may contain invalid characters for a JavaScript string (for example " or '), JavaScript escaping is not the same as JSON escaping (done with Json.Encode()) but fortunately we have an ad-hoc function HttpUtility.JavaScriptStringEncode() (JavaScriptEncoder for .NET Core, see later):

static string AsJsArrayString(this IEnumerable<string> items)
    => "[" + String.Join(",", items.Select(x => "'" +  HttpUtility.JavaScriptStringEncode(x) + "'")) + "]";

Using String.Join() we also quickly fixed last issue in the list. If you can't/don't want to use an extension method you may also inline this function.

Last thing to change is that @Html.Raw() declaration into something less error-prone (I don't know you but I often forget @Html.Raw() if it does not always produce an invalid output):

@functions {
    public static string ToJsArrayString(IEnumerable<string> items)
    {
        return "["
          + String.Join(",", items.Select(x => "'" + HttpUtility.JavaScriptStringEncode(x) + "'"))
          + "]";
    }
}

Now we may simply write:

$("#email").emailautocomplete({
    domains: @ToJsArrayString(Model.Organisations.Select(x => x.EmailDomain))
});

HttpUtility is a .NET thing, in .NET Core you have a brand you class JavaScriptEncoder. Let's write an extension method for that:

public static string Encode<T>(this JavaScriptEncoder encoder, IEnumerable<T> items)
{
    return '[' + String.Join(",", items.Select(x => EncodeAndQuote(x))) + ']';

    string EncodeAndQuote(string text)
        => '"' + encoder.Encode(text) + '"';
}

Used like this:

$("#email").emailautocomplete({
    domains: @JavaScriptEncoder.Encode(Model.Organisations.Select(x => x.EmailDomain))
});

Code injection. Unfortunately I've not been clear, domain names can contain characters that need to be escaped. In case you think it's not an issue: would you allow your users to have a perfectly valid and usable email address that, however, will break your Javascript code (I guess in an administrative page) with a chance to execute arbitrary code? I would not.

Let's pick this perfectly valid e-mail address: john.smith@(here\I\can\write\anything)example.com. If not escaped then it will break your JavaScript code. An easy way to block your service until you will find the issue and fix the problem.

Worse that that, they can execute code with the privileges of the user of that page (and it seems an administrative page). Imagine they write this perfectly valid e-mail address:

john.smith@(",$.post('/api/customers/delete'),")example.com

Your JavaScript code isn't broken but you're executing code they injected, in this case an innocent AJAX POST.

In short: do not EVER trust user input, even when you think you do not need escaping.


Basic mortgage calculator I'm making a few forms of a mortgage calculator for my intro to computer science class. With inputs, I currently have it outputting the monthly payments. I'll eventually be outputting how long it will take to pay off the loan once I add some code. I'm just looking for suggestions for any easier/quicker ways to do it. It currently compiles and runs fine.

#include "math.h"
#include <iostream>
using namespace std;

int main()
{
   int a; //amount of the loan
   double i; // the loan interest rate
   int y; //years of the loan
   int t = 12; //loan term in months
   double mPayment; //variable for ouputting the payment

   cout.setf(ios::fixed);
   cout.setf(ios::showpoint);
   cout.precision(2);

   cout << "\n Enter the amount of loan in $ = ";
   cin >> a;
   cout << "\n Enter the interest rate = ";
   cin >> i;
   cout << "\n Enter years of the loan = ";
   cin >> y;

   mPayment = (a * i) / (1 - pow(1+i,-t)); //Formula to figure mortgage payment amount

   cout<< "\nYour Monthly Payment Amount is: $"<< mPayment; //prints out montyly payment amount

   return 0;
}

"math.h" should be <cmath>.  The brackets are for system libraries whereas the quotes are for user-defined headers.  The .h ending is mostly for C.
You don't actually need return 0 at the end of main().  It will do this same return for you at the very end.
You should consider input validation, at least if you decide to expand on this outside of the class.  You could even allow the user the option to input via the command line, making this a bit more convenient.
You don't need those last two comments since they're already obvious.  Only use them where explanation is needed for you and/or the reader.
Consider having the calculation done in a function for better maintainability:

// add this to better explain that value
const int loan_term = 12;

double calculateMortgagePayment(int loan, double interest_rate)
{
    return (loan * interest_rate) / (1 - pow(1 + interest_rate, -loan_term));
}

int main()
{
    // ...

    // could also be const since value shouldn't change later
    const double mortgagePayment = calculateMortgagePayment(loan, interest_rate);

    // ...
}

I've taken into account@Conduit's advice regarding variable names.  I've also kept the double return type, which isn't really ideal for money.  There are resources online that explain more about that.


Converting a SecureString to a byte array Does it allocates something that hasn't freed? Something that remains in memory? Except result byte array, ofc.

private unsafe byte[] ToByteArray(SecureString secureString, Encoding encoding = null)
{
    if (secureString == null)
        throw new ArgumentNullException(nameof(secureString));
    if (encoding == null)
        encoding = Encoding.UTF8;

    int maxLength = encoding.GetMaxByteCount(secureString.Length);

    IntPtr bytes = Marshal.AllocHGlobal(maxLength);
    IntPtr str   = Marshal.SecureStringToBSTR(secureString);

    try
    {
        char* chars = (char*)str.ToPointer();
        byte* bptr  = (byte*)bytes.ToPointer();
        int len     = encoding.GetBytes(chars, secureString.Length, bptr, maxLength);

        byte[] _bytes = new byte[len];
        for (int i = 0; i < len; ++i)
        {
            _bytes[i] = *bptr;
            bptr++;
        }

        return _bytes;
    }
    finally
    {
        Marshal.FreeHGlobal(bytes);
        Marshal.ZeroFreeBSTR(str);
    }
}

Although you have done a decent job, you could do this in a better way, without even to use unsafe.

First thing to mention, you should always use braces {} although they are optional for single lined if statements. This will just make your code less error prone which is in such a security context wanted.

Instead of using if (encoding==null) { encoding = Encoding.UTF8; } you can use the null coalescing operator ??.

How I use this usually (integrated in your method)

private byte[] ToByteArray(SecureString secureString, Encoding encoding = null)
{
    if (secureString == null)
    {
        throw new ArgumentNullException(nameof(secureString));
    }

    encoding = encoding ?? Encoding.UTF8;

    IntPtr unmanagedString = IntPtr.Zero;
    try
    {
        unmanagedString = Marshal.SecureStringToGlobalAllocUnicode(secureString);

        return encoding.GetBytes(Marshal.PtrToStringUni(unmanagedString));
    }
    finally
    {
        if (unmanagedString != IntPtr.Zero)
        {
            Marshal.ZeroFreeBSTR(unmanagedString);
        }
    }
}

Based on the valid comment

  Marshal.PtrToStringUni allocates managed string so until GC clear this we will have unprotected string with sensitive data in memory.

I would like to suggest another aproach which depends on your implementation. The main problem I see is that with your implementation the returned byte[] needs to be cleaned up from the caller of the code.

How about having a class which implements IDisposable which is wrapping the shown method ?

public sealed class SecureStringWrapper : IDisposable
{
    private readonly Encoding encoding;
    private readonly SecureString secureString;
    private byte[] _bytes = null;

    public SecureStringWrapper(SecureString secureString)
          : this(secureString, Encoding.UTF8)
    {}

    public SecureStringWrapper(SecureString secureString, Encoding encoding)
    {
        if (secureString == null)
        {
            throw new ArgumentNullException(nameof(secureString));
        }

        this.encoding = encoding ?? Encoding.UTF8
        this.secureString = secureString;
    }

    public unsafe byte[] ToByteArray()
    {

        int maxLength = encoding.GetMaxByteCount(secureString.Length);

        IntPtr bytes = IntPtr.Zero;
        IntPtr str   = IntPtr.Zero;

        try
        {
            bytes = Marshal.AllocHGlobal(maxLength);
            str   = Marshal.SecureStringToBSTR(secureString);

            char* chars = (char*)str.ToPointer();
            byte* bptr  = (byte*)bytes.ToPointer();
            int len     = encoding.GetBytes(chars, secureString.Length, bptr, maxLength);

            _bytes = new byte[len];
            for (int i = 0; i < len; ++i)
            {
                _bytes[i] = *bptr;
                bptr++;
            }

            return _bytes;
        }
        finally
        {
            if (bytes != IntPtr.Zero)
            {
                Marshal.FreeHGlobal(bytes);
            }
            if (str != IntPtr.Zero)
            {
                Marshal.ZeroFreeBSTR(str);
            }
        }
    }

    private bool _disposed = false;

    public void Dispose()
    {
        if (!_disposed)
        {
            Destroy();
            _disposed = true;
        }
        GC.SuppressFinalize(this);
    }

    private void Destroy()
    {
        if (_bytes == null) { return; }

        for (int i = 0; i < _bytes.Length; i++)
        {
            _bytes[i] = 0;
        }
        _bytes = null;
    }

    ~SecureStringWrapper()
    {
        Dispose();
    }
}

now the caller could use it like so

using (SecureStringWrapper wrapper = new SecureStringWrapper(secureString))
{

    byte[] _bytes = wrapper.ToByteArray();
    // use _bytes like wanted

}

A good read about the topic of SecureString: http://web.archive.org/web/20090928112609/http://dotnet.org.za/markn/archive/2008/10/04/handling-passwords.aspx


Python 3 palindrome checker I am writing a python3 implementation of palindromes checker.

import string

def is_palindrome(text):
    """A string of characters is a palindrome if it reads the same forwards and
    backwards, ignoring punctuation, whitespace, and letter casing"""
    # implement is_palindrome_iterative and is_palindrome_recursive below, then
    # change this to call your implementation to verify it passes all tests
    assert isinstance(text, str)
    return is_palindrome_iterative(text)
    # return is_palindrome_recursive(text)

def is_palindrome_iterative(text):
    word = text

    # if the word[index] is not the same, return False
    # iteration through the index from  the half of the length
    # doing it through
    for index in range(len(word)//2):
        if word[index] != word[-1-index]:
            return False
    return True

def main():
    import sys
    args = sys.argv[1:]  # Ignore script file name
    if len(args) > 0:
        for arg in args:
            is_pal = is_palindrome(arg)
            result = 'PASS' if is_pal else 'FAIL'
            str_not = 'a' if is_pal else 'not a'
            print('{}: {} is {} palindrome'.format(result, repr(arg), str_not))
    else:
        print('Usage: {} string1 string2 ... stringN'.format(sys.argv[0]))
        print('  checks if each argument given is a palindrome')

if __name__ == '__main__':
    main()

I think it works well.

ran python3 palindromes.py 'ABA'
output is
PASS: 'ABA' is a palindrome

1

I personally like to reduce the amount of string assembling where possible, so instead of:

result = 'PASS' if is_pal else 'FAIL'
str_not = 'a' if is_pal else 'not a'
print('{}: {} is {} palindrome'.format(result, repr(arg), str_not))

I would write:

PASS_TMPL = "PASS: {!r} is a palindrome"
FAIL_TMPL = "FAIL: {!r} is not a palindrome"
s = (PASS_TMPL if is_pal else FAIL_TMPL).format(arg)

In your example you had 3 variables in the string, in my example it's reduced to just 1 variable and 2 different templates.

of-course in this small program probably it doesn't matter which style you use. However if you build larger application the later style will bring you a few benefits:

output is more easily predictable
easier to change the output string without having a need to refactor the code
when debugging, and having to find location in code which produced certain output, it will be easier to grep the code for the specific string in question, if there is less string assembling going on.
if you need to add i18n support for your application at some point in time, you will learn that the way you assembled the strings only works in english, and your code needs to be refactored in order to support other languages

Because both styles usually are equally easy to code, avoiding the first style and preferring the 2nd style I think is a good habit to pick up.

2

Instead of the for loop to compare every individual letter, you can compare if the word is equal reversed version of it:

return word == word[::-1]

word[::-1] will reverse the word

This is not only shorter to write, but also in interpreted languages like python should run much faster too. A simple loop in python is very slow compared to a simple loop in C. String comparison will use compiled C implementation instead of looping each letter in python's loop.


HTML parsing algorithm for extracting  tags My intention is to create a complete HTML parser, so far I made a basic algorithm that iterates trough text and extracts everything in an "a" tag.

It works on everything I tried, but I want a review of what I could do better or what I did right.

#include <iostream>
#include <stdio.h>
#include <vector>
#include <fstream>
#include <streambuf>
#include <sstream>

std::vector<std::string> performCheck(std::string check, std::string text)
{
        int iterator = 0;
        int second_iterator = 0;

        bool is_flag = false;

        std::string returnString = "";
        std::vector<std::string> returnVector;

        for(iterator = 0;iterator < text.size();iterator++)
        {
            if(text[iterator] == '<')
            {

                is_flag = true;

                for(second_iterator = 0;second_iterator < check.size();second_iterator++)
                {
                    if(text[iterator + second_iterator] == check[second_iterator])
                    {
                        returnString = returnString + text[iterator + second_iterator];
                    }
                    else
                    {
                        is_flag = false;
                        returnString = "";
                        break;
                    }
                }

                if(is_flag == true)
                {
                    for(second_iterator = iterator + second_iterator;second_iterator < text.size();second_iterator++)
                    {
                        returnString = returnString +  text[second_iterator];

                        if(text[second_iterator - 2] == '/' && text[second_iterator - 1] == 'a')
                        {
                            returnVector.push_back(returnString);
                            break;
                        }
                    }
                }

            }

        }

    return returnVector;
}

int main(int argc, char** argv) {

    std::string check = "<a href=\"";
    std::vector<std::string> urlmap;

    std::ifstream t("your_file.ext");
    std::stringstream buffer;
    buffer << t.rdbuf();

    std::string textBig = buffer.str();

    urlmap = performCheck(check, textBig);

    for(int iterator = 0;iterator < urlmap.size();iterator++)
    {
        std::cout<<urlmap[iterator]<<std::endl;
    }

    system("PAUSE");

    return 0;
}

First of all, as Loki mentioned in the comments, it is incredibly cumbersome to parse any text that made by human. We tend to abuse things, so I'd refrain from parsing it myself. There are good libraries that does parsing. If you want performance, I'd go with boost.spirit that is based on expression templates. I've also made a topic request about expression templates on so.docs, but it still has low upvote count, so don't rely on it to come in near future. I strongly encourage you to read about both, because it is the greatest power to have in parsing input files.
About the code:
Good try, but probably you will need a lot more. Some people would think that I need to put suggestion about regex over hand rolled search, but I will just put
link to an article on very famous blog.
Use <cstdio> instead of <stdio.h>. All C headers have C++ equivalents and are preferred over C ones.
Don't mix <cstdio> and <iostream>. The I/O may become unordered.
Overflow:
int can and will overflow. For example here:
iterator + second_iterator

The C standard guarantees only 16 bit for it as I remember. It is not mandated by the standard that return type of the std::string::size() is int. In fact it will probably never be int, because it is signed type, but size can not be negative. I recommend you to use std::string::size_type. It is guaranteed by the standard that this type will have enough capacity to store the maximum possible index that std::string can have.
Readability comments:
second_iterator - 2
second_iterator - 1

Those 2 expressions rise alerts in my head, because type of second_iterator is int, which is signed. I know that it will never be less than 2 by design, but I think that there is at least one better solution, one of which is mentioned above.
I'd wait bool(s) from any function that has check word in it's name. I think the function now performs what sequence of std::string::find()s does. This suggests using standard one. It will be much more clear.
Performance comments:
Usually standard containers are implemented in terms of 3 pointers, not a pointer and 2 indices. This suggests that iterator based algorithms will outperform index based ones. Further reading.
I found it impractical using push_back() without preceding reserve() call where performance matters. On top of that, every push will have control flow manipulators, which will greatly decrease the throughput on the function.
I'd use std::string::compare to compare check string and the substring. The implementations are allowed to deploy more intelligent comparison functions, so it might have zero development cost optimization.
Redesign:
I'd redesign the function to accept Input Iterators. It will make it more customizable with zero runtime overhead (well it depends on the iterator, but it is possible and easy to do). It will also keep strings from being copied.
Notes about example code:
You can use range for loop to traverse whole container/range.
Declaring urlmap right on assignment would trigger copy elision (the output will start constructing right in urlmap).
Don't use system(). It is the source of many security issues.


Search filter list using jQuery I looked at a search filter code code and edited it to catered to my functional needs. I understood most of the JavaScript except the extend method bit. Can someone explain that?  Also, is this the best way to create such a filter?  Please let me know of any ways to improve the code.

    <input type="text" id="search-text" placeholder="search">

red

orange

yellow

pink

blue

green

violet

purple

Indigo

grey

white

black

  No items to show..

The extend method is being applied to jQuery.expr which is the set of selectors jQuery will apply when selecting elements (see here) -- jQuery.expr[":"] are the pseudo selectors jQuery acknowledges. The extend is useful if you want to add multiple jQuery selectors succinctly but for adding pseudo selectors I think its just a norm noting you can also add the selector as below

//extends :contains to be case insensitive
$.expr[':'].containsi = function(elem, i, match, array){
    return (elem.textContent || elem.innerText || '').toLowerCase().indexOf((match[3] || "").toLowerCase()) >= 0;
};

One more note: you probably want to add your custom selectors near the top of your code (and definitely outside of your keyup event handler!) in case you want to use your selector somewhere else in your code.

Now some nitpicks

Your Javascript near the end becomes difficult to read because the formatting of your keyup handler. Try pasting your code through an automatic formatter and notice how much easier it will be to follow (eg http://prettydiff.com)

if(itemCount == '0'){ //dont compare to '0' mate we know its an integer as its the length of a collection
  $('.empty-item').show();
}
else {
  $('.empty-item').hide();
}

I would write that as simply: $('.empty-item').toggle(itemCount === 0);

You can also write the following code in this similar one liner:

$("#list li:containsi('" + searchSplit + "')").each(function(e) {
    $(this).removeClass('hidden');
});

//more elegant:
$("#list li:containsi('" + searchSplit + "')").removeClass('hidden');


Comma delimited string from list of items Is there a simple way to create a comma delimited string from a list of items without adding an extra ", " to the end of the string?

I frequently need to take an ASP.NET CheckBoxList and format the selected values as a string for insertion into an e-mail. It's straightforward to loop over the selected checkboxes, get the values, and add them to a StringBuilder with a ", " separating them, but the Count property returns the number of items in the list total, not the number of items that are actually selected. So it's hard to know when you've reached the last item, and you end up with a stray ", " on the end.

I've played around with several different approaches to this problem. Lately I've been doing something like this:

private string ConcatenateCheckBoxListItems()
{
    StringBuilder sb = new StringBuilder();
    char[] characters = new char[] {" ", ","};
    String trimmedString;

    foreach (ListItem item in MyList)
    {
        sb.Append(item.Value + ", ");
    }

    trimmedString = sb.ToString().TrimEnd(characters);
    return trimmedString;
}

But it's definitely clunky. So do the various for loop approaches I've tried. Any suggestions for a cleaner implementation?

Would this not suffice?

private string ConcatenateCheckBoxListItems()
{
    return string.Join(", ", from item in MyList select item.Value);
}


Processing buy and sell orders My code is too slow, it does the desired, but not in \$O(\log n)\$ or faster.

Explanation of the auction system:

For simplicity we will assume that all orders are for a single unit of the traded item. The
double action system operates as follows. When a new buy order of $x arrives it is simply
stored for future use if none of the stored sell orders has an asking price lower than or equal
to $x. Otherwise, the new buying bid is matched with the sell order with lowest asking price
(earliest such order if there are ties). The two matched orders are removed from the system,
and the buyer receives the item in exchange for paying a clearing price equal to the matched
sell order asking price, i.e., the lowest asking price available at the time the new buy order
was received.

It's supposed to be a simple program that takes orders per line and sells or buys. The program must read from the standard input a list of buy and sell orders in the format given in the samples below, with one order per line. The program must print to the standard output the number of units exchanged and the total clearing price, using the format given in the examples below, and printing a new line at the end. Note that after processing the input there can be orders left uncleared.

Sample Input

order 1 sell at 30
order 2 buy at 20

Output for Sample Input

units exchanged 0 total clearing price 0

public static void main(String args[]) {
  // Create an array list for order to read data from the file
  ArrayList<String> orders = new ArrayList<String>();

  // Create sell and buys array list to get the list of
  // buyers and sellers
  ArrayList<String> sells = new ArrayList<String>();
  ArrayList<String> buys = new ArrayList<String>();

  // declare the required variables
  int price = 0;
  int exchange = 0;

  // since we are using files put the execution code in try..catch
  // block
  try {
      // create a file object by passing it to scanner class
      // to read the file data
      Scanner input = new Scanner(System.in);

      // read the lines from the text file
      while (input.hasNextLine()) {
           orders.add(input.nextLine());
      }

      // separate the orders
      for (int i = 0; i < orders.size(); i++) {
           System.out.println("" + orders.get(i));

           if (orders.get(i).contains("sell")) {
                sells.add(orders.get(i));
           }

           if (orders.get(i).contains("buy")) {
                buys.add(orders.get(i));
           }

      }

      // now get the price values from each line from the
      // respective orders and compare the values
      int buy = 0;
      int sell = 0;

      for (int i = 0; i < buys.size(); i++) {
           buy = getPrice(buys.get(i));
           for (int j = 0; j < sells.size(); j++) {
                sell = getPrice(sells.get(j));

                // if the values are same, then compute the clear price
                // values
                // and increment the exchange value
                if (buy == sell) {
                     price += 2 * buy;
                     exchange += 2;
                }
           }
      }

      // print the final statement.
      System.out.println("\nUnits of exchanged " + exchange + " total clearing price "+ price);
  } catch (Exception e) {
      e.printStackTrace();
  }
}

// getPrice method that retrieves the integer values from
// each line
 public static int getPrice(String s) {
  // use the scanner object to access the line values
  Scanner in = new Scanner(s).useDelimiter("[^0-9]+");

  int i = 0;
  int j = 0;

  // get the integer values
  while (in.hasNext()) {
      i = in.nextInt();
      j = in.nextInt();
  }

  // return the required price value
  return j;
}

You should always convert the input data in something sane ASAP. Dealing with string can get inefficient and unreadable pretty soon. Something like

enum Type {SELL, BUY}

@RequiredArgsConstructor
@Getter
@ToString
class Order {
    private final Type type;
    private final int id;
    private final int price;
}

sounds good. The above annotations are from Lombok, but you can write the boilerplate manually if you really want to.

By using a class instead of extracting the information from the string again and again, you'll surely gain a factor of 10+. That's good, but the more important part is the quadratic complexity.

  Otherwise, the new buying bid is matched with the sell order with lowest asking price (earliest such order if there are ties).

This sounds like a Comparator: Order by price and then by id. I'd probably go for Guava's ComparisonChain, but it's simple enough:

class OrderComparator implements Comparator<Order> {
    public int compare(Order o1, Order o2) {
        int result = Integer.compare(o1.price, o2.price);
        if (result != 0) {
            return result;
        }
        return Integer.compare(o1.id, o2.id);
}

In theory, we could create an array of all sells and use Arrays.sort with our OrderComparator on it. However, when a new sell comes in, we'd need to insert it in the appropriate position or sort the array again. Too bad.

But we need no sorted array. All we need is

to get the least element
remove it if it's good enough
add a new element

Fortunately, there's a PriorityQueue, which provides these operations efficiently (O(log n)). So we're nearly done.

A piece of code could look like

private void processNextBuy(Order buy) {
    assert buy.type == Type.BUY;
    // just look at it, but don't remove yet
    Order sell = sellQueue.peek();
    if (sell!=null && sell.price <= buy.price) {
        sellQueue.poll(); // paired, so remove it
        price += 2 * buy;
        exchange += 2;
    } else {
        buyQueue.add(buy); // unpaired, so process later
    }
}

From the description, I'm unsure if a new sell should be processed the same way. The boring parts around are left as an exercise for the reader. ;)


Different selects in one form with same name attribute? I have the following scenario:

    class Product < ActiveRecord::Base
        belongs_to :categories
    end

    class Category < ActiveRecord::Base
        has_many :products
    end

Categories table has 2 level nesting, for example.
Main category is 'Men', sub-category is 'Accessories' and sub-sub-category is 'Watches'.

Now when user creates new product he must choose category(only main category is required, but he can also chose sub and sub-sub category optional).

My idea is this: I created 3 different select boxes with same name "product[category_id]", so only the last selected value will be sent to the server through params[:product][:category_id].

     Main category? required
    <%= f.select :category_id,
            Category.where('category_id IS ?', nil).collect {|c| [c.name, c.id]},
            { include_blank: "Select category..." },
            id: 'main-category', class: 'form-control' %>

     What type? optional
    <%= f.select :category_id, {}, {}, class: 'form-control' %>

     What type? optional
    <%= f.select :category_id, {}, {}, class: 'form-control' %>

2nd(sub-categories) and 3rd(sub-sub-categories) are initial empty ({}, {}) and hidden through CSS(display: none) and will be populated through Ajax call.

$('#main-category, .category-level-1 > select').change(function() {
    var selectBox = this.id;
    var selectedValue = $(this).val();
    var url = '/categories/' + selectedValue + '/subcategories';

    $.get(url, function(data) { createSelect(data, selectBox); });
});

function createSelect(data, selectBox) {
  var $currentSelect = null;

  if (selectBox == 'main-category') {
    $('.category-level-1').show();
    $('.category-level-2').hide();
    $('.category-level-1 > select').find('option').remove();

    $currentSelect = $('.category-level-1 > select');
  }
  else {
    $('.category-level-2').show();
    $('.category-level-2 > select').find('option').remove();

    $currentSelect = $('.category-level-2 > select');
  }

  $currentSelect.append('<option selected disabled>Select Category...</option>');
  for(var i=0; i<data.length; i++) {
    $currentSelect.append('<option value="' + data[i].id + '">' + data[i].name +
                          '</option>');
  }
}

Where Ajax call is sent to following route (categories controller)

def subcategories
    id = params[:id]
    @subcategories = Category.where('category_id = ?', id)
    render json: @subcategories
end

So for final result i have following:

First of all, is it normal to have different inputs in one form with same names created manually like I did in this example?  For some reason, it seem like 'code-smell' to me. I'm relatively new to Rails, so I wanted to ask if I am violating some good practices with this code.  Can you suggest better ways of achieving the same results?

These are nested selects, you can try awesome_nested_set, and use it like this:

<%= f.select :parent_id,
             nested_set_options(Category, @category) {|i| "#{'-' * i.level} #{i.name}" } %>


Converting CSV files to Excel workbooks My converter converts all CSV files in the subfolders of folders 1, 2 & 3 into Excel workbooks. As of now, I am converting using codes for each folders. I previously tried to combine those into one using for loop, but an error occurred, so I've rolled back to the working code I had before the loop.

Can anyone show me how to clean this up with a loop or another method?

Private Sub CommandButton1_Click()

 Dim CSVfolder As String, CSVfolder1 As String, CSVfolder2 As String
 Dim fname, fname1, fname2 As String
 Dim wBook As Workbook
 Dim colSF As Collection
 Dim vFile, vFile1, vFile2
 Dim bHadFiles As Boolean
 CSVfolder = "C:\Charts\1\"
 CSVfolder1 = "C:\Charts\2\"
 CSVfolder2 = "C:\Charts\3\"

 Set colSF = GetSubFolders(CSVfolder)
 For Each vFile In colSF
 fname = Dir(CSVfolder & vFile & "\" & "*.csv")
 Do While fname <> ""
    bHadFiles = True
    Application.ScreenUpdating = False
    Set wBook = Workbooks.Open(CSVfolder & vFile & "\" & fname, Format:=6, Delimiter:=",")
    wBook.SaveAs CSVFolder & vFile & "\" & Replace(fname, ".csv", ""), xlOpenXMLWorkbook
    Application.CutCopyMode = False
    wBook.Close False
 fname = Dir()
 Loop
 If bHadFiles Then Kill CSVfolder & vFile & "\" & "*.csv"
 Next

Set colSF = GetSubFolders(CSVfolder1)
For Each vFile1 In colSF
fname1 = Dir(CSVfolder1 & vFile1 & "\" & "*.csv")
Do While fname1 <> ""
    bHadFiles = True
    Application.ScreenUpdating = False
    Set wBook = Workbooks.Open(CSVfolder1 & vFile1 & "\" & fname1, Format:=6, Delimiter:=",")
    wBook.SaveAs CSVFolder1 & vFile1 & "\" & Replace(fname1, ".csv", ""), xlOpenXMLWorkbook
    Application.CutCopyMode = False
    wBook.Close False
fname1 = Dir()
 Loop
 If bHadFiles Then Kill CSVfolder1 & vFile1 & "\" & "*.csv"
 Next

Set colSF = GetSubFolders(CSVfolder2)
For Each vFile2 In colSF
fname2 = Dir(CSVfolder2 & vFile2 & "\" & "*.csv")
Do While fname2 <> ""
    Application.ScreenUpdating = False
    Set wBook = Workbooks.Open(CSVfolder2 & vFile2 & "\" & fname2, Format:=6, Delimiter:=",")
    wBook.SaveAs CSVFolder2 & vFile2 & "\" & Replace(fname2, ".csv", ""), xlOpenXMLWorkbook
    Application.CutCopyMode = False
    wBook.Close False
 fname2 = Dir()
 Loop
 If bHadFiles Then Kill CSVfolder2 & vFile2 & "\" & "*.csv"
 Next
 Application.ScreenUpdating = True
 End Sub

Function GetSubFolders(sPath As String) As Collection
    Dim col As New Collection, f
    f = Dir(sPath, vbDirectory + vbNormal)
    Do While f <> ""
        If GetAttr(sPath & f) And vbDirectory Then
            If f <> "." And f <> ".." Then col.Add f
        End If
        f = Dir()
    Loop
    Set GetSubFolders = col
End Function

The very first thing we need to do is fix the indentation. If we can't read the code, we can't make it better. Everything inside of Sub...End Sub should be one level in. Add another level when you enter an If, For, For Each, or Select.

Sub Foo

    ' some code

    Set colSF = GetSubFolders(CSVfolder)
    For Each vFile In colSF
        fname = Dir(CSVfolder & vFile & "\" & "*.csv")
        Do While fname <> ""
            bHadFiles = True
            Application.ScreenUpdating = False
            Set wBook = Workbooks.Open(CSVfolder & vFile & "\" & fname, Format:=6, Delimiter:=",")
            wBook.SaveAs CSVfolder & vFile & "\" & Replace(fname, ".csv", ""), xlOpenXMLWorkbook
            Application.CutCopyMode = False
            wBook.Close False
            fname = Dir()
        Loop
        If bHadFiles Then Kill CSVfolder & vFile & "\" & "*.csv"
    Next

    ' more code

End Sub

You had the right idea with the loop. A loop will definitely clean this up immensely, but before we get to a loop, first let's extract a method to remove the duplication.

Private Sub CommandButton1_Click()
    Application.ScreenUpdating = False

    Dim CSVfolder As String, CSVfolder1 As String, CSVfolder2 As String
    CSVfolder = "C:\Charts\1\"
    CSVfolder1 = "C:\Charts\2\"
    CSVfolder2 = "C:\Charts\3\"

    TransformFile CSVfolder
    TransformFile CSVfolder1
    TransformFile CSVfolder2

    Application.ScreenUpdating = True
End Sub

Private Sub TransformFile(ByVal CSVfolder As String)
    Dim fname As String
    Dim vFile
    Dim colSF As Collection
    Dim wBook As Workbook

    Set colSF = GetSubFolders(CSVfolder)
    For Each vFile In colSF
        fname = Dir(CSVfolder & vFile & "\" & "*.csv")
        Do While fname <> ""
            bHadFiles = True
            Set wBook = Workbooks.Open(CSVfolder & vFile & "\" & fname, Format:=6, Delimiter:=",")
            wBook.SaveAs CSVfolder & vFile & "\" & Replace(fname, ".csv", ""), xlOpenXMLWorkbook
            Application.CutCopyMode = False
            wBook.Close False
            fname = Dir()
        Loop
        If bHadFiles Then Kill CSVfolder & vFile & "\" & "*.csv"
    Next
End Sub

Note that all I did was move the code into it's own method and call it appropriately. There's still no loop, but now moving to a loop is both trivial and almost unnecessary. We like clean code around here though, so let's go ahead and do that.

Private Sub CommandButton1_Click()
    Application.ScreenUpdating = False

    Dim folders As New Collection
    folders.Add "1"
    folders.Add "2"
    folders.Add "3"

    'must be a variant in order to loop over a string collection
    'using a string array instead of a collection is another good option
    Dim CSVFolder As Variant
    For Each CSVFolder In folders
        TransformFile CSVFolder
    Next

    Application.ScreenUpdating = True
End Sub

I noticed you have this is a code behind, you may want to move this code to it's own module or class and call it from the click handler. That way your logic isn't bound up in the GUI where it can't be re-used. The only other thing to mention here is that if you're turning the screen updating off, then you must use an error handler to ensure that it always gets turned back on.

But we're not done yet, we extracted that method out, but left our mess hidden away in there. Let's clean it up too.

Private Sub TransformFile(ByVal CSVFolder As String)
    Dim fname As String
    Dim vFile
    Dim colSF As Collection
    Dim wBook As Workbook

    Set colSF = GetSubFolders(CSVFolder)
    For Each vFile In colSF
        fname = Dir(CSVFolder & vFile & "\" & "*.csv")
        Do While fname <> ""
            bHadFiles = True
            Application.ScreenUpdating = False
            Set wBook = Workbooks.Open(CSVFolder & vFile & "\" & fname, Format:=6, Delimiter:=",")
            wBook.SaveAs CSVFolder & vFile & "\" & Replace(fname, ".csv", ""), xlOpenXMLWorkbook
            Application.CutCopyMode = False
            wBook.Close False
            fname = Dir()
        Loop
        If bHadFiles Then Kill CSVFolder & vFile & "\" & "*.csv"
    Next
End Sub

How many times do you need to concatenate this path together?

CSVFolder & vFile & "\" & "*.csv"

Yikes! Do it once before you enter the while loop.

For Each vFile In colSF

    filePath = CSVFolder & vFile & "\"

    fname = Dir(filePath & "*.csv")
    Do While fname <> ""
        bHadFiles = True
        Set wBook = Workbooks.Open(filePath & fname, Format:=6, Delimiter:=",")
        wBook.SaveAs filePath & Replace(fname, ".csv", ""), xlOpenXMLWorkbook
        Application.CutCopyMode = False
        wBook.Close False
        fname = Dir()
    Loop
    If bHadFiles Then Kill filePath & "*.csv"
Next

No offense, but WTF is colSF? It's a collection of folder names, right? Then just call it that. While we're at it, burn the hungarian notation. The name hadFiles already tells us that it's a boolean. I'm also going to add some vertical white space to group related actions together. Oh, and I'm going to remove Application.CutCopyMode = False. I seriously have no idea what it's doing here. It's not doing anything important at least.

Private Sub TransformFile(ByVal CSVFolder As String)
    Dim filename As String
    Dim vFile As Variant
    Dim folderNames As Collection
    Dim wBook As Workbook
    Dim hadFiles As Boolean
    Dim filePath As String

    Set folderNames = GetSubFolders(CSVFolder)
    For Each vFile In folderNames

        filePath = CSVFolder & vFile & "\"

        filename = Dir(filePath & "*.csv")
        Do While filename <> ""
            hadFiles = True

            Set wBook = Workbooks.Open(filePath & filename, Format:=6, Delimiter:=",")
            wBook.SaveAs filePath & Replace(filename, ".csv", ""), xlOpenXMLWorkbook
            wBook.Close False

            filename = Dir()
        Loop

        If hadFiles Then Kill filePath & "*.csv"
    Next
End Sub

In general, don't oneline If statements. It makes them hard to visually parse. This is doubly important on lines of code that Kill files.

If hadFiles Then
    Kill filePath & "*.csv"
End If

Things are getting better, but there's still a lot of variables. We can remove one by leaning on the face that colSF (i.e. folderNames) is never used as anything but an iterator.

    Set folderNames = GetSubFolders(CSVFolder)
    For Each vFile In folderNames

Becomes

    For Each vFile In GetSubFolders(CSVFolder)

Don't worry about the function getting called repeatedly, it won't. It executes once and then we're iterating over the collection that it returned.

Here's the code I ended up with. It's probably not getting much simpler unless you switch to the more powerful FileSystemObject in the Scripting Runtime. I encourage you to take a look at what's available there and leave it as an exercise for you to implement this using it instead.

Private Sub CommandButton1_Click()
    Application.ScreenUpdating = False
    Const rootFolder As String = "C:\Charts\"

    Dim folders As New Collection
    folders.Add "1"
    folders.Add "2"
    folders.Add "3"

    Dim CSVFolder As Variant 'must be a variant in order to loop over a string collection
    For Each CSVFolder In folders
        TransformFile rootFolder & CSVFolder & "\"
    Next

    Application.ScreenUpdating = True
End Sub

Private Sub TransformFile(ByVal CSVFolder As String)
    Dim filename As String
    Dim vFile As Variant
    Dim wBook As Workbook
    Dim hadFiles As Boolean
    Dim filePath As String

    For Each vFile In GetSubFolders(CSVFolder)

        filePath = CSVFolder & vFile & "\"

        filename = Dir(filePath & "*.csv")
        Do While filename <> ""
            hadFiles = True

            Set wBook = Workbooks.Open(filePath & filename, Format:=6, Delimiter:=",")
            wBook.SaveAs filePath & Replace(filename, ".csv", ""), xlOpenXMLWorkbook
            wBook.Close False

            filename = Dir()
        Loop

        If hadFiles Then
            Kill filePath & "*.csv"
        End If
    Next
End Sub


Game of Life component in React I have been learning webdevelopment at Freecodcamp and have therefore been writing a lot of javascript by myself. They have a code-review chatroom, but the people in that chatroom seem to review the final product and not the code itself.

So, I am searching for some constructive criticism about the structure and style of my code.

The code shown below is a class from a Game of Life game as part of the Freecodecamp course and does work. It is written in Javascript and uses React. There is more code for this project, but I don't think it is necessary to show all of the code, for you to critique my code structure and style. If it the rest of the code is required I can of course post it as well.

class GameOfLife extends Component {
  constructor(props) {
    super(props)
    this.toggleGameState = this.toggleGameState.bind(this);
    this.stepGame = this.stepGame.bind(this);
    this.squareClicked = this.squareClicked.bind(this);
    this.clearGrid = this.clearGrid.bind(this);
    this.generateRandomGrid = this.generateRandomGrid.bind(this);
    this.toggleSpeed = this.toggleSpeed.bind(this);
    this.state = {
      cellRows: generateRandomCellDistribution({height: 50, width: 50}),
      size: {height:50, width:50},
      gameState: 'playing', speed: 1,
      generation: 0
    };
  }

  componentDidMount() {
    this.setState({gamteState: 'playing', interval: setInterval(this.stepGame, 200/this.state.speed)});
  }

  toggleGameState() {
    var newState = this.state.gameState === 'playing' ? 'paused' : 'playing';
    if(newState === 'playing') {
      this.setState({gameState: newState, interval: setInterval(this.stepGame, 200/this.state.speed)});
    } else {
      this.setState({'gameState': newState});
      clearInterval(this.state.interval);
    }
  }

  squareClicked(row, column) {
    var cellRows = this.state.cellRows.slice();
    var newState = cellRows[row][column].state === 'alive' ? 'dead' : 'alive';
    cellRows[row][column] = {state: newState};
    this.setState({cellRows: cellRows});
  }

  generateRandomGrid() {
    this.setState({cellRows: generateRandomCellDistribution(this.state.size), generation: 0})
  }

  clearGrid() {
    this.setState({cellRows: getBlankCellRows(this.state.size), generation: 0});
  }

  toggleSpeed(){
    var newSpeed = this.state.speed < 4 ? this.state.speed+1 : 1;
    clearInterval(this.state.interval);
    this.setState({speed: newSpeed, interval: setInterval(this.stepGame, 200/newSpeed)});
  }

  stepGame() {
    var newCellRows = getBlankCellRows(this.state.size);
    var currentCellRows = this.state.cellRows.slice();
    var aliveNeighbours = getAliveNeighbours(currentCellRows);

    for(var i = 0; i < currentCellRows.length; i++) {
      for(var q = 0; q < currentCellRows[i].length; q++) {
        newCellRows[i][q].state = getNewState(currentCellRows[i][q].state, getAliveNeighbours(currentCellRows, i, q));
      }
    }

    this.setState({cellRows: newCellRows, generation: this.state.generation + 1});
  }

  render(){
    return(

          <button className="button-theme" onClick={this.toggleGameState}>{this.state.gameState === 'playing' ? 'Pause' : 'Start'}</button>
          <button className="button-theme" onClick={this.clearGrid}>Clear grid</button>
          <button className="button-theme" onClick={this.generateRandomGrid}>Random</button>
          <button className="button-theme" onClick={this.toggleSpeed}>Speed: {this.state.speed}x</button>

Generation: {this.state.generation}

        <Grid
          gameState={this.state.gameState}
          size={this.state.size}
          cellRows={this.state.cellRows}
          squareClicked={this.squareClicked}
        />

    );
  }
}

// Good: Object properties are in a logical order
// (alphabetical order isn't always best)
this.state = {
  cellRows: generateRandomCellDistribution({height: 50, width: 50}),
  size: {height:50, width:50},
  gameState: 'playing',
  speed: 1,       // This property should be on its own line like the rest.
  generation: 0
};

// Style should be consistent (Source: Code Complete)
// Object properties on their own lines like above
// (even as parameters).
componentDidMount() {
  this.setState({
    gamteState: 'playing',
    // No magic numbers (Source: Code Complete)
    // Why 200/speed? What does that represent?
    // Assign a variable above like var tickSpeed = 200/this.state.speed
    // Something to explain why you're doing the division.
    // Even better would be to put that in another function
    // to be used in all places you do this conversion.
    interval: setInterval(this.stepGame, 200/this.state.speed)
  });
}

// Function name should describe what the function does
// (rather than what triggers it) (Source: Code Complete)
squareClicked(row, column)  -->  handleClickInSquare(row, column) or updateStateForCell(row, column)

// Same here. Function doesn't just return a random grid,
// it changes the grid to be a random grid (affects state).
generateRandomGrid()  -->  setGridToRandomGrid() or randomizeGrid()

// This one is pretty good, but toggle usually refers to
// something with two options like Caps Lock (Source: Google define).
// This is tricky and you might think up an even better name that
// includes the facts that you're increasing the speed, there are
// several options, and it wraps around back to the first option.
toggleSpeed()  -->  cycleSpeed(), increaseSpeedOrWrap(), or selectNextFasterSpeed()

// It's better to select clear iterators although it's not
// uncommon to see i, j, and k. Not sure what q is, though.
// How about i --> row, q --> cell. (Source: Code Complete)
// I've changed the code to demonstrate my suggestions.
// I also recommend using currentCellGrid or similar
// to make it clear that you have a 2D array.
for(var row = 0; row < currentCellGrid.length; row++) {
  // Here you could add a variable to clarify what currentCellGrid[row] is.
  // Use more variables to document what the code is doing.
  // (Source: Code Complete)
  var currentRowCells = currentCellGrid[row];
  for(var cell = 0; cell < currentRowCells.length; cell++) {
    newCellGrid[row][cell].state = getNewState(currentRowCells[cell].state, getAliveNeighbours(currentCellGrid, row, cell));
  }
}


Vue.js: extracting data from a nested object using for...in I have a getter in my vuex module which gets data from a nested object, then check if there is any key match to the string and push these matched objects into an array. ESLint says that for..in loops iterate over the entire prototype chain, which is virtually never what I want. Its suggests to use Object.{keys,values,entries}, and iterate over the resulting array.
How can I refactor this?

getUsersData({ users: { data: { users } } }) {
  if (users) {
    const arr = Object.keys(users.stats || {}).map(key => users.stats[key]);
    const str = 'active';
    const newArr = [];
    arr.forEach((el) => {
      const obj = {};
      for (const key in el) {
        if (!key.includes(str)) {
          obj[key] = el[key];
        }
      }
      newArr.push(obj);
    });
    return newArr;
  }
  return [];
}

Your original question has a partial answer here I believe. In fact, ESLint is telling you the exact method(s) to use, which is Object.keys(el), for example.

Aside from that, there are a few semi-related things you can refactor...

Object.keys(users.stats || {}).map(key => users.stats[key]) is essentially the same as Object.values(users.stats || {}).

...as well as unrelated ones:

Names like arr, el, newArr, obj, key, and str are extremely abstract, and dealing with them in real code forces spending brain cycles on decrypting their meaning instead of understanding the intent of the code. In fact, I would hesitate to post a question before renaming the variable into something meaningful. See, the readers don't know the complete structure of the object(s) you're working with and it complicates the review process.
Unnecessary temporary objects may be avoided, if map(), filter(), reduce() and other chaining-friendly functions are used together.
A guarding construction if (!users) return [] is more idiomatic for JavaScript/TypeScript and reduces the nesting (fewer {s and }s).

The result may look something like this. I'm sorry if I missed something — I don't know how to reliably test that my refactoring went well. Plus, it's a late night in my timezone now, so my brain acts funny. :)

getUsersData({ users: { data: { users } } }) {
  if (!users) return [];

  return Object.values(users.stats || {})
    .map((statValues) => {
      Object
        .getOwnPropertyNames(statValues)
        .filter(key => !key.includes('active'))
        .reduce((result, key) => {
          result[key] = statValues[key];
          return result;
        }, {});
    });
}


Scrabble algorithm review and performance suggestions After finishing Project Euler 24, I decided to make a scrabble type of application using /usr/lib/dict as my dictionary what I cat that into a word file. It does take a few seconds, and the dictionary selection isn't that great.

Is there any way I could make it faster, more effective, and with a better dictionary source?

public class Scrabble {

    public static ArrayList<String> numbers = new ArrayList<String>();
    public static ArrayList<String> numbers2 = new ArrayList<String>() {
    };

    public static void main(String[] args) {
        Scanner dict = null;

        try {
            dict = new Scanner(new File("words.txt"));
        } catch (FileNotFoundException ex) {
        }

        while (dict.hasNextLine()) {

            numbers.add(dict.nextLine());
        }

        String n = "gojy";//random text here

        rearrange("", n);
        LinkedHashSet<String> listToSet = new LinkedHashSet<String>(numbers2);
        ArrayList<String> listWithoutDuplicates = new ArrayList<String>(listToSet);
        for (int i = 0; i < listWithoutDuplicates.size(); i++) {

            if (numbers.contains(listWithoutDuplicates.get(i))) {
                System.out.println(listWithoutDuplicates.get(i));
            }
        }

    }

    public static void rearrange(
            String q, String w) {
        if (w.length() <= 1) {
            String k = q + w;
            numbers2.add(k);//full word
            numbers2.add(q);//smaller combination to get words with less letters. doesn't work too well
        } else {

            for (int i = 0; i < w.length(); i++) {

                String c = w.substring(0, i)
                        + w.substring(i + 1);

                rearrange(q
                        + w.charAt(i), c);

            }
        }
    }
}

If what you are trying to do is store a huge list of valid words in such a way that you can test whether a given string is a valid word, a Trie works well.  See this Stack Overflow question.

Load the wordlist into the Trie when your server starts, and use the same Trie for all games (assuming this is a client server game).  It's trickier than the linked thread if you need to include more than the standard 26 letters of the alphabet, but I've done a Unicode Trie for a chat filter before and I could help if you need that.

I don't understand what you are trying to do with the rearrange method.  Can you explain?


Bank account data structure I need to model simple account data structure. The user should be able:

to add a transaction
get information about transaction
take all transaction of some type
sum a transaction and all its children

class AccountModel(object):
    def __init__(self):
        self.types = {}
        self.amounts = {}
        self.parent_to_chilren = {}
        self.children_to_parent = {}
        self.of_type = {}

    def add(self, id, amount, type, parentId=None):
        assert(
            not(id in self.amounts),
            'there is already a transaction with such id'
        )
        self.amounts[id] = amount
        self.of_type[id] = type

        lstOfType = self.types.get(type, [])
        lstOfType.append(id)
        self.types[type] = lstOfType

        if parentId:
            self.children_to_parent[id] = parentId
            lstOfChilren = self.parent_to_chilren.get(parentId, [])
            lstOfChilren.append(id)
            self.parent_to_chilren[parentId] = lstOfChilren

    def get(self, id):
        return (
            self.amounts[id], self.of_type[id],
            self.children_to_parent.get(id, None)
        )

    def get_list_by_type(self, type):
        return self.types[type]

    def sum(self, id):
        lst = self.parent_to_chilren.get(id, [])
        res = self.amounts[id]
        for i in lst:
            res += self.sum(i)
        return res

And the tests:

import unittest

from challenge import AccountModel
class AccountModelTestCase(unittest.TestCase):

    def setUp(self):
        self.model = AccountModel()

    def test_can_add_a_transaction_and_get_information(self):
        self.model.add(1, 10, 'car')
        self.assertEqual(
            self.model.get(1),
            (10, 'car', None),
        )

    def test_can_add_a_transaction_and_get_information_with_parent_id(self):
        self.model.add(1, 10, 'car')
        self.model.add(2, 10, 'car', 1)
        self.assertEqual(
            self.model.get(2),
            (10, 'car', 1),
        )

    def test_can_add_few_transactions_of_a_different_type_and_them(self):
        self.model.add(1, 10, 'car')
        self.model.add(2, 15, 'shopping')
        self.model.add(3, 18, 'car')
        self.assertItemsEqual(
            self.model.get_list_by_type('car'),
            [1, 3]
        )

    def test_can_add_few_transactions_of_a_different_type_and_calc_sum(self):
        self.model.add(1, 10, 'car')
        self.model.add(2, 15, 'shopping', 1)
        self.model.add(3, 18, 'car')
        self.assertEqual(
            self.model.sum(1),
            25
        )
        self.assertEqual(
            self.model.sum(2),
            15
        )
        self.assertEqual(
            self.model.sum(3),
            18
        )

    def test_can_add_few_transactions_parent_of_parent_of_parent(self):
        self.model.add(1, 10, 'car')
        self.model.add(2, 15, 'shopping', 1)
        self.model.add(3, 18, 'car', 2)
        self.assertEqual(
            self.model.sum(1),
            43
        )
        self.assertEqual(
            self.model.sum(2),
            33
        )

if __name__ == '__main__':
    unittest.main()

The questions:

Is there a more effective (in terms of used memory) implementation?
Is there a way to change a data structure and provide more effective (in terms of complexity) sum function implementation?

Your problem statement is a bit unclear,I'm uncertain what they mean by the children of a transition. In addition I would ask the interviewer what types (categories) of transaction it should support. Let's assume the basic ones, withdrawal, deposit and transfers.

I wouldn't store this information directly into the AccountModel, instead I would create a Transaction class which holds information regarding the transaction which is general and present for each type of transaction, namely: an id, amount, timestamp.

Then you can create subclasses Withdrawal, Deposit and Transfer, these can then be stored in a list of account which represents the history of the account's actions. An action is then performed by creating the action: account.perform(Withdrawal(10)), where accountis the an object of the AccountModelclass. In the perform()method I would check the type of transaction and update the account balance accordingly (which you store in the account directly). This way you don't need to use the sum()to run over the list of transactions each time.


Huffman tree decoding Description:

  Huffman coding assigns variable length codewords to fixed length input
  characters based on their frequencies. More frequent characters are
  assigned shorter codewords and less frequent characters are assigned
  longer codewords. A Huffman tree is made for the input string and
  characters are decoded based on their position in the tree. We add a
  '0' to the codeword when we move left in the binary tree and a '1'
  when we move right in the binary tree. We assign codes to the leaf
  nodes which represent the input characters.

  You have to decode an encoded string using the Huffman tree.

  You are given pointer to the root of the Huffman tree and a binary
  coded string. You need to print the actual string.

Code:

void decode(String S ,Node root)
    {
        if (root == null) return;
        StringBuilder sb = new StringBuilder();
        int pos = 0;
        Node current = root;
        char[] chars = S.toCharArray();
        while (pos < chars.length) {
            char c = chars[pos];
            if (c == '0' && current.left != null) {
                current = current.left;
            }
            else if (c == '1' && current.right != null) {
                current = current.right;
            }
            if (current.left == null && current.right == null) {
                sb.append(current.data);
                current = root;
            }
            pos++;
        }
        System.out.print(sb.toString());
    }

You could replace the while with a for-each loop. That will get rid of the otherwise unused pos variable, and make the code slightly more compact and idiomatic.

for (char c : S.toCharArray()) {
    // ...
}

Instead of printing the output in the same function that does the main computation, you could return the string result, to minimize the function's responsibilities.

Other than these minor points, the code is fine.