How to separate data from logic when serializing types - grid tile map example I am creating a tile based game in Unity and ran into a problem with my architecture of the Map and Tile types. The tile class contains logic like events when the tile type changes. When I tried to save the type to disk those events were being serialized as well, which I don't want (causing several problems in Unity). I basically want to make sure that I only save pure data like x, y position and enum types. Here is my approach:

public class Tile
{
    public event EventHandler<TileTypeChangedEventArgs> TileTypeChanged;

    public const TileType defaultType = TileType.Empty;

    public TileType type
    {
        get { return _type; }
        set
        {
            if (Enum.IsDefined(typeof(TileType), value) == false)
                throw new InvalidOperationException("Trying to set undefined tile type.");

            if (value != _type)
            {
                _type = value;
                var args = new TileTypeChangedEventArgs(_type);
                OnTileTypeChanged(args);
            }
        }
    }

    public int x { get { return _x; } }
    public int z { get { return _z; } }

    TileType _type;

    readonly int _x;
    readonly int _z;

    public Tile(int x, int z, TileType type = Tile.defaultType)
    {
        _x = x;
        _z = z;
        this.type = type;
    }

    protected virtual void OnTileTypeChanged(TileTypeChangedEventArgs e)
    {
        var handler = TileTypeChanged;
        if (handler != null)
            handler.Invoke(this, e);
    }
}

public class TileTypeChangedEventArgs : EventArgs
{
    public TileType type { get; private set; }

    public TileTypeChangedEventArgs(TileType type)
    {
        this.type = type;
    }
}

public enum TileType
{
    Empty,
    Grass,
    Water,
    Dirt
}

[Serializable]
public sealed class TileData
{
    public static readonly int version = 1;

    public TileType type
    {
        get { return _type; }
        set { _type = value; }
    }

    public int x
    {
        get { return _x; }
        set { _x = value; }
    }

    public int z
    {
        get { return _z; }
        set { _z = value; }
    }

    TileType _type;
    int _x;
    int _z;

    public static TileData Convert(Tile tile)
    {
        var data = new TileData();
        data.x = tile.x;
        data.z = tile.z;
        data.type = tile.type;
        return data;
    }

    public static Tile Convert(TileData data)
    {
        return new Tile(data.x, data.z, data.type);
    }
}

I created the TileData class because it would work similar to Data Transfer Objects in web programming. This way I am sure that my data remains serializable and I don't run into problems when I save a tile map and change it simultaneously.

However, it also means duplicated code and more methods for conversion. What do you think of my approach? Would you just modify the tile class so that it serializes correctly? I believe that I would also need to include copy methods to make sure saving to disk works while modifying the map state etc.

I have been experimenting and found this solution to be the best:

[Serializable]
public class Tile
{
    [field: NonSerialized]
    public event EventHandler<TileTypeChangedEventArgs> TileTypeChanged;

    [NonSerialized]
    public const TileType defaultType = TileType.Empty;

    public TileType type
    {
        get { return _type; }
        set
        {
            if (Enum.IsDefined(typeof(TileType), value) == false)
                throw new InvalidOperationException("Trying to set undefined tile type.");

            if (value != _type)
            {
                _type = value;
                var args = new TileTypeChangedEventArgs(_type);
                OnTileTypeChanged(args);
            }
        }
    }

    public int x { get; private set; }
    public int z { get; private set; }

    TileType _type;

    public Tile() { }

    public Tile(int x, int z, TileType type = Tile.defaultType)
    {
        this.x = x;
        this.z = z;
        this.type = type;
    }

    public Tile(Tile original)
    {
        this.x = original.x;
        this.z = original.z;
        this.type = original.type;
    }

    public override string ToString()
    {
        return string.Format("Tile: {0}|{1} = {2}", x, z, type);
    }

    protected virtual void OnTileTypeChanged(TileTypeChangedEventArgs e)
    {
        var handler = TileTypeChanged;
        if (handler != null)
            handler.Invoke(this, e);
    }
}

public class TileTypeChangedEventArgs : EventArgs
{
    public TileType type { get; private set; }

    public TileTypeChangedEventArgs(TileType type)
    {
        this.type = type;
    }
}

public enum TileType
{
    Empty,
    Grass,
    Water,
    Dirt
}

The problematic point was the accidental serialization of the event. I didn't know about the [field: NonSerialized] attribute for properties, but this works very well for the BinaryFormatter, XmlSerializer and Json.Net. I also had to add a little bit of ContractResolver logic for the later to deserialize private properties, but I found it much cleaner than having to supply more attributes in my data classes.

Another thing to note is the use of a copy constructor in the Tile class as well as in the Map which holds an array of tiles. When writing data to disk I copy everything before saving to avoid problems when I save on another thread while my users change the current map instance.

I'd still be happy to read any suggestions about how to improve my class design.


Is there a better way to optimize these conditionals? Public Function MyProjectFuncion(firstObject As Object,
                                 myEnumeration As Enumeration,
                                 theString As String,
                                 someCollection As List(of Object),
                                 anotherObject As Object) _
                            As String

    If IsNothing(firstObject) Then
        Throw New Exception("No data was received in 'firstObject'")
    Else
        If IsNothing(anotherObject) Then
            Throw New Exception("No data was received in 'anotherObject'")
        Else
            If IsNothing(myEnumeration) OrElse myEnumeration = 0 Then
                Throw New Exception("No data was received in 'myEnumeration'")
            Else
                If IsNothing(theString) OrElse theString = String.Empty Then
                    Throw New Exception("No data was received in 'theString'")
                Else
                    If IsNothing(someCollection) OrElse someCollection.Count = 0 Then
                        Throw New Exception("No data was received in 'someCollection'")
                    End If
                End If
            End If
        End If
    End If

    Return "All the variables have value"
End Function

You're throwing System.Exception - that's bad, you'll want to throw a meaningful exception. See this answer for more details.

The main problem with this code, is that you're using exceptions to control the execution flow of your program, which is very expensive - I assume you're going to be catching these exceptions somewhere down the call stack.

If a is Nothing, then accessing it will throw an NullReferenceException. Same for b, c, d and e (say, you also have f in there?).

Let it blow up with whatever exception you get - and catch it. Use exceptions for exceptional cases, and avoid throwing them altogether if you can.

"Exceptions are expensive"

"Throwing and catching exceptions is expensive", you'll read [almost] everywhere. There's truth in that, Jon Skeet has a great article here that correlates performance to the depth of the call stack. The reason for this, is because the thread stops at the throw instruction, and then the exception's StackTrace gets populated - the call stack gets unwinded and the thread walks it until it reaches a catch block. The deeper the stack, the longer it takes.

But the problem isn't about performance - it's about efficiency. Exceptions are not a way to control your execution flow. Regardless of whether or not you care about wasting cycles and populating a stack trace, regardless of how long that takes for the runtime to complete, the point and the matter is, there are ways to make the intent much clearer to whoever is maintaining this code, than by throwing an exception.

The goal is to validate a bunch of objects, and return a message that informs which object is in an invalid state.

By throwing an exception, the first invalid object exits the function and you can't know if other ones were invalid.

By nesting the conditions and returning a String, the last invalid object gets its status reported... if you're lucky - I mean, when anotherObject isn't null, nothing else gets checked and you can't report that theString was NullOrEmpty.

How about returning a flag enum that contains all validation errors? (not sure of VB syntax here, my background is C#):

[Flags]
Public Enum MyValidationError
    None = 0
    FirstObjectIsInvalid = 1
    MyEnumerationIsInvalid = 2
    TheStringIsInvalid = 4
    SomeCollectionIsInvalid = 8
    AnotherObjectIsInvalid = 16
End Enum

Then the function can return a MyValidationError value:

Public Function MyProjectFunction(...) As MyValidationError

    Dim result As MyValidationError
    If IsNothing(firstObject) Then result += MyValidationError.FirstObjectIsInvalid
    If IsNothing(anotherObject) Then result += MyValidationError.AnotherObjectIsInvalid
    If MyEnumeration = 0 Then result += MyValidationError.MyEnumerationIsInvalid 'an enum type shouldn't ever be 'Nothing'...
    If String.IsNullOrEmpty(theString) Then result += MyValidationError.TheStringIsInvalid
    If IsNothing(SomeCollection) OrElse SomeCollection.Count = 0 Then result += MyValidationError.SomeCollectionIsInvalid

    Return result

End Function

Notice all parameters are validated every time now, and the result will contain everything that went wrong. Then another method can use bitwise AND operations on the result and determine the message(s) to log/display, all without throwing an exception, and without nesting anything.


Fraction or integer parser Question

How can the following implementation of fractionParse be made
less ugly? (Preserving unreduced numerators and denominators is supposed to be a feature---as shown in the second example---so using readMaybe s :: Maybe (Ratio Int) doesn't really help.)

Things that irked me in the process of writing it, include:

Not being able to find a way to use do-notation in conjunction with Maybe in a way that would return as soon as Just appears rather than when the first Nothing appears. In other words a Maybe monad instance that emulates or rather than and.
case and pattern matching colliding in unhelpful ways, forcing me to write nested if-then-elses.

Usage examples

λ> fractionParse "2/3"
Fraction 2 3
λ> fractionParse "12 / 6"
Fraction 12 6
λ> fractionParse "23"
Whole 23
λ> fractionParse "a"
Rubbish
λ> fractionParse "2/3/4"
Rubbish
λ> fractionParse "2%3"
Rubbish

Implementation

import Text.Read (readMaybe)
import Data.String.Utils (split)
import Data.Maybe (isJust, fromJust)

data FractionParse = Fraction Int Int
                   | Whole Int
                   | Rubbish deriving (Show, Eq)

fractionParse :: String -> FractionParse
fractionParse s = let i = maybeInt s
                      r = maybeNumDenom s in
  if isJust r
  then let (Just (n,d)) = r in Fraction n d
  else if isJust i
       then Whole (fromJust i)
       else Rubbish

maybeInt :: String -> Maybe Int
maybeInt s = readMaybe s

maybeNumDenom :: String -> Maybe (Int, Int)
maybeNumDenom s = do
  if containsExactlyOneSlash s then Just () else Nothing
  let [ns,ds] = split "/" s
  n <- readMaybe ns
  d <- readMaybe ds
  return (n,d)

containsExactlyOneSlash s = (length $ filter (=='/') s) == 1

You're looking for the First Monoid instance for Maybe. Used like—

import Data.Foldable (foldMap)
import Data.Monoid (First(..))

fractionParse :: String -> FractionParse
fractionParse s = fromMaybe Rubbish . getFirst
                $ foldMap First [ maybeFraction s
                                , maybeWhole s
                                ]

It doesn't make sense to encode your own failure values in your datatypes, so I'd remove the Rubbish Constructor.

data FractionParse = Fraction Int Int | Whole Int
  deriving (Show, Eq)

fractionParse :: String -> Maybe FractionParse

Use Control.Monad.guard instead of manually sending up your own sentinel values (as in maybeNumDenom).

maybeNumDenom s = do
  guard $ containsExactlyOneSlash s
  -- ...

Take advantage of incremental parsing to implement maybeNumDenom, then you won't have to do so much extraneous filtering, counting, and finger crossing. And by removing the Rubbish constructor, at the point you have a correct parse you know you can return a Fraction, so—

import Data.Maybe (listToMaybe)

maybeFraction :: String -> Maybe FractionParse
maybeFraction s = listToMaybe $ do
  (n, '/':s') <- reads s
  (d, "") <- reads s'
  return (Fraction n d)

The above function operates in the list monad. listToMaybe converts a list to a Maybe value by returning Just the first element of the list, or Nothing in the case of an empty list. reads :: Read a => String -> [(a, String)] produces possible parses from a given string, returning the remainder of the unparsed string as the second element of each tuple. Binding to (d, "") ensures that only parses that consume the whole string will be returned.


Accessing the contents of a project's root directory in Python This morning I was trying to find a good way of using os.path to load the content of a text file into memory, which exists in the root directory of my current project.

This approach strikes me as a bit hackneyed, but after some thought it was exactly what I was about to do, except with os.path.normpath(os.path.join(__file__, '..', '..'))

These relative operations on the path to navigate to a static root directory are brittle.

Now, if my project layout changes, these associations to the (former) root directory change with it. I wish I could assign the path to find a target, or a destination, instead of following a sequence of navigation operations.

I was thinking of making a special __root__.py file. Does this look familiar to anyone, or know of a better implementation?

my_project
|
| __init__.py
| README.md
| license.txt
| __root__.py
| special_resource.txt
| ./module
  |-- load.py

Here is how it is implemented:

"""__root__.py"""
import os
def path():
    return os.path.dirname(__file__)

And here is how it could be used:

"""load.py"""
import __root__
def resource():
    return open(os.path.join(__root__.path(), 'special_resource.txt'))

I never heard of __root__.py and don't think that is a good idea.

Instead create a files.py in a utils module:

MAIN_DIRECTORY = dirname(dirname(__file__))
def get_full_path(*path):
    return join(MAIN_DIRECTORY, *path)

You can then import this function from your main.py:

from utils.files import get_full_path

path_to_map = get_full_path('res', 'map.png')

So my project directory tree looks like this:

project_main_folder/
    utils/
        __init__.py (empty)
        files.py
    main.py

When you import a module the Python interpreter searches if there's a built-in module with that name (that's why your module name should be different or you should use relative imports). If it hasn't found a module with that name it will (among others in sys.path) search in the directory containing your script. You can find further information in the documentation.


C++ application calling C# Dll marshalling strings The code appears to work fine. The values are returned as expected from  the DLL and the values are added correctly to the DLL.

Will the way I am passing the string to C# cause a memory leak?

Also, I have to return a string from C# as type LPSTR to add to a char pointer, how could I change it so I could use the string type for the return value.

Also, I assume its related to the .Net garbage collector not kicking in yet but here is the values in TaskManager at different points:

First Line Of C Code          =  1,424k
Just After Create Instance    =  6,752k
Just Before Calling EndString =  26,264k
After EndString               =  26,276k
After CoUninit                =  26,320k

I would have expected the values to drop back down? Or do we have to wait for the garbage collector?

Here is the sample code I put together in Visual Studio 6.0:

#include "stdafx.h"
#include <string>
using namespace std;

#import "C:\\Documents and Settings\\Administrator\\My Documents\\Visual Studio 2008\\Projects\\COMCallable\\COMCallable\\bin\\Release\\COMCallable.tlb" raw_interfaces_only
using namespace COMCallable;

MyNetInterfacePtr pComObject;

string RetString();

void AddValue(long i);

void AddValue2(long i);

int main(int argc, char* argv[])
{
    HRESULT hr = S_OK;

    hr = CoInitialize(NULL);

    if(pComObject == NULL)
    {
        pComObject.CreateInstance(__uuidof(MyNETClass));
    }

    pComObject->StartString();

    for(long i = 1; i <= 100000; i++)
    {
        printf("Value %ld: %s\r\n", i, RetString().c_str());

        AddValue(i);
    }

    pComObject->EndString();

    pComObject->Release();

    CoUninitialize();

    while(true)
    {
        Sleep(1000);
    }

    return 0;
}

string RetString()
{
    char *c = NULL;

    if(pComObject != NULL)
    {
        pComObject->TestMethodString(&c);
    }

    if(c != NULL)
    {
        return c;
    }
    else
    {
        return "";
    }
}

void AddValue(long i)
{
    char numstr[21] = {0}; // enough to hold all numbers up to 64-bits
    sprintf(numstr, "%ld", i);

    string s = string("Some String Here ") + string(numstr);

    if(pComObject != NULL)
    {
        pComObject->AddString((char *)s.c_str());
    }
}

And here is the .NET code:

Interface:

using System;
using System.Collections.Generic;
using System.Text;
using System.Runtime.InteropServices;

namespace COMCallable
{
    [ComVisible(true)]
    public interface MyNetInterface
    {

        [return: MarshalAs(UnmanagedType.LPStr)]
        string TestMethodString();

        void StartString();

        void AddString([MarshalAs(UnmanagedType.LPStr)]string s);

        void EndString();
    }
}

Class

using System;
using System.Collections.Generic;
using System.Text;
using System.Runtime.InteropServices;
using System.Threading;
using System.Diagnostics;

namespace COMCallable
{
    [ComVisible(true)]
    public class MyNETClass : MyNetInterface
    {
        private StringBuilder sb = new StringBuilder();

        public string TestMethodString()
        {
            return "Some String ";
        }

        public void StartString()
        {
            sb = null;
            sb = new StringBuilder();
        }

        public void AddString(string s)
        {
            if (sb == null)
            {
                sb = new StringBuilder();
            }

            sb.Append(s);
        }

        public void EndString()
        {
            sb = null;
        }
    }
}

I would have expected the values to drop back down? Or do we have to wait for the garbage collector?

That's not how the GC works. AFAIK, it only returns memory to the OS if the OS says it'as low on memory. The assumption is that if the OS doesn't need the memory, then it's better to keep the memory as a part of the managed heap, so that the GC doesn't need to run that often.

using namespace std;

This is generally considered a bad practice in C++, since the std namespace contains many simple names that you might want to use yourself.

#import "C:\\Documents and Settings\\Administrator\\My Documents\\Visual Studio 2008\\Projects\\COMCallable\\COMCallable\\bin\\Release\\COMCallable.tlb" raw_interfaces_only

I think that both projects should be part of the same solution and in under the same parent directory. That way, this path can be relative, which will make it much short and it will also work if you copy the whole solution to another computer.

MyNetInterfacePtr pComObject;

I think this shouldn't be a global variable. If you need to pass the object from one function to another, use parameters.

HRESULT hr = S_OK;

hr = CoInitialize(NULL);

There is no reason to initialize hr if you're going to overwrite the value right away anyway.

And if you know that a function can fail, you should check whether it failed or not, not just ignore the HRESULT.

if(pComObject == NULL)

pComObject has to be NULL at this point, so there is no reason for this check.

while(true)
{
    Sleep(1000);
}

If you want to wait forever, the documentation says you should be able to use Sleep(INIFINITE);.

char *c = NULL;

if(pComObject != NULL)
{
    pComObject->TestMethodString(&c);
}

I don't know much about memory management in COM, but this certainly looks like a memory leak. pComObject can't know when are you done using it, so it can't deallocate it itself and you also don't deallocate it.

sprintf(numstr, "%ld", i);

The documentation says:

  Using sprintf, there is no way to limit the number of characters written, which means that code using sprintf is susceptible to buffer overruns.

By my count, 21 characters should be enough here, but I would probably use the safer version anyway, just to be sure.

if(pComObject != NULL)
{
    pComObject->AddString((char *)s.c_str());
}

So, if pComObject is NULL, this function doesn't do anything? I think that's a bad design, when something bad happens, the user should learn about it ASAP, not weeks later when they find out that their database is full of bogus data (or whatever).

sb = null;
sb = new StringBuilder();

That first line is completely unnecessary. If you're going to set the field to a new value anyway, setting it to null first won't change anything.

I don't understand the design of that class. What is the purpose of StartString and EndString? Practically, both do exactly the same thing, because of the null check in AddString().

You could provide just a single Reset() method that sets sb to a new StringBuilder and that would be enough. You could also remove the null check from AddString(), because sb would never be null.

Or even better, make MyNETClass single use. Instead of resetting, you would just create a new instance.


JavaScript metaballs optimization I created this basic metaball visualization in JavaScript. While it does look nice, it is very slow if you increase number of balls and/or make drawing area larger. I think that the performance is so bad because it iterates through every pixel every time the frame is updated but I cannot think anyway how I could update only those pixels where something has actually happened.

Here's the code:

// canvas

var c = document.getElementById("canvas");
width = c.width = /*window.innerWidth;*/1280;
height = c.height = /*window.innerHeight;*/720;
var ctx = c.getContext("2d");

// imagedata

var imageData = ctx.createImageData(width,height);
var data = imageData.data;

// array for balls

var balls = [];
var num = 20;

// random number generation

function random(min, max){
 var num = Math.floor(Math.random()*(max-min+1)+min);
 return num;
}

// metaball

function ball() {
 this.posX = random(0, width);
 this.posY = random(0, height);
 this.dx = random(-4,4);
 this.dy = random(-4,4);
 this.radius = random(10, 100);
}

// ball movement

ball.prototype.move = function() {

 if (this.posX > width || this.posX < 0) { // change direction if edge is encountered, x-axis
  this.dx = -(this.dx);
 }

 if (this.posY > height || this.posY < 0 ) { // change direction if edge is encountered, y-axis
  this.dy = -(this.dy);
 }

 this.posX += this.dx; // move balls, x-axis
 this.posY += this.dy; // move balls, y-axis

}

// create balls and put them into an array

for (i=0; i<num;i++) {
 var metaball = new ball();
 balls.push(metaball);
}

// draw and move metaballs

function Loop(){

 for (var y=0; y<height; y++) {  // loop through draw area pixels, height
  for (var x=0; x<width; x++) { // loop through draw area pixels, width
   var index = (x+y*width)*4; // pixel index
   var color = 0;    // pixel color

   for (i=0; i<balls.length; i++) {
    color += balls[i].radius * 10000 / (Math.pow((x - balls[i].posX),2) + Math.pow((y - balls[i].posY),2)); // metaball calculation formula
   }

    // assign color to pixel based on previous formula
    data[index+0] = color*1.05; // r
    data[index+1] = color;  // g
    data[index+2] = color*1.4; // b
    data[index+3] = 255;  // a
   }
 }

 // move balls

 for (i=0; i<balls.length; i++) {
  balls[i].move();
 }

 // put imagedata on canvas

 ctx.putImageData(imageData, 0,0);

 // animate

 requestAnimationFrame(Loop);
}

// call loop function

Loop();
<!DOCTYPE html>
<html>
<head>
 <title>metaballs</title>

<style type="text/css">

 body {
  padding: 0px;
  margin: 0px;
 }

</style>

</head>
<body>

<div>
 <canvas id="canvas">
 </canvas>
</div>

<script type="text/javascript" src="metaballs.js">
</script>

</body>
</html>

Is there anything I can do to optimize this?

Since I'm a total beginner in JavaScript and programming in general, I would be very happy to hear if there are better practices than what I've used here :)

JavaScript metaballs optimization

I read that as "JavaScript meatballs optimization" :D

First optimization I would do is... code organization. Slow-looking code will be slow code, since it takes you an extraordinary amount of time working with it. So keep your code tidy. Use proper indentation, name variables properly, organize code in a coherent fashion.

Nested loops become a problem, fast. Avoid nested loops as much as possible. In the case of looping through canvas pixels, instead of using a nested loop to traverse horizontally and vertically, you can simply use one loop and skip by fours. That will give you the same pixel-traversing effect.

But isn't it the same amount of iterations? Yes, but here's the thing. Loops are a dead end in terms of optimization. If you get your x and y that way, that's it. However, if you do the 1d loop and do math to get your x and y, you still have a chance at optimization since math can be optimized (bitwise operations) and values can be cached easily.

Another perf killer is property access. It's negligible alone, but if you're operating at 3M pixels per frame, it becomes very visible. Cache values as much as possible, and avoid property accesses.

One optimization you could do is heuristics. I you know a pixel is far enough from any ball, you can just color it black without calculation. Same goes for pixels within a ball. If it's within a ball, it will be white regardless of overlapping balls. Also, if a pixel is already white, you can just skip calculation of the other balls since there's no way it could darken.

Another trick you could do is just use a smaller canvas and upscale it with CSS scale transform. To the script, the number of pixels on the canvas is lesser which means lesser iterations. But from the viewing perspective, the animation is about the same as the full-scale version.

Here's my take on it. Slightly faster due to skipping and upscaling.

const c = document.getElementById("canvas");
const width = c.width = 640;
const height = c.height = 360;
const ctx = c.getContext("2d");
const imageData = ctx.createImageData(width, height);
const data = imageData.data;
const dataLength = data.length;
const num = 20;
const balls = Array(num).fill().map(_ => new Ball());

function random(min, max) {
  var num = Math.floor(Math.random() * (max - min + 1) + min);
  return num;
}

function Ball() {
  this.posX = random(0, width);
  this.posY = random(0, height);
  this.dx = random(-4, 4);
  this.dy = random(-4, 4);
  this.radius = random(2, 25);
}

Ball.prototype.move = function() {
  if (this.posX > width || this.posX < 0) this.dx = -(this.dx);
  if (this.posY > height || this.posY < 0) this.dy = -(this.dy);
  this.posX += this.dx;
  this.posY += this.dy;
}

function updateBalls() {
  for (var i = balls.length; i--;) balls[i].move();
}

function renderBalls() {

  for (var sp = 0; sp < dataLength; sp += 4) {
    const index = sp / 4;
    const y = index / width | 0;
    const x = index % width | 0;
    var color = 0;

    for (var i = balls.length; i--;) {
      const ball = balls[i];
      const radius = ball.radius;
      const posX = ball.posX;
      const posY = ball.posY;

      color += radius * 2500 / (Math.pow((x - posX), 2) + Math.pow((y - posY), 2));

      if(color >= 255) break;
    }

    data[sp + 0] = color * 1.05;
    data[sp + 1] = color;
    data[sp + 2] = color * 1.4;
    data[sp + 3] = 255;
  }

  ctx.putImageData(imageData, 0, 0);
}

;(function loop() {
  requestAnimationFrame(loop);
  updateBalls();
  renderBalls();
})();
body {
  padding: 0px;
  margin: 0px;
}

canvas{
  transform: scale(4);
}
<canvas id="canvas"></canvas>


JavaFX design writes to static field from constructor In JavaFX, the lifecycle of an Application is quite different than Swing. Your constructor must take no arguments so that the Application class can create an instance of your object. Once the instance has been created, it calls start(Stage), which gives you a Stage for you to put your user interface on. This may be fine for some people, but I want to access instance variables of my Application class (which happens to be called LJGM), and the only way I thought I could do this was to have a single, static instance of my LJGM object, and have a static instance() method which returns the instance. Whenever the constructor is called, instance is set to this. There has to be a better way to do this.

public class LJGM extends Application {

    /** The instance of {@link LJGM}. */
    private static LJGM instance;

    /**
     * Gets the only instance of LJGM.
     *
     * @return The instance of LJGM.
     */
    public static LJGM instance() {
        return instance;
    }

    // (other variables here)

    /**
     * Instantiates a new LJGM object.
     */
    public LJGM() {
        LJGM.instance = this;
        // (initialize other variables)
    }

    /*
     * (non-Javadoc)
     *
     * @see javafx.application.Application#start(javafx.stage.Stage)
     */
    @Override
    public void start(Stage primaryStage) throws Exception {
        // (add GUI to the primaryStage here)
    }

    /**
     * Main method.
     *
     * @param args
     *            The arguments to pass to the application.
     */
    public static void main(String[] args) {
        launch(args);
    }
}

Here is the Application Javadoc  in case you need it.

One alternative is to use the Borg Pattern. Allow creating multiple instances, all of which, simply share the same state. Most online references will be in Python (where it is real easy to code), but the idea can be transplanted to Java too.

The advantage over the Singleton pattern is that it hides the fact that there is shared state from clients of the class, especially those that are interested in creating instance.

Clients (in casu the JavaFX framework) can simply use the constructor. Your own code can also easily get the Application by creating a separate instance.

so some sample code :

public class LJGM extends Application {

    private static SharedState sharedState = new SharedState();

    private static class SharedState {

        private Object variable;

        private Object getVariable() {
            return variable;
        }
    }

    public LJGM() {
    }

    public Object getVariable() {
        return sharedState.getVariable();
    }

    @Override
    public void start(Stage primaryStage) throws Exception {
        // (add GUI to the primaryStage here)
    }

    public static void main(String[] args) {
        launch(args);
    }
}

All that being said, I don't see any reason why you could not simply use dependency injection to avoid having global state here. So rather than having low level components query the Application object, let the Application pass them the parameters they will need, by passing itself (preferably implementing an interface) or by passing in the variables themselves.

public class LJGM extends Application implements ApplicationContext {

    private Object variable;

    public LJGM() {
    }

    public Object getVariable() {
        return variable;
    }

    @Override
    public void start(Stage primaryStage) throws Exception {
        // (add GUI to the primaryStage here)
        new LowLevelComponent(this);
        new OtherLowLevelComponent(variable);
    }

    public static void main(String[] args) {
        launch(args);
    }
}


Iterate through properties of an object I need to round numeric values for properties of a given object.
The following code works fine, but I am concerned if there is a way to make it faster.

I am targeting latest Chrome and Firefox.

http://jsbin.com/bahedegoni/edit?html,output

window.app = {
            start: function () {
                var cmd = {
                    a: 11.12,
                    b: 0.29,
                    c: 'x',
                    d: 'y'
                };
                Object.keys(cmd).forEach(function (prop) {
                    if (typeof cmd[prop] === 'number') {
                        cmd[prop] = Math.round(cmd[prop]);
                    }

                }.bind(this));
                console.log(cmd);
            }
        };

There's no reason to use bind; you don't rely on this anywhere in the code, so it doesn't matter what the this-context is.

Secondly, the "original" way to iterate objects in JavaScript is to use a for...in loop. However, it will also iterate inherited properties, so it's good practice to check if the key's the object's own key by using hasOwnProperty. The basic construction looks like so:

var key;
for(key in object) {
  if(!obj.hasOwnProperty(key)) continue; // skip inherited properties
  // ... do something
}

Thirdly, due to the way JavaScript expresses a NaN (Not A Number) value, it turns out that typeof NaN === 'number'. Of course, it doesn't really matter here since Math.round(NaN) is still NaN.

Point is, this would probably be one of the simplest implementations:

var key;
for(key in cmd) {
  if(obj.hasOwnProperty(key) && typeof cmd[key] === 'number') {
    cmd[key] = Math.round(cmd[key]);
  }
}


Moving files while preserving the folder structure I'm writing a console application that will look through a directory and move any log files that have a date modified older than X days (configurable in the app.config file).

The problem I'm having is with the Move method I wrote. It works, but I think there might be a better way to write this method and eliminate the need for the sourceRoothPath parameter. Here's what I have (it's part of a LogManager class I wrote to do the checking and moving of log files):

public void Move(string sourceRootPath, string sourcePath, string targetPath)
{
    if (String.IsNullOrWhiteSpace(sourcePath))
        throw new ArgumentNullException("sourcePath");

    if (String.IsNullOrWhiteSpace(targetPath))
        throw new ArgumentNullException("destinationPath");

    if (!File.Exists(sourcePath))
        throw new FileNotFoundException(sourcePath);

    try
    {
        // trimmedPath becomes the file path with all the subfolders, but without the
        // sourceRootPath that comes in front of it. i.e. it strips the value passed
        // in sourceRootPath from the value passed in sourcePath. The "+ 1" is to include the
        // trailing "\" in the path.
        string trimmedPath = sourcePath.Substring(sourceRootPath.Length + 1);
        string newPath = Path.Combine(targetPath, trimmedPath);
        string fileName = Path.GetFileName(sourcePath);

        // folderStructure is used for creating the subfolder structure I want to preserve.
        // (It is just removing the file name and extension from the newPath.)
        string folderStructure = newPath.Substring(0, (newPath.Length - fileName.Length));

        // Directory.CreateDirectory will create the entire folder structure for me; no need
        // for looping or recursive calls.
        Directory.CreateDirectory(folderStructure);

        // File.Move has no ability to overwrite, so I have to delete the file if it exists in the
        // destination directory so that File.Move doesn't throw an exception.
        if (File.Exists(newPath))
            File.Delete(newPath);

        File.Move(sourcePath, newPath);
        loggingService.LogInfo("Moved file from " + sourcePath + " to " + newPath + ".");
    }
    catch (Exception ex)
    {
        loggingService.LogError(ex.Message);
        throw ex;
    }
}

I included the sourceRootPath parameter because I need this method to duplicate the folder structure of the sourcePath, but I don't want it to include some of the top level folders.

So, for example, if I'm having this application scan a log file directory at C:\someFolder\test\logs, and there are some subfolders like production\someDate or HR\HRSystem\someDate etc. I want to keep the subfolder structure in the production and HR folders, but move them to a new location like C:\test_log_archive. So C:\someFolder\test\logs\production\someDate should become C:\test_log_archive\production\someDate, and the value I would pass for sourceRootPath would be C:\someFolder\test\logs.

Does it make sense to remove the sourceRootPath and how would I do it? I feel like I should be able to accomplish this with just sourcePath and targetPath parameters, but I can't figure out a way to do it other than the code I provided above.

No, you do definitely need 3 parameters to do what you are trying to do

Although I would suggest a different 3.

public void Move(string sourceRoot, string targetRoot, string[] relative) {
    /* Precondition checks and try-catch removed for clarity */

    string source = Path.Combine(sourceRoot, relative);
    string target = Path.Combine(targetRoot, relative);
    string folderStructure = Path.GetDirectoryName(target);

    /* etc */
}

This makes clear that there is shared structure under source and target, and there isn't any direct string manipulation on file and folder names. I would even be tempted to factor this out into a seperate class, e.g.

class DirectoryMover {
    private string source;
    private string target;

    private void Move(string[] relative) { ... }
    public void MoveAll() { /* iterate over folders calling Move */ }
}


C# BufferBlock that keeps unique items In our application we watch file changes using .NET FileSystemWatcher. After each file change we process the new content asynchronously. In order to make everything flexible and smooth, we used TPL BufferBlock<T>

After deployment and some real tests we realized, that watcher might be triggered too much times on the same file. And while we process only one file simultaneously, we anyway re-work the same content.

So, basically, we want the following:

Have easy-to-use collection that works similarly to queue and provides a possibility to wait for new items without any while (true) loops.
Thread-safety.
Items inside should be unique.

Since we are not able to extend BufferBlock (it is sealed) and we don't have access to the full items collection inside BufferBlock, we ended with the following solution:

using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;
using System.Threading.Tasks.Dataflow;

public class UniqueBufferBlock
{
    private readonly BufferBlock<string> _buffer = new BufferBlock<string>();
    private readonly HashSet<string> _hashSet = new HashSet<string>();
    private readonly object _locker = new object();

    public void Post(string value)
    {
        lock (_locker)
        {
            if (_hashSet.Add(value))
            {
                _buffer.Post(value);
            }
        }
    }

    public string Recieve()
    {
        lock (_locker)
        {
            var value = _buffer.Receive();
            _hashSet.Remove(value);
            return value;
        }
    }

    public Task<bool> OutputAvailable(CancellationToken cancelToken)
    {
        return _buffer.OutputAvailableAsync(cancelToken);
    }
}

Typical usage:

fileWatcher.FileChanged += (_, fileName) => uniqueBufferBlock.Post(fileName);

// ...

while (await uniqueBufferBlock.OutputAvailable(cancelToken))
{
    var fileName = uniqueBufferBlock.Receive();
    await ProcessFile(fileName, cancelToken);
}

What drawbacks could we face with it and what can be improved?

If you're aiming for KISS at the level of maintenance, this looks like a reasonable solution, although there are some minor details which IMO could be improved.

public class UniqueBufferBlock
{
    private readonly BufferBlock<string> _buffer = new BufferBlock<string>();
    private readonly HashSet<string> _hashSet = new HashSet<string>();

Why not genericise the class?

    public string Recieve()

Fix that spelling. Incorrectly spelt names are one of the banes of the maintenance programmer.

    public Task<bool> OutputAvailable(CancellationToken cancelToken)

Follow conventions: this should have a name ending Async, like the method it wraps.

There are a couple of other directions which might be worth considering. One is to go the full hog and implement IPropagatorBlock. The other is to slim down the dependencies by removing _buffer and implementing OutputAvailableAsync with TaskCompletionSource. Both would make the implementation of UniqueBufferBlock more fiddly, and it's reasonable to decide not to go in either direction.

PS on a point of English language pedantry: distinct is a better choice of word than unique. Perhaps deduplicated would be better still in this context.


Initial stages of MVP, is the bootstrap file efficient coding I am attempting to put my new OOP skills to the test and am eventually going to build a simple OOP login/registration system.

However as I am making use of OOP I have decided to make my own simple MVC.

Here is my layout of my directory

root-directory/
   .htaccess
   app/
      Config/
      Controllers/
      Models/
      Views/
      Lib/
        Vendor/
        composer.json
      bootstrap/
         app.php
   public/
      assets/
        js/
        css/
        img/
      index.php
      .htaccess

I have my app file in the Bootstrap folder I have written my code in a manor so it checks if the _GET super variable has anything in it, if it doesn't then set the $this->controller = 'home' and set the $this->method = 'index'

Here is my Bootstrap/app.php file

<?php

namespace App\Bootstrap;

require __DIR__ . '/../Lib/vendor/autoload.php';

class App
{

    private $controller;
    private $method;
    private $requests;

    public function __construct($requests)
    {
        $this->requests = $requests;

        $this->requests = $this->requests['PATH_INFO'];

        $this->requests = explode('/',$this->requests);

        if ($this->requests[0] == '')) {

            $this->controller = 'home';

        } else {

            $this->controller = $this->requests[0];

        }

        if ($this->requests[1] == '' ) {

            $this->method = 'index';

        } else {

            $this->method = $this->requests[1];

        }

        echo $this->controller;
        echo '
';
        echo '
';
        echo $this->method;

        die();

    }
}

Here is my index.php file

<?php
error_reporting(E_ALL);
ini_set('display_errors',1);

require __DIR__ . '/../app/Bootstrap/app.php';

$b = new \App\Bootstrap\App($_GET);

Here is my .htaccess file

<IfModule mod_rewrite.c>
Options +FollowSymLinks
RewriteEngine on
RewriteCond %{REQUEST_FILENAME} !-f
RewriteCond %{REQUEST_FILENAME} !-d
RewriteRule ^(.*)$ index.php?PATH_INFO=$1 [L,QSA]

</IfModule>

Is this a correct way of starting this project? As my next step would be to create a base controller and then link the url to the controller.

If this is not a correct/appropriate coding manor for OOP would a good correction include changing the RewriteRule to include to variables for the controller and method.

You can add another folder named libraries inside of app.
Follow a particular case convention. Like don't have config (all small) and other folders with first letter capitalized. Either all small or all first letter capitalized.
Add a language folder within app. That will store your language strings within each language folder. For example, you can have a language/english folder & similarly for other languages.

I like that you have separated the public folder.


Rspec unit test for updating a property of a model I know how to write simple RSpec tests and unit tests. I just want to know if this is good enough or if there are any improvements I can make.

I intend to test the the toggle_property method of the TaskItemUserReference model which updates the property, completed from false to true or vice versa.

Relationships

User has many Tasks through TaskUserReference
Task has many Users through TaskUserReference

TaskItem has many Users through TaskItemUserReference
User has many TaskItem through TaskItemUserReference

#Factories

FactoryGirl.define do
  factory :task do
    name Faker::Lorem.sentence(2)
  end
end

FactoryGirl.define do
  factory :task_user_reference do
    user
    task
  end
end

FactoryGirl.define do
  factory :task_item do
    name Faker::Lorem.sentence(3)
    task
  end
end

FactoryGirl.define do
  factory :task_item_user_reference do
    task_item
    user
    active true
    completed false
  end
end

# RSPEC test
require "rails_helper"

RSpec.describe Task, type: :model do

  let(:owner) { create(:user) }
  let(:task) { create(:task, owner: owner) }
  let(:user) { create(:user) }
  let(:task_item) { create(:task_item, task: task) }

  before do
    user.tasks << task
    user.task_item_user_references.create(
      user_id: user.id,
      completed: true,
      task_item: task_item
    )
  end

  describe "task_items" do
    it "updates status" do
      task_item_user_reference = user.task_item_user_references.first
      task_item_user_reference.toggle_property(:completed)

      expect(task_item_user_reference.completed).to eq(false)
     end
  end
end

It looks like you don't need the whole user-task-task item setup for the test you provided. It's actually a TaskItemUserReference spec, not Task spec.

RSpec.describe TaskItemUserReference, type: :model do

  describe "#toggle_property" do
    it "updates property" do
      task_item_user_reference = TaskItemUserReference.new(completed: true)

      expect do
        task_item_user_reference.toggle_property(:completed)
      end.to change {task_item_user_reference.completed}.from(true).to(false)
    end
  end
end


Breaking up a navigation menu This code works as I intended, but I am definitely a JS noob (C++ background), so I'm not sure if this is the best way to do it (for example using .each instead of some kind of a for loop was completely new to me).

https://github.com/TaylorHuston/menuBreak/blob/master/menuBreak.js

function menuBreak() {

var windowWidth = $(window).width();

var menuLength = 0;

$("#navUl > li").each( function (index, element) {
    menuLength += $(this).width();
});

if( menuLength > windowWidth*.9 ) {
    var newWidth= menuLength*.7;
    $('#navUl').css({"maxWidth":newWidth});
} else {
    $('#navUl').css({"maxWidth":windowWidth});
}

}

And in my HTML, not sure if this is the 'best' way to call it

<script>
  $(document).ready(function() {
    menuBreak();
  });

  $(window).resize(function() {
    menuBreak();
  });
</script>

From a once over:

You could have used a loop instead of .each(), it would be marginally faster
Your indenting is off, I assume you had trouble pasting the code
.9 and .7 are magic constants, you should declare these with var and a good name
I would use a ternary for this:

if {
    var newWidth= ;
    $('#navUl').css({"maxWidth":newWidth});
} else {
    $('#navUl').css({"maxWidth":});
}

could be

var navWidth = ( menuLength > windowWidth *.9 ) ? menuLength *.7 : windowWidth;
$('#navUl').css({"maxWidth":navWidth});

of course with properly named constants this might stretch too much to be easily readable, up to you.
With jQuery, the most common way to trigger on ready is

$( function(){
  //Do Something
  menuBreak();
});

now, since $() expects a function, we might as well pass menuBreak immediately:

$( menuBreak );

The same things goes for resize:

$(window).resize( menuBreak );


Hide content unless an array contains at least one non-empty string I have an array of hints. Sometimes, this array could contain an empty string. Occasionally, the array might contain only empty strings, in which case I want the whole block to be skipped. This is what I came up with:

<% @hints.each do |hint| %>
    <% unless hint.empty? %>

Please observe the following hints:

        <% @hints.each do |hint| %>
            <% unless hint.empty? %>

<%= hint %>

            <% end %>
        <% end %>

        <% break %>
    <% end %>
<% end %>

An example of @hints:

["", "", "Have you tried turning it off and on again?"]

Or:

[""]

Anyone know a better way?

As @tokland pointed out, your current code has one too many loops really.

As for checking if all the strings are empty, you have some options.

1) The any? method accepts a block, so you can test if any of the elements in an array match a certain condition. In your case, you could do

@hints.any? { |string| string.present? }

since present? checks if a string is non-empty. But you can also shorten that to just

@hints.any?(&:present)

With that you could do something like

<% if @hints.any?(&:present) %>

Please observe the following hints:

    <% @hints.each do |hint| %>
        <% unless hint.empty? %>

<%= hint %>

        <% end %>
    <% end %>

<% end %>

2) Alternatively, and I'd say nicer, you can filter the array beforehand, to simply not include blank strings using select or, conversely, reject.

@hints = some_hints.reject(&:blank?)   # reject blank strings
# or...
@hints = some_hints.select(&:present?) # keep non-blank strings

This should happen in the controller when you first set @ hints (or even earlier - don't know where the hints come from, but there's little need to keep empty strings around if you don't have to, so either strip them out early, or just avoid adding them to begin with).

With a "clean" @hints array you can do this:

<% unless @hints.empty? %>

Please observe the following hints:

    <% @hints.each do |hint| %>

<%= hint %>

    <% end %>

<% end %>


Loop round and remove the consonants The main objective of this code is that you must accept an input from the user and loop and remove the consonants and vice versa and display the new word.

I know there is a easier way of doing it, but I have no idea how.

String test = "john doe", check;
    for(int ktr = 0; ktr < test.length(); ktr++){
       // System.out.println(test.substring(ktr, ktr+1));
        check = test.substring(ktr, ktr+1);
        if(!"b".equals(check) && !"c".equals(check)){
            if(!"d".equals(check) && !"f".equals(check)){
                if(!"g".equals(check) && !"h".equals(check)){
                    if(!"j".equals(check) && !"k".equals(check)){
                        if(!"l".equals(check) && !"m".equals(check)){
                            if(!"n".equals(check) && !"p".equals(check)){
                                if(!"q".equals(check) && !"r".equals(check)){
                                    if(!"s".equals(check) && !"t".equals(check)){
                                        if(!"v".equals(check) && !"w".equals(check)){
                                            if(!"x".equals(check) && !"y".equals(check)){
                                                if(!"z".equals(check)){
                                                     System.out.print(check);
                                                }
                                            }
                                        }
                                    }
                                }
                            }
                        }
                    }
                }
            }
        }
    }

So it looks like you're just printing out the vowels in your string, right?

System.out.println(answer.replaceAll("[^AaeEiIoOuU]", ""));

Removes the for loop and compacts your code a lot, making it a lot more readable IMO.  Also notice how easy it was for me to extend to capital letters.

It would be good to note that this is a replacement for your block of code as it is right now.  If you plan on printing the vowels in a variable amount of strings (perhaps input by the user), then you would want to make a method that would do this for you.  In this function you would pass in the precompiled regex and the string to print.

I wrote this up really quick in a text editor and didn't compile it, but here is a program that could handle multiple strings better:

class VowelPrinter
{
    static final Pattern p = Pattern.compile("[^AaeEiIoOuU]");

    public static String removePattern(Pattern p, String str)
    {
        return p.matcher(str).replaceAll("");
    }
    public static void main (String[] args)
    {
        int num = 0;

        System.out.print("How many names are you going to save: ");
        Scanner in = new Scanner(System.in);
        num = Integer.parseInt(in.nextLine().trim());

        String names[] = new String[num];
        for (int i = 0; i < names.length; i++)
        {
            System.out.print("Type a name: ");
            names[i] = removePattern(p, in.nextLine());
        }
        System.out.println("Names with consonants removed:");
        for (int i = 0; i < names.length; i++)
        {
            System.out.println(names[i]);
        }
    }
}


Switch questions using JavaScript and CSS I am writing a Q&A wap page and want to switch questions in one page. This is my first time to write JS and CSS code, so I think there must be some improvement.

$(function(){
    $("#parent").on('click','.children', function(e){
        var that = e.currentTarget;
        var wid = parseInt($(that).parent().css('width'));
        var margin = parseInt($(that).parent().css('marginLeft'));
        margin -= wid;
        $(that).parent().css('marginLeft', margin + "px");
    });
});
#wrap {
  width: 100%;
  height: 30px;
  overflow: hidden;
}
#parent {
  width: 100%;
  -webkit-transition: all .2s linear;
  white-space: nowrap;
}
#parent div {
  background-color: gray;
  width: 100%;
  height: 30px;
  display: inline-block;
}
<!DOCTYPE html>

    <meta charset="UTF-8">
    <title></title>
    <meta name="viewport" content="width=device-width, initial-scale=1.0, maximum-scale=1.0, minimum-scale=1.0, user-scalable=no" />
    <meta name="apple-touch-fullscreen" content="yes" />
    <meta name="apple-mobile-web-app-capable" content="yes" />
    <meta name="apple-mobile-web-app-status-bar-style" content="black" />
    <meta name="format-detection" content="telephone=no" />

    <link rel="stylesheet" href="../css/change.css"/>

NO.1

NO.2

NO.3

NO.4

result

Running example

I don't want to my last result div to switch, so there is no event listener on my last result div.

JavaScript

I would make the following changes in the jQuery code:
1) Create a private scope for your code using an IIFE.
2) Cache your selectors.
3) Supply the "radix" parameter to parseInt.

(function( $ ) { //start IIFE to create a private scope and $=jQuery

  function updateElement() {
    var $this = $(e.currentTarget),  //cache selectors
        $parent = $this.parent(),
        wid = 0, margin = 0;
    wid = parseInt( $parent.css('width'), 10); //added radix parameter
    margin = parseInt( $parent.css('marginLeft'), 10);
    $parent.css('marginLeft', (margin - wid) + "px");
  }

  $(function(){
    $("#parent").on( 'click', '.children', updateElement );
  });

})( jQuery );  //pass jQuery to function so $ is always set correctly.

If you are pointing to the #parent element you could change the JS to be more performant by caching the parent element as well.

 (function( $ ) {

  var $parent = $('#parent');

  function updateElement() {
    var wid = 0, margin = 0;
    wid = parseInt( $parent.css('width'), 10); //added radix parameter
    margin = parseInt( $parent.css('marginLeft'), 10);
    $parent.css('marginLeft', (margin - wid)+ "px");
  }

  $(function(){
    $parent.on( 'click', '.children', updateElement );
  });

 })(jQuery);

CSS

In your CSS you should remove the vendor prefixes or supply all of them ( Here is a list. Ok not all just the most common: ms,o,moz,webkit,etc ). Also, you should include the non-prefixed property last in the list so that once it is available non-prefixed your code still works.

#parent {
  -moz-transition: all .2s linear;
  -o-transition: all .2s linear;
  -ms-transition: all .2s linear;
  -webkit-transition: all .2s linear;
  transition: all .2s linear;
}

You could also use a tool like autoprefixer in your build process to add the prefixes automatically.  I would recommend this approach.


how to write generic extensions for standard collection types in java? I am a Java beginner and I am looking for an idiomatic way of writing a function that involves generics. I wrote this helper class (below) that pushes items into a sorted generic collection and I wanted to ask for your feedback. Should I perhaps extends some base class of some collection? Or maybe there is some better approach that follows the Java philosophy?

import java.util.Comparator;
import java.util.List;

public abstract class ListExtensions {
    public static <T> void addOnCompare(List<T> collection, T item,
                                        Comparator<T> comparator) {
        synchronized(collection) {
            int i = 0;
            int size = collection.size();
            if (size == 1) {
                int diff = comparator.compare(item, collection.get(0));
                switch(diff) {
                case 1: i++; break;
                default: break;
                }
            } else {
                int range = size - 1;
                i = size / 2;
                int left = 0;
                int right = range;
                while(true) {
                    if (i <= 0) { i = 0; break; }
                    if (i > range) { i = range; break; }
                    int diff = comparator.compare(item, collection.get(i));
                    if (diff == 0) break;
                    else {
                        if (diff == -1) right = i;
                        if (diff == 1) left = i;
                        int near = i + diff;
                        if (near < 0) { i = 0; break; }
                        if (near > range) { i = range + 1; break; }
                        int diff_near = comparator.compare(item, collection.get(near));
                        if (diff_near == 0) { i = diff_near; break; }
                        if (diff_near == diff) {
                            int step = (right-left)/2;
                            if (step == 0) step = 1;
                            switch(diff){
                            case -1:
                                right = i;
                                i = i - step; break;
                            case 1:
                                left = i;
                                i = i + step; break;
                            }
                        } else if (diff > diff_near) {
                            i = near; break;
                        } else { break; }
                    }
                }
            }
            collection.add(i, item);
        }
    }
}

Parts this question have already been answered on StackOverflow - https://stackoverflow.com/a/13529644/139985  So I'm going to treat this as a simple request for a code review.

The method name is opaque ... addOnCompare does not clearly imply a particular action ... and there is no javadoc for the method.  This immediately makes code-review hard ... because I have to try and figure out what the code is trying to do before I can comment on whether it is doing that well.
The parameter name collection is poor.  It is a list, not a collection.
The local variable diff_near should be diffNear.
This is convoluted:

        switch(diff) {
        case 1: i++; break;
        default: break;
        }

Write it as:

        if (diff == 1) { i = 1; }

I don't know what these lines are trying to do, but they are definitely wrong:

        int near = i + diff;
        if (near < 0) { i = 0; break; }
        if (near > range) { i = range + 1; break; }
        int diff_near = comparator.compare(item, collection.get(near));

The value returned by a comparator has no meaning beyond being -ve, zero, or +ve.  So adding it to the index i is going to have an unpredictable effect. Indeed, since range never changes, this code makes the behaviour of the entire algorithm too difficult to understand.
I'm going to hazard a guess that the method is intended to insert item at the correct position in a previously sorted list.  But given the above, I have grave doubts that it will actually work for all possible (valid) comparators and all possible (sorted) input lists.
Even if this code did work, the performance for a linked list would be poor.  The List.get(int) method for a linked list is O(N), so even an optimal binary search pattern would result in O(Nlog(N)) operations to insert one element into the linked list.  A simple traversal of the list followed by an insert would be O(N).


Set a default if key not in a dictionary, *or* value is None Let's say I have a key 'messages' that is usually a list, but might be None, or might not be present. So these are all valid inputs:

{
   'date': 'tuesday',
   'messages': None,
}

{
   'date': 'tuesday',
   'messages': ['a', 'b'],
}

{
   'date': 'tuesday',
}

If I want to retrieve 'messages' from the input, and iterate over them, I need to do something like this:

messages = d.get('messages', []) # Check for key existence
if messages is None:             # Check if key is there, but None
    messages = []
for message in messages:
    do_something()

That seems a little verbose for Python - is there a simpler way to write that?

for message in d.get('messages') or ():
    do_something()

get returns None by default for missing keys.
a or b evaluates to b when bool(a) == False. An empty list and None are examples of values that are false in a boolean context.
Note that you cannot use this trick if you intend to modify the possibly empty list that is already in the dict. I'm using () instead of [] to guard against such mistakes. Also, CPython caches the empty tuple, so using it is slightly faster than constructing an empty list.


Small implementation of a singly-linked list in C89 I'm very new to C and have extensive experience with C# and very little with C++, but basically none with C.

I wrote a small program that declares a singly-linked list struct, a few methods operating on the list, and then uses it from the main method. Even though I've read about it a lot, this is only my second/third program I've written in C, so please beware.

#include <assert.h>
#include <inttypes.h>
#include <stddef.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

typedef struct linked_list_node linked_list_node; // forward declare linked_list_node and add it to the struct/global namespaces

struct linked_list_node
{
    linked_list_node* next;
    int32_t item;
};

typedef struct
{
    linked_list_node* head;
    size_t size;
} linked_list;

void init_list(linked_list* list) { /* Nothing to do here */ }

void add_last(linked_list* list, int32_t item)
{
    assert(list != NULL);

    if (list->head == NULL)
    {
        // No elements in the list yet.
        list->head = malloc(sizeof(linked_list_node));
        list->head->next = NULL;
        list->head->item = item;
    }
    else
    {
        // At least 1 element in the list.
        assert(list->size > 0);

        // Walk the list until we find the last node.
        linked_list_node* node = list->head;
        while (node->next != NULL)
        {
            node = node->next;
        }

        // Allocate a new last node
        linked_list_node* next = malloc(sizeof(linked_list_node)); // note: no typecast from void* here
        node->next = next;
        next->next = NULL; // need to explicitly do this since malloc's memory is uninitialized
        next->item = item;
    }

    list->size++;
}

typedef void action(int32_t);

void foreach_list(linked_list* list, action* act)
{
    assert(list != NULL && act != NULL);

    linked_list_node* node;
    for (node = list->head;
        node != NULL;
        node = node->next)
    {
        act(node->item);
    }
}

// Cleans up all of the memory used up by the list's
// nodes. DOES NOT FREE THE LIST ITSELF.
void cleanup_list(linked_list* list)
{
    assert(list != NULL);

    list->size = 0;

    // Walk the list, freeing nodes as we go.
    linked_list_node* node, *next;
    for (node = list->head;
        node != NULL;
        node = next)
    {
        next = node->next;
        free(node);
    }
}

void print_int32(int32_t to_print)
{
    printf("Found %"PRIi32"!\n", to_print); // format specifier for int32_t
}

int main(void)
{
    linked_list list;
    memset(&list, 0, sizeof(list));
    init_list(&list);

    add_last(&list, 3);
    add_last(&list, 4);
    add_last(&list, 5);

    foreach_list(&list, print_int32);
    printf("Size: %"PRIu64"\n", (uint64_t)list.size); // do not use %zu since this is targeting C89

    cleanup_list(&list);
    return 0;
}

I compiled this under MinGW with C89.

Here is the type of feedback I'm looking for:

Did I introduce and potential sources of UB? For example, is there a flaw in my logic where a NULL pointer can be dereferenced and go undetected by asserts?
Is my memory management logic correct? Did I accidentally malloc something that never got cleared, or did I free anything twice in cleanup_list? Is there a way to make all of the logic less error-prone?
Did I rely on any features specific to GCC to get this to work? For example, will this program also work with other compilers under C89?
Any way I could clean up the code and make it more neat/concise? (I am aware that I should probably have put the list definition/methods into separate header/source files than main.)

init_list

should really initialize the list. Otherwise you are forcing the caller to make an non-obvious memset.

void init_list(linked_list * list) {
    list->head = 0;
    list->size = 0;
}

is much more clean.
add_last

is not a particularly good name. Traditionally this operation is called append or push_back.

The function can be streamlined. First, assert your invariant fully (that is either head == 0 and size == 0 or head != 0 and size > 0) and immediately.

Second, once the invariant is deemed correct, allocate the node (you need to do it anyway). It is a good idea to delegate node allocation to a function.

Finally, find the tail (it is also a good candidate to factor out into a function), and attach the node. For example,

void append (linked_list * list, int32_t item)
{
    assert(list_is_good(list));
    linked_list_node * node = create_list_node(item);
    linked_list_node * tail = find_list_tail(list);
    if (tail == NULL) {
        list->head = node;
    } else {
        tail->next = node;
    }
    list->size += 1;
}

malloc may fail

Test the results of malloc. There is not much you can do once it fails, but it is good form anyway.


Counting all words from text and ordering it I did it and it works as intended but it seemed very inefficient. I made the string a list, then I standardize it (removing accents, commas and points). After that, I count every word using a dictionary, then sort it in a loop.

Is there a better way to do it?

Edit - Thanks to you both, with your tips I managed to improve the code a lot, it's much faster and efficient now: https://pastebin.com/EN74daBG

import unidecode
from operator import itemgetter

def word_counter(text):
    counter = {}
    for word in text.lower().replace(",", "").replace(".", "").split():
        standardize = unidecode.unidecode(word)
        if standardize.isalnum():
            counter.setdefault(standardize, 0)
            counter[standardize] += 1
    for key, value in sorted(counter.items(), key=itemgetter(1), reverse=True):
        print("{} = {}".format(key, value))

word_counter('''text here''')

All in all this is not bad.

Split into functions

I would split this into more functions.

to generate a stream of words
to do the count
for the presentation

splitting the text

This is a simple generator

def text_split(text):
    text = text.lower().replace(",", "").replace(".", "")
    for word in text.split():
        yield unidecode.unidecode(word)

You can generalize this a bit using re and string.punctuation:

import re
import string

PUNCTUATION = re.compile(rf'[{string.punctuation}]')
def text_split_re(text):
    text = PUNCTUATION.sub('', text.lower())
    for word in text.split():
        yield unidecode.unidecode(word)

This removes all punctuation in 1 go.

Counter

you use dict.setdefault, so you read the documentation. If you had looked a tiny bit further, in the collections module, you'd have found Counter, which is exactly what you need, especially with it's most_common method. This is so handy, you don't really need the second method. You just need to feed it to the Counter.

presentation

def print_result(word_count: Counter):
    for word, count in word_count.most_common():
        print(f'{word} = {count}')

as simple as:

putting it together

from collections import Counter
if __name__ == '__main__':
    words = text_split_re('''text here''')
    word_count = Counter(words)
    print_result(word_count)

tests

splitting this in parts also allows you to unit-test each of the parts

assert list(text_split_re('text here')) == ['text', 'here']
assert list(text_split_re('Text here')) == ['text', 'here']
assert list(text_split_re('Text he.re')) == ['text', 'here']
assert list(text_split_re('''Text
    here''')) == ['text', 'here']


Parsing sentences to create a chat bot I am developing a chat bot with Node.Js. I am trying to find efficient way to parsing sentence.

what do you think about humanity
-> ba bla bla.

what do you think about cats
->I love cats

Right now I am using the intersection technique, but I don't think it's efficient:

var sentence = sentence.split(",")
var inters = intersectionOfArrays(sentence, ["what", "do", "you", "think", "about"])
if(inters.length >= 3) { //we have common words
    keyword = extractionOfarrays(sentence,["what", "do", "you", "think", "about"]) // get question word (humanity, cats etc)
    if(keyword == "cats")
       responseToServer("I love cats")
    if(keyword == "humanity")
       responseToServer("bla bla bla")
}

Is this technique bad? I want to answer thousands of questions so I need to know what is the best method for doing some "AI" magic.

From a personal standpoint, I think this method is fine. You might want to re-work it a little. I'd recommend creating an array with possible intersections, and looping through that array and checking user input against those. For example:

var intersections = [
    ["what", "do", "you", "think", "about"],
    ["what", "is", "your"],
    ...
];

Finally, I'd recommend having something that allows for multiple keywords, rather than one. In addition to this, you may not want to be checking the length of intersections based on a constant, in this case, three.

Anyways, if there's anything else that you want me to cover, just mention it in the comments, and I'll see what I can do.


Better way of comparing integers I wrote this class which is comparing some values, how can I improve its performance and maybe make it prettier?

You can see at the bottom what type of data it requires, also the array size can reach 100 000 elements.

As it is the class is working fine, but I would like to get a second opinion on how to improve it (I know that is not something complicated but I would like to find new ways of doing this).

module Stat
  class CompareData
    def initialize(datas)
      @datas = datas
      @result_data = {'g_yesterday' => {inc: 0, dec: 0}, 'g_week' => {inc: 0, dec: 0}, 'g_month' => {inc: 0, dec: 0}}
end

  def get_data
    @datas.each do |data|
      %w[g_yesterday g_week g_month].each do |compare|
        @today_value = data.g_current
        compare_data data[compare], compare
      end
    end
    @result_data
  end

  private
  def compare_data second_value, date
    unless @today_value == 0 && second_value == 0
      if @today_value != second_value
        period = @result_data[date]
        period[:dec] += 1 if @today_value == 0
        if second_value == 0
          period[:inc] += 1
        else
          @today_value < second_value ? period[:inc] += 1 : period[:dec] += 1
        end
      end
    end
  end
 end
end

values = [{'g_current' => '2', 'g_yesterday' => '0', 'g_week' => '3', 'g_month' => '4'}, ..]

init = Stat::CompareData.new(values)
init.get_data

EDIT

I have pasted the wrong code here is the one that I wanted to post:

  def get_data
    @datas.each do |data|
      %w[g_yesterday g_week g_month].each do |compare|
        @today_value = data['g_current'].to_i
        compare_data data[compare].to_i, compare
      end
    end
    @result_data
  end

  def compare_data second_value, date
    unless @today_value == 0 && second_value == 0
      if @today_value != second_value
        period = @result_data[date]
        if @today_value == 0 || second_value == 0
          @today_value > second_value ? period[:inc] += 1 : period[:dec] += 1
        else
          @today_value > second_value ? period[:dec] += 1 : period[:inc] += 1
        end
      end
    end
  end

I'm not usually harsh, but this code is a buggy mess. Sorry.

We don't review broken code on CodeReview, but I just can't help myself. As I said in a comment, this code is so broken that it actually ends up almost working correctly.

Well, except for the fact that data.g_current raises an exception, because you should be writing data['g_current'].

But let's ignore that and look at #compare_data:

def compare_data second_value, date
  # this is *always* false, since neither value is a number;
  # they're strings and a string can't be zero
  unless @today_value == 0 && second_value == 0

    # And this condition makes the one above completely redundant anyway
    if @today_value != second_value
      period = @result_data[date]

      # This *never* happens because (again) a string can't be zero.
      # But that's a good thing, because if it worked, and
      # @today_value was zero, while second_value was 1 or more
      # you'd end up adding 1 to :dec here, *and* adding 1 to
      # :inc in the else-block below
      period[:dec] += 1 if @today_value == 0

      if second_value == 0
        # this never happens either (same reason again)
        period[:inc] += 1
      else

        # this is the only line that'll ever actually do anything!
        # ... but it's broken.
        # You're comparing strings, not numbers, so in string terms,
        # "2" is greater than "10", "323" is less than "5" and so on,
        # and the branch will do the wrong thing. It just happens to
        # work if both values are single digit strings
        @today_value < second_value ? period[:inc] += 1 : period[:dec] += 1
      end
    end
  end
end

In other words: there are issues here.

Other stuff:

What you want is a method, not a class. What you seem to be going for is a straight folding/reduction of a data set (i.e. Array#inject).
You're setting an instance variable in a loop (@today_value), and relying on that in a method. This is, frankly, terrible. If anything, just pass it to the method as an argument. There's just a lot of really weird coupling and state-keeping going on, none of which should be happening.
#get_data should be idempotent. Right now, if you call it more than once, you'll just get larger and larger inc/dec values because you keep adding to the same @result_data hash. But again, you don't want a class anyway.
And #get_data is stranger still. You set @today_value again and again although it doesn't change in the inner each loop.
Fix your indentation
Don't call a class CompareData. Classes should be nouns; "compare data" is imperative. And don't call a variable init, and don't pluralize data since it's already plural.
Be consistent with your parentheses. You use them for #initialize, but you don't for #compare_data

Here's something that I think is closer to what you're actually trying to do

changes = values.inject({'g_yesterday' => {inc: 0, dec: 0}, 'g_week' => {inc: 0, dec: 0}, 'g_month' => {inc: 0, dec: 0}}) do |memo, datum|
  current = datum['g_current'].to_i
  memo.each do |key, hash|
    previous = datum[key].to_i
    if current < previous
      hash[:inc] += 1
    elsif current > previous
      hash[:dec] += 1
    end
  end
  memo
end

No class, no strange coupling or state-keeping. Just input and output.

Update: New code posted, and while my answer was already accepted, I might as well. The new code is thankfully working with numbers rather than strings, which helps, but most of my points above still stand. Looking at the #compare_data method again:

def compare_data second_value, date
  # just return if you don't want to run the rest of the code
  unless @today_value == 0 && second_value == 0

    # again: This condition makes the one above pointless
    # and again, just return instead of wrapping everything
    if @today_value != second_value
      period = @result_data[date]

      # Why are zeros treated differently? Isn't the point simply to figure
      # out what value is higher, and update inc/dec accordingly?
      # Why does one of the values being zero make it behave exactly
      # opposite of how it behaves if both values are non-zero?
      #
      #  today_value   |  second_value   |  Add 1 to...
      # ----------------------------------------------
      #  1  (highest)  |  0              |  :inc
      #  2  (highest)  |  1              |  :dec
      #
      # This doesn't make sense to me...
      if @today_value == 0 || second_value == 0
        @today_value > second_value ? period[:inc] += 1 : period[:dec] += 1
      else
        @today_value > second_value ? period[:dec] += 1 : period[:inc] += 1
      end
    end
  end
end


Counting length-2 substrings that are common to two strings at the same offset I am working through the CodingBat exercises for Java. I just completed this one, an exercise that requests the comparison of substrings of two strings:

  Given 2 strings, a and b, return the number of the positions where they contain the same length 2 substring. So "xxcaazz" and "xxbaaz" yields 3, since the "xx", "aa", and "az" substrings appear in the same place in both strings.

stringMatch("xxcaazz", "xxbaaz") → 3
stringMatch("abc", "abc") → 2
stringMatch("abc", "axc") → 0

Here is my method (which does work):

public int stringMatch(String a, String b) {
    int count = 0;

    String sub2 = "";

    String arrayA[] = new String[a.length()];
    String arrayB[] = new String[b.length()];

    for (int i = 0; i < a.length()-1; i++) {
        sub2 = a.substring(i, i+2);
        arrayA[i] = sub2;
    }

    for (int i = 0; i < b.length()-1; i++) {
        sub2 = b.substring(i, i+2);
        arrayB[i] = sub2;
    }

    for (int j = 0; j < arrayA.length-1; j++) {

        if (j >= b.length())
            break;

        if (arrayA[j].equals(arrayB[j]))
            count++;
    }

    return count;
}

And here is the answer on the site:

public int stringMatch(String a, String b) {

  int len = Math.min(a.length(), b.length());
  int count = 0;

  for (int i=0; i<len-1; i++) {
    String aSub = a.substring(i, i+2);
    String bSub = b.substring(i, i+2);
    if (aSub.equals(bSub)) {  // Use .equals() with strings
      count++;
    }
  }

  return count;
}

I feel that my code is longer and messy and drawn out compared to other people's.  As a beginner programmer, how should I improve it?

If I were you I would try to write more concise solutions. Readability is extremely important. If you write code that is hard to read, it will not be reused as much, it will be hard to change or extend the code, your code will probably perform worse, and it will be hard to find bugs.

Your first approach can be based on your intuition, but then you should look at your code, and try to find ways to simplify it. With time and experience, this will get easier and easier, and your intuitive solution will more closely resemble a concise solution.

Ignoring the complexity of your solution, here are some things that could be better:

Your first two for loops perform exactly the same functionality, and you shouldn't duplicate code. Whenever you copy-paste a piece of code, think about extracting it to a method. This will increase the readability, maintainability, and reusability of your code.
define variables when they are needed, not earlier. The fewer variables a reader has to remember at any point, the easier it will be to understand your code. count for example isn't needed at the beginning of the method, but just in the last for loop, so declare it there. Another example is sub2, which isn't needed at all, and which also doesn't have a very expressive name.
use spaces around - and + to increase readability.
don't declare variables that are only used once, but use those values directly, eg arrayA[i] = a.substring(i, i+2);.
use curly brackets even for one line statements to increase readability and possibly avoid future bugs.
be consistent. Why do you use i as loop variable for the first two loops, but not the third? If there is no reason, use i for all.
Why have if (j >= b.length())? It's not intuitively clear what it does, so you should at least add a comment.


Bilinear interpolation The following code is to enlarge pictures using bilinear interpolation.  What can be modified in the function of slow_rescale to make it more efficient?  I expect to modify it from the view of Principles of Computer Organization.

unsigned char *slow_rescale(unsigned char *src, int src_x, int src_y, int dest_x, int dest_y)
{
 double step_x,step_y;          // Step increase as per instructions above
 unsigned char R1,R2,R3,R4;     // Colours at the four neighbours
 unsigned char G1,G2,G3,G4;
 unsigned char B1,B2,B3,B4;
 double RT1, GT1, BT1;          // Interpolated colours at T1 and T2
 double RT2, GT2, BT2;
 unsigned char R,G,B;           // Final colour at a destination pixel
 unsigned char *dst;            // Destination image - must be allocated here!
 int x,y;               // Coordinates on destination image
 double fx,fy;              // Corresponding coordinates on source image
 double dx,dy;              // Fractional component of source image    coordinates

 dst=(unsigned char *)calloc(dest_x*dest_y*3,sizeof(unsigned char));   // Allocate and clear   destination image
 if (!dst) return(NULL);                           // Unable to allocate image

 step_x=(double)(src_x-1)/(double)(dest_x-1);
 step_y=(double)(src_y-1)/(double)(dest_y-1);

 for (x=0;x<dest_x;x++)         // Loop over destination image
 for (y=0;y<dest_y;y++)
  {
fx=x*step_x;
fy=y*step_y;
dx=fx-(int)fx;
dy=fy-(int)fy;
getPixel(src,floor(fx),floor(fy),src_x,&R1,&G1,&B1);    // get N1 colours
getPixel(src,ceil(fx),floor(fy),src_x,&R2,&G2,&B2); // get N2 colours
getPixel(src,floor(fx),ceil(fy),src_x,&R3,&G3,&B3); // get N3 colours
getPixel(src,ceil(fx),ceil(fy),src_x,&R4,&G4,&B4);  // get N4 colours
   // Interpolate to get T1 and T2 colours
   RT1=(dx*R2)+(1-dx)*R1;
   GT1=(dx*G2)+(1-dx)*G1;
   BT1=(dx*B2)+(1-dx)*B1;
   RT2=(dx*R4)+(1-dx)*R3;
   GT2=(dx*G4)+(1-dx)*G3;
   BT2=(dx*B4)+(1-dx)*B3;
   // Obtain final colour by interpolating between T1 and T2
   R=(unsigned char)((dy*RT2)+((1-dy)*RT1));
   G=(unsigned char)((dy*GT2)+((1-dy)*GT1));
   B=(unsigned char)((dy*BT2)+((1-dy)*BT1));
  // Store the final colour
  setPixel(dst,x,y,dest_x,R,G,B);
 }
  return(dst);
}
void getPixel(unsigned char *image, int x, int y, int sx, unsigned char *R, unsigned char *G, unsigned char *B)
{
 // Get the colour at pixel x,y in the image and return it using the provided RGB pointers
 // Requires the image size along the x direction!
 *(R)=*(image+((x+(y*sx))*3)+0);
 *(G)=*(image+((x+(y*sx))*3)+1);
 *(B)=*(image+((x+(y*sx))*3)+2);
}

void setPixel(unsigned char *image, int x, int y, int sx, unsigned char R, unsigned char G, unsigned char B)
{
 // Set the colour of the pixel at x,y in the image to the specified R,G,B
 // Requires the image size along the x direction!
 *(image+((x+(y*sx))*3)+0)=R;
 *(image+((x+(y*sx))*3)+1)=G;
 *(image+((x+(y*sx))*3)+2)=B;
}

Please make everyone (including yourself) a favor :

Declare your variable in the smallest possible scope. Also define them as your declare them if you can (and you usually can).
Indent your code properly.

Then you'd get something like :

unsigned char *slow_rescale(unsigned char *src, int src_x, int src_y, int dest_x, int dest_y)
{
    unsigned char* dst=(unsigned char *)calloc(dest_x*dest_y*3,sizeof(unsigned char));   // Allocate and clear destination image
    if (!dst) return(NULL); // Unable to allocate image

    double step_x=(double)(src_x-1)/(double)(dest_x-1);
    double step_y=(double)(src_y-1)/(double)(dest_y-1);

    for (int x=0;x<dest_x;x++) // Loop over destination image
    {
        for (int y=0;y<dest_y;y++)
        {
            double fx=x*step_x;
            double fy=y*step_y;
            double dx=fx-(int)fx;
            double dy=fy-(int)fy;

            unsigned char R1,R2,R3,R4; // Colours at the four neighbours
            unsigned char G1,G2,G3,G4;
            unsigned char B1,B2,B3,B4;

            getPixel(src,floor(fx),floor(fy),src_x,&R1,&G1,&B1); // get N1 colours
            getPixel(src,ceil(fx),floor(fy),src_x,&R2,&G2,&B2); // get N2 colours
            getPixel(src,floor(fx),ceil(fy),src_x,&R3,&G3,&B3); // get N3 colours
            getPixel(src,ceil(fx),ceil(fy),src_x,&R4,&G4,&B4);  // get N4 colours
            // Interpolate to get T1 and T2 colours
            double RT1=(dx*R2)+(1-dx)*R1;
            double GT1=(dx*G2)+(1-dx)*G1;
            double BT1=(dx*B2)+(1-dx)*B1;
            double RT2=(dx*R4)+(1-dx)*R3;
            double GT2=(dx*G4)+(1-dx)*G3;
            double BT2=(dx*B4)+(1-dx)*B3;
            // Obtain final colour by interpolating between T1 and T2
            unsigned char R=(unsigned char)((dy*RT2)+((1-dy)*RT1));
            unsigned char G=(unsigned char)((dy*GT2)+((1-dy)*GT1));
            unsigned char B=(unsigned char)((dy*BT2)+((1-dy)*BT1));
            // Store the final colour
            setPixel(dst,x,y,dest_x,R,G,B);
        }
    }
    return(dst);
}
void getPixel(unsigned char *image, int x, int y, int sx, unsigned char *R, unsigned char *G, unsigned char *B)
{
    // Get the colour at pixel x,y in the image and return it using the provided RGB pointers
    // Requires the image size along the x direction!
    *(R)=*(image+((x+(y*sx))*3)+0);
    *(G)=*(image+((x+(y*sx))*3)+1);
    *(B)=*(image+((x+(y*sx))*3)+2);
}

void setPixel(unsigned char *image, int x, int y, int sx, unsigned char R, unsigned char G, unsigned char B)
{
    // Set the colour of the pixel at x,y in the image to the specified R,G,B
    // Requires the image size along the x direction!
    *(image+((x+(y*sx))*3)+0)=R;
    *(image+((x+(y*sx))*3)+1)=G;
    *(image+((x+(y*sx))*3)+2)=B;
}

Then, try to compute things once and only once when possible and you'll get something like :

unsigned char *slow_rescale(unsigned char *src, int src_x, int src_y, int dest_x, int dest_y)
{
    unsigned char* dst=(unsigned char *)calloc(dest_x*dest_y*3,sizeof(unsigned char));   // Allocate and clear destination image
    if (!dst) return(NULL); // Unable to allocate image

    double step_x=(double)(src_x-1)/(double)(dest_x-1);
    double step_y=(double)(src_y-1)/(double)(dest_y-1);

    for (int x=0;x<dest_x;x++) // Loop over destination image
    {
        double fx=x*step_x;
        double dx=fx-(int)fx;
        int ffx = floor(fx);
        int cfx = ceil(fx);
        for (int y=0;y<dest_y;y++)
        {
            double fy=y*step_y;
            double dy=fy-(int)fy;
            int ffy = floor(fy);
            int cfy = ceil(fy);

            unsigned char R1,R2,R3,R4,G1,G2,G3,G4,B1,B2,B3,B4;

            getPixel(src, (ffx + ffy*src_x)*3, &R1,&G1,&B1); // get N1 colours
            getPixel(src, (cfx + ffy*src_x)*3, &R2,&G2,&B2); // get N2 colours
            getPixel(src, (ffx + cfy*src_x)*3, &R3,&G3,&B3); // get N3 colours
            getPixel(src, (cfx + cfy*src_x)*3, &R4,&G4,&B4); // get N4 colours
            // Interpolate to get T1 and T2 colours
            double RT1 = dx*R2 + (1-dx)*R1;
            double GT1 = dx*G2 + (1-dx)*G1;
            double BT1 = dx*B2 + (1-dx)*B1;
            double RT2 = dx*R4 + (1-dx)*R3;
            double GT2 = dx*G4 + (1-dx)*G3;
            double BT2 = dx*B4 + (1-dx)*B3;
            // Obtain final colour by interpolating between T1 and T2
            unsigned char R = dy*RT2 + (1-dy)*RT1;
            unsigned char G = dy*GT2 + (1-dy)*GT1;
            unsigned char B = dy*BT2 + (1-dy)*BT1;
            // Store the final colour
            setPixel(dst, (x + y*dest_x)*3,R,G,B);
        }
    }
    return(dst);
}

inline void getPixel(unsigned char *image, int offset, unsigned char *R, unsigned char *G, unsigned char *B)
{
    *R=image[offset+0];
    *G=image[offset+1];
    *B=image[offset+2];
}

inline void setPixel(unsigned char *image, int offset, unsigned char R, unsigned char G, unsigned char B)
{
    image[offset+0]=R;
    image[offset+1]=G;
    image[offset+2]=B;
}

I'd expect this to be much much faster than your original code and to do roughtly the same thing (I have have introduced errors though).


Three buttons with click listeners that pop up a quick toast I currently have three buttons, perhaps more to come. When I click on a button, I want a toast notification or message (popup message) to show up with some text. What I have works perfectly but it is very redundant.

How I refactor what I have below to prevent redundant code?

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);

        Button button1 = (Button) findViewById(R.id.button1);
        button1.setOnClickListener(new View.OnClickListener() {
            public void onClick(View v) {

                Context context = getApplicationContext();
                CharSequence text = "This button will launch button1 App!";
                int duration = Toast.LENGTH_SHORT;

                Toast toast = Toast.makeText(context, text, duration);
                toast.show();
            }
        });

        Button button2 = (Button) findViewById(R.id.button2);
        button2.setOnClickListener(new View.OnClickListener() {
            public void onClick(View v) {

                Context context = getApplicationContext();
                CharSequence text = "This button will launch button2 App!";
                int duration = Toast.LENGTH_SHORT;

                Toast toast = Toast.makeText(context, text, duration);
                toast.show();
            }
        });

        Button  button3= (Button) findViewById(R.id.button3);
        button3.setOnClickListener(new View.OnClickListener() {
            public void onClick(View v) {

                Context context = getApplicationContext();
                CharSequence text = "This button will launch button3 App!";
                int duration = Toast.LENGTH_SHORT;

                Toast toast = Toast.makeText(context, text, duration);
                toast.show();
            }
        });
    }

Anonymous classes and final keyword
It's good that you realize that you have redundancy in your code.  I think what is holding you back from refactoring the code yourself is that you aren't sure how to set the same onClickListener for each button because each button has a slightly different toast to pop up.
What you need to do is use final variables.  A final variable declared outside an anonymous class instantiation can be used within the anonymous class (i.e. your View.OnClickListener).  So in your case, you could do this:
protected void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    setContentView(R.layout.activity_main);

    setToastForButton(R.id.button1, "This button will launch button1 App!");
    setToastForButton(R.id.button2, "This button will launch button2 App!");
    setToastForButton(R.id.button3, "This button will launch button3 App!");
}

// Notice the final keyword here.  It allows the variable "text" to
// be used in the onClick() method below.
void setToastForButton(int btnId, final CharSequence text)
{
    Button button = (Button) findViewById(btnId);

    button.setOnClickListener(new View.OnClickListener() {
        public void onClick(View v) {
            Context context = getApplicationContext();
            int duration = Toast.LENGTH_SHORT;

            Toast toast = Toast.makeText(context, text, duration);
            toast.show();
        }
    });
}

You can learn more about anonymous classes here.


Optimization of exponentiation I'm writing some crypto code, and as part of it, we have to implement modular exponentiation.
I have it working, both with the right-to-left binary method and with the Montgomery formula.

While they both work, the Montgomery calculations take three times longer than the right-to-left binary method. This is extremely frustrating, as I worked out the right-to-left in an afternoon, but it's taken me three days solid work to figure out the Montgomery. I thought it would be faster, so I'm guessing it's my implementation - three different function calls are used to work it out.

Here is the code, if anyone can see anything that would speed it up I'd be delighted to hear it.

First I needed a function to get the values for Extended Euclid -

private static BigInteger[] extendedEuclid(BigInteger p, BigInteger q) {

if(q.compareTo(BigInteger.ZERO) == 0)
    return new BigInteger[] {p, BigInteger.ONE, BigInteger.ZERO};

BigInteger[] vals = extendedEuclid(q, p.mod(q));
BigInteger d = vals[0];
BigInteger a = vals[2];
BigInteger divBit = p.divide(q);
BigInteger mulBut = divBit.multiply(vals[2]);
BigInteger b = vals[1].subtract(mulBut);
return new BigInteger[] {d, a, b};
}

This works but calculates value I never need - all I use is values[1] but to get it I need to calculate the others, I think?

Next we have the helper function monPro :

private static BigInteger monPro(BigInteger aBar, BigInteger bBar, BigInteger r, BigIntegernPrime, BigInteger n) {

    BigInteger t = aBar.multiply(bBar);
    BigInteger m = (t.multiply(nPrime)).mod(r);
    BigInteger u = (m.multiply(n).add(t)).divide(r);

    if(u.compareTo(n) >= 0)
        return u.subtract(n);
    else return u;
}

This is needed to use the final function, modExp -

private static BigInteger modExp(BigInteger M, BigInteger e, final BigInteger n) {
BigInteger r = new BigInteger("2").pow(n.bitLength());
BigInteger[] values = extendedEuclid(r, n);
BigInteger rInverse = values[1].mod(n);
BigInteger mBar = M.multiply(r).mod(n);
BigInteger xBar = new BigInteger("1").multiply(r).mod(n);
BigInteger nPrime = n.negate().modInverse(r);

for(int i = e.bitLength() -1 ; i >= 0 ; i--) {
    xBar = monPro(xBar, xBar, r, nPrime, n);
    if (e.testBit(i) == true)
        xBar = monPro(mBar, xBar, r, nPrime, n);
    }
    BigInteger returnValue = monPro(xBar, BigInteger.ONE, r, nPrime, n);
    return returnValue;
    }
}

I'm almost certain the running time is due to the amount of time the actual modExp functon calls monPro. Euclid is only called once so I can't see that being the huge deal, I could be wrong. Anyway, any tips or pointers much appreciated, this has turned out to be an extremely frustrating exercise (optimize for three days and slow it down by a factor of three....booooooo!)

PS - I'm not allowed use the BigInteger function modPow.

There are two points where you can improve the performance. This is more related to the algorithm itself, but I guess the answer fits better here than in crypto.SE...

When computing m and u, you are using mod(r) and divide(r). But since r is a power of 2, these are just a bitwise and (with r - 1) and a right shift (by n.bitLength()), which should be much faster. It also helps reducing t modulo r (also using and) before multiplying by nPrime.
Montgomery works better at the word level. If n has w words, then your code takes 2 * (w^2) word multiplications when computing m and u, but could take w^2 + w when using word-level Montgomery. So if you really need speed, you'll need to use an array of integers instead of BigInteger.