Project Euler #14 (Longest Collatz sequence) A Collatz sequence for a given positive integer \$n\$ is to take half of \$n\$ if even and \$3n+1\$ if \$n\$ is odd; repeat until the value goes to \$1\$.  The question is which number from 1 to 1,000,000 has the longest Collatz sequence?

The Collatz sequence for \$n=13\$ from Project Euler #14:

  13 → 40 → 20 → 10 → 5 → 16 → 8 → 4 → 2 → 1

I wrote this code with lists, but the program never seemed to come to an end. I rewrote it with arrays, and it's still the same. Starting from 10, increasing the number 10 times seems to increase the program run time by roughly 100 times, and at this rate it will take about 15 days for it to complete.

The algorithm is to take a number, generate its Collatz sequence, and eliminate all the even multiples of the numbers from the sequence.

import numpy as np

p = 1000000    #maximum number that the code works up to
collatz_array = np.zeros([3, p-2], dtype='int64')

for i in range(2, p):
    i = int(i)
    collatz_array[0][i-2] = collatz_array[1][i-2] = i  #1st row and 2nd row are populated with natural numbers from 2 to p

for i in collatz_array[0]: #the numbers in the first row of the array can be zeroed if they are done with, while the second row is used as a reference
    if i != 0:
        k = i    #for i to remain unchanged
        count = 1
        sequence = []    #list to store the complete collatz sequence
        sequence.append(k)
        while k > 1:
            if k % 2 == 0:
                count += 1
                k = k//2
                sequence.append(k)
            else:
                count += 1
                k = int((3*k)+1)
                sequence.append(k)
        c = 0
        for q in sequence:                           #q is an element in the sequence
            if q in collatz_array[0]:
                q = int(q)
                collatz_array[2][q-2] = count - c    #the length of the collatz sequence of a number is stored in the third row
                                                     #as the sequence progresses, the count is decreased with 'c'
                w = q                                #for q to remain unchanged
                for e in range(1, p//q):             #loop is run for about the number of even multiples of q
                    w *= 2                           #the next even multiple
                    if w in collatz_array[0]:
                        collatz_array[2][w-2] = collatz_array[2][i-2] + e    #the length of the sequence increases by 1, for an even multiple
                        collatz_array[0][w-2] = 0
                collatz_array[0][q-2] = 0
            c += 1

max_value, max_count = 0, 0
for i in range(2, p):
    if max_count < collatz_array[2][i-2]:
        max_count = collatz_array[2][i-2]
        max_value = collatz_array[1][i-2]

print("Number with longest collatz sequence: ", max_value)

Example: i = 13, sequence = [13, 40, 20, 10, 5, 16, 8, 4, 2, 1], count = 10 for 13 and decreases by 1 for the next number,  w = 13 and it's even multiples 26, 52, 104 and so on are eliminated from collatz_array[0] with their counts increased by 1.

This is the best algorithm I have been able to think of, that seems to work. Is it possible to start from 2 and go up? I can't think of a complete algorithm to do that.

(P.S. I know there are other similar questions, which I haven't gone through and might do after solving the problem myself, but I only want to optimize my code.)

First, put everything in a main function. This makes the variables local, which makes lookups a little faster. It's also neater.

It's worth formatting the comments with line wrapping and putting them in front of the commented line to keep lines short.

collatz_array[1] is never used except to iterate over, so just iterate over range(2, p) instead and ditch collatz_array[1]. In fact, at point of use it's simpler to just call enumerate:

max_value, max_count = 0, 0
for i, val in enumerate(collatz_array[2], 2):
    if max_count < val:
        max_count = val
        max_value = i

Then you should split collatz_array into two arrays, which I'm tentatively calling progression_mask and progression_length. They can be initialized as

progression_mask = np.arange(2, p, dtype='int64')
collatz_array = np.zeros(p-2, dtype='int64')

count is just len(sequence), so generate it at the end with len. When looping over sequence you can do:

count = len(sequence)
for q in sequence:
    ...
    count -= 1

if q in progression_mask is \$O(n)\$ but you can do the check in \$O(1)\$ with

if q < p and progression_mask[q-2]:

The same goes for if w in progression_mask.

This gets you much, much faster... but we can go faster still.

I would remove the -2 from the indexes by changing the arrays to:

progression_mask = np.arange(p, dtype='int64')
progression_length = np.zeros(p, dtype='int64')

Using pure lists is faster than Numpy arrays, so switch to them:

progression_mask = list(range(p))
progression_length = [0] * p

The final loop can be simplified with a max and index:

max_count = max(progression_length)
max_value = progression_length.index(max_count)

The main loop would be better with an early exit on the if i != 0 check, and it should be spelt if i (or if not i when inverted).

for i in progression_mask:
    if not i:
        continue

Generating the collatz numbers should be done with a function:

def collatz_progression(n):
    while n > 1:
        yield n

        if n % 2 == 0:
            n = n//2
        else:
            n = int((3*n)+1)

Note that this doesn't yield 1, as I don't think it's needed. It's called as

sequence = list(collatz_progression(i))

This isn't needed once we've moved to plain lists:

q = int(q)

If progression_mask is 0, we don't need to continue so can break from the loop:

if q < p:
    if not progression_mask[q]:
        break

    ...

count -= 1

I would ditch progression_mask and just check if progression_length is set.

Your loop

w = q
# Loop is run for about the number of even multiples of q
# the next even multiple
for e in range(1, p//q):
    w *= 2
    if w < p and progression_mask[w]:

would be better as

w = q
count_ = count
while w < p:
    progression_length[w] = count_

    count_ += 1
    w *= 2

in part so it can replace the other assignment. I would move it into a new function and rename the variables:

def assign_ascending_collatz(start, count, progression_length):
    while start < len(progression_length):
        progression_length[start] = count

        start *= 2
        count += 1

n = int((3*n)+1) can just be n = 3*n + 1.

if not n % 2 is more idiomatic than if n % 2 == 0, but even better is to flip the conditional and do

if n % 2:
    n = 3*n + 1
else:
    n = n//2

or maybe even

n = (3*n + 1) if n % 2 else (n // 2)

This gives:

def collatz_progression(n):
    while n > 1:
        yield n

        n = (3*n + 1) if n % 2 else (n // 2)

def assign_ascending_collatz(start, count, progression_length):
    while start < len(progression_length):
        progression_length[start] = count

        start *= 2
        count += 1

def main():
    up_to = 1000000

    progression_length = [-1] * up_to

    for i in range(up_to):
        if progression_length[i] != -1:
            continue

        sequence = list(collatz_progression(i))

        for i, val in enumerate(sequence):
            if val < up_to and progression_length[val] != -1:
                    break

            # Cache the count
            assign_ascending_collatz(val, len(sequence)-i, progression_length)

    max_count = max(progression_length)
    max_value = progression_length.index(max_count)

    print("Number with longest collatz sequence: ", max_value)

main()

With PyPy3 this takes me ~1.5s to run.

Most of the time seems to be in collatz_progression, so we can speed that up by letting it short-circuit against progression_length. This gives:

def collatz_progression(n, progression_length):
    while True:
        yield n

        if n < len(progression_length) and progression_length[n] != -1:
            return

        n = (3*n + 1) if n % 2 else (n // 2)

def assign_ascending_collatz(start, count, progression_length):
    while start < len(progression_length):
        progression_length[start] = count

        start *= 2
        count += 1

def main():
    up_to = 1000000

    progression_length = [-1] * up_to

    # Skip 0 and set 1
    progression_length[0] = 0
    progression_length[1] = 0

    for i in range(up_to):
        if progression_length[i] != -1:
            continue

        sequence = list(collatz_progression(i, progression_length))
        count = len(sequence) + progression_length[sequence.pop()]

        for val in sequence:
            # Cache the count
            assign_ascending_collatz(val, count, progression_length)
            count -= 1

    max_count = max(progression_length[1:])
    max_value = progression_length.index(max_count)

    print("Number with longest collatz sequence: ", max_value)

main()

Note that I had to fix progression_length[1] to avoid the -1 adding to the other values.

With PyPy3 this takes ~0.3 seconds to run. Hopefully that's fast enough.

Note that now your clever optimization of going forward as well with assign_ascending_collatz doesn't actually help. Getting rid of it even gives a tiny speed improvement:

def collatz_progression(n, progression_length):
    while True:
        yield n

        if n < len(progression_length) and progression_length[n] != -1:
            return

        n = (3*n + 1) if n % 2 else (n // 2)

def main():
    up_to = 1000000

    progression_length = [-1] * up_to

    # Skip 0 and set 1
    progression_length[0] = 0
    progression_length[1] = 0

    for i in range(up_to):
        if progression_length[i] != -1:
            continue

        sequence = list(collatz_progression(i, progression_length))
        count = len(sequence) + progression_length[sequence.pop()]

        for val in sequence:
            if val < len(progression_length):
                progression_length[val] = count

            count -= 1

    max_count = max(progression_length[1:])
    max_value = progression_length.index(max_count)

    print("Number with longest collatz sequence: ", max_value)

main()

Some good questions from the comments:

  Before turning the lists into arrays, for some p, the execution time was 305s and after the change, it became 448s. Does it not count against lists?

Numpy arrays are faster than lists for some things and slower for others. In particular, arrays tend to be slower when indexing single items (eg. my_array[12]). They tend to be faster on operations that affect the whole array, such as q in progression_mask, which searches the whole array.

Before removing if q in progression_mask and if w in progression_mask, most of the time was spent in those two operations. This meant arrays were significantly faster.

After removing them, all operations on the array were just indexes. This means that lists became faster.

So lists are only faster after making certain changes to the code.

  Why did you populate the progression_length with -1? It didn't seem to affect the result, either with 0 or -1.

I (arbitrarily) set progression_length[1] = 0 (instead of 1). This meant that 0 was a valid value, so it didn't make sense in my opinion to use it as a sentinel.

I would have chosen None instead of -1 if PyPy didn't have a special optimization for lists of only integers, meaning it's faster if I use an integer. If you don't want to use PyPy or don't mind PyPy being a little slower, I suggest using None.

  And you lost me completely after you 'short-circuited collatz_progression against progression_length'.

Let's say you have these lengths:

  0 1 2 3 4 5 6 7
[ 0 0 1 6 1 4 ? ? ]

where the ? is represented with -1.

We want to find the collatz number for 6. The original method would find this list:

[6, 3, 10, 5, 16, 8, 4, 2]

find its length and then loop over it, setting the values in progression_lengths.

However, once you have gotten

[6, 3, ...]

you know that every number afterwards is already set in the array since progression_length[3] != -1. This means you don't need to set them in the array.

You still need to know the number of skipped items, so you look it up from

# The last value before we stopped
progression_length[3]

which gives 6. You add that to the number of values in front (1) and get 7. This means you only have to generate 2 values from collatz_progression instead of 7 in order to get the appropriate length.

  And why didn't even multiples elimination not work?

It did work but the above optimization does the same thing in a better way.


Rendering three vertical dots using SCSS I have a pretty simple question but I can't seem to find a solution for it. I have the following SCSS code (can be found below) which works fine, but I think it could be simplified to get rid of the double code.

I'm trying to create 3 simple dots with CSS. I figured I could use one div and the :before and :after selectors. I'm looking for a pure (S)CSS solution and don't want to use icons/images. I have a working example but I feel like it could be a lot better structured.

Mainly I'm looking to change the selectors so I can get a format like:

div {
  // Styling for the div in it's normal state.
  [selector for div itself] &:after, &:before {
    // Styling for the div in it's normal state AND for it's :before and :after state
  }
  &:after, &:before {
    // Styling for JUST the :before and :after state
  }
  &:after {
    // Styling for JUST the :after state
  }
  &:before {
    // Styling for JUST the :before state
  }
}

Current code

SCSS:

.row-options-icon{
  padding: 2rem;
  &:hover{
    div, div:after, div:before{
      background-color: black;
    }
  }
  div {
    position: relative;
    border-radius:100px;
    background-color:gray;
    width:.5rem;
    height:.5rem;
    &:after, &:before {
      content: '';
      position: absolute;
      border-radius:100px;
      background-color:gray;
      width:.5rem;
      height:.5rem;
      display: inline-block;
    }
    &:after {
      top: -1rem;
    }
    &:before {
      top: 1rem;
    }
  }
}

HTML:

Working Codepen example

What you're looking for is just &:

  div {
    position: relative;
    &, &:after, &:before {
      border-radius:100px;
      background-color:gray;
      width:.5rem;
      height:.5rem;
    }
    &:after, &:before {
      content: '';
      position: absolute;
      display: inline-block;
    }
    &:after {
      top: -1rem;
    }
    &:before {
      top: 1rem;
    }
  }


Billing for work using an hourly, dual, or flat rate I asked this on Stack Overflow and got some good ideas, but was advised to post here instead. There are obviously several ways to code this, each with a drawback and a benefit.

A person works a number of hours and can bill for the work in one of three ways:

Hourly: the number of hours worked * hourly rate
Dual: (for simplicity sake) first rate + second rate
Flat: just an amount, regardless of the hours worked

Here's the code that I came up with, but I just can't help but think that there's a much more elegant way to do this. In the code below I have a class per rate type, one base class for a rate, a person, and a calculator that does the math. Suggestions?

using System;

public class Program
{
    public static void Main()
    {
        Person p = new Person();
        p.HoursWorked = 2;
        p.Rate = new Hourly(10);
        //p.Rate = new Dual(10, 1);
        //p.Rate = new Flat(10);
        Console.WriteLine(Calculator.Calculate(p));
    }
}

public class Person
{
    public RateStyle Rate { get; set; }
    public int HoursWorked { get; set; }
}

public abstract class RateStyle
{
}

public class Dual : RateStyle
{
    public int First { get; private set; }
    public int Second { get; private set; }

    public Dual(int a, int b)
    {
        First = a;
        Second = b;
    }
}

public class Hourly : RateStyle
{
    public int Rate { get; private set; }
    public Hourly(int r)
    {
        Rate = r;
    }
}

public class Flat : RateStyle
{
    public int Rate { get; private set; }
    public Flat(int r)
    {
        Rate = r;
    }
}

public static class Calculator
{
    public static int Calculate(Person p)
    {
        if (p.Rate is Hourly)
        {
            return p.HoursWorked * ((Hourly)p.Rate).Rate;
        }
        if (p.Rate is Dual)
        {
            return ((Dual)p.Rate).First + ((Dual)p.Rate).Second;
        }

        return ((Flat)p.Rate).Rate;
    }
}

OO or not

The first thing to do is decide whether or not you want an object-oriented design for this problem. From the requirements you've described, there's a very simple solution that doesn't need any real design work at all:

public static int CalculateFee(int flatRate, int hourlyRate, int hours)
{
    return flatRate + hourlyRate*hours;
}

(The two parts of a "dual rate" would just be summed before being passed in, and either flatRate or hourlyRate could be 0)

You can then carry around those variables however is most convenient for you. For example maybe you'd put the rates together into some property-bag class:

public class Rate
{
    public int Hourly { get; set; }
    public int Flat { get; set; }
}

Or maybe you'd just pass them around as integers. Likewise whether CalculateFee was a public method somewhere or a private method, or even in-lined in another method, is really just a choice about how to organize things to read most clearly, rather than about design.

I think the above should be your default starting point. Building a design means making decisions, and every decision is a potential wrong decision, which becomes technical debt. So you should never build more or earlier than you're driven to by your requirements.

But there are some reasons you may immediately want a heavier design than this. For example:

You know- or think it's very likely- that you'll have to add new types of rates in addition to flat, dual and hourly.
There may be more functionality that you need associated with each rate, like wanting to be able to write a descriptive string ("A rate of 30$ per hour").
You want to be able to replace a rate with a test double for unit testing (I'm not sure that's really appropriate with these classes, it's hard to know without seeing how they're used)

Polymorphism

I won't write out a full OO version because Dmitry Nogin's answer already demonstrates this really well.

But to distill out the key point from that, if you're going to use an OO solution, you want to use polymorphism. If you're doing code like:

IThing thing;

if(thing is ThingTypeA)
{
    CalculationA(thing);
}
else if(thing is ThingTypeB)
{
    CalculationB(thing);
}
else if...

Then that's a pretty good sign that you should instead have a Calculation method on IThing, with a different implementation for ThingTypeA and ThingTypeB. That's a core part of OO design, and one that's easy to apply here (as Dmitry's answer shows)

Note that this also lets you encapsulate your data- those integer fields can be private to the Rate implementations. Your classes aren't just bags to hold data for anyone to read, they provide useful behaviour.


Calculating if vehicle's mileage is in service window I'm trying to calculate whether a given vehicle's mileage is within the recommended service windows.

For example, if a fuel filter replacement is every 15,000 miles and the service window is 5,000 miles, then a vehicle is within the service window from 12,500-17,500 miles, 27,500-32,500 miles, etc.

My current logic works (I think), but I'm not sure if it's too convoluted or if there are better ways of figuring this out:

$mileage = 27500;
$service = 15000;
$window = 2500;
$is_due = $mileage < $service
    ? $service - $mileage <= $window
    : $mileage % service <= $window
        || abs($mileage % $service - $service) <= $window;

This seems overly complex or not complex enough.  If the fuel filter is replaced at 12,500, then the next service window should be from 25,000 to 30,000, not 27,500 to 32,500.

If the fuel filter is not replaced by 17,500, it doesn't go back into service either.  It still needs replaced at 18,000, etc.

$current_mileage = 27500;
$last_service = 12500;
$service_interval = 15000;
$half_window = 2500;

$is_due = $current_mileage + $half_window <= $last_service + $service_interval;

$is_overdue = $current_mileage > $last_service + $service_interval + $half_window;

This seems to better reflect how service windows work in practice.


Find an array slice equaling sum I would like to be reviewed on efficiency, style, and obviously if there is a bug I would like to know.

def findsum(a, n):
  ''' Find whether a consecutive array slice sums up to n '''
  for i in range(len(a)):
    j = i + 1
    while j <= len(a):
      if sum(a[i:j]) == n:
        return True
      j += 1
  return False

There's a bug:

>>> a = [1, 2, 3]
>>> findsum(a, 0)
False

but the result should be True because the consecutive slice a[0:0] sums to zero.
The docstring could be clearer. "a consecutive array slice" — of which array? Presumably the parameter a is meant, but it is best to be explicit. Similarly, "Find whether" — and then what? The docstring should say what value is returned.
The function has runtime \$Θ(n^3)\$. That's because sum(a[i:j]) takes an unnecessary copy of the slice, and because it sums it starting at the beginning of the slice. But if you maintained a running sum, then the runtime would come down to \$Θ(n^2)\$. That's easy to implement using itertools.accumulate and  itertools.islice:

from itertools import accumulate, islice

def findsum(a, n):
    """Return True if any consecutive slice of the sequence a sums to n,
    False if none do.

    """
    return n == 0 or any(total == n
                         for i in range(len(a))
                         for total in accumulate(islice(a, i, None)))


Data class where i need to set values for different fields at different times I am creating a request for an api using below class-

class ABCDRequest {

    @JsonProperty("name")
    private var name: String? = null

    @JsonProperty("question")
    private var question: String? = null

    @JsonProperty("ans")
    private var ans: String? = null

    fun getName(): String? {
        return name
    }

    fun setName(userName: String?) {
        this.name = userName
    }

    fun getQuestion(): String? {
        return question
    }

    fun setQuestion(ques: String?) {
        this.question = ques
    }

    fun getAns(): String? {
        return ans
    }

    fun setAns(answer: String?) {
        this.ans = answer
    }

}

I need both setter and getter methods and i need to set values for different fields at different times(not at time time of creation of object for this class).

Is the above way idle or is there any 1 or few lines code for this in kotlin?

Use Data Class which automatically generates field accessors, hashCode(), equals(), toString() and other methods. Read this for a quick introduction.

data class ABCDRequest (
    @JsonProperty("name")
    private var name: String? = null,
    @JsonProperty("question")
    private var question: String? = null,
    @JsonProperty("ans")
    private var ans: String? = null
)

For the official documentation on Data Class, head over to this link.


Using NSPredicate with NSFetchRequest in a NSFetchedResultsController I am learning CoreData and I wanted to have my code reviewed.  As usual, I get something to work, then I attempt to refine it.
I have a data model that consists of an Item entity and a subItem entity.  The Item has a 'to many' relationship with the subItem and subItem has a 'to one' relationship with Item.  Basically Item can have multiple subitems, but subitem can only be tied to one Item.  My initial tableview displays the items and selecting a row segues to a tabbarcontroller that consists of other controllers, one being another tableview.  That tableview displays the subitems of the Item.  This is the code I have in my viewDidLoad:

NSFetchRequest *fetchRequest = [[NSFetchRequest alloc] init];

NSEntityDescription *subItemEntity = [NSEntityDescription entityForName:@"KFPSubItem" inManagedObjectContext:self.managedObjectContext];

[fetchRequest setEntity:subItemEntity];

NSPredicate *predicate = [NSPredicate predicateWithFormat:@"item = %@", self.record];

//add sort descriptors
[fetchRequest setSortDescriptors:@[[NSSortDescriptor sortDescriptorWithKey:@"name" ascending:YES]]];

//initialize fetchedresultscontroller
self.fetchedResultsController = [[NSFetchedResultsController alloc] initWithFetchRequest:fetchRequest managedObjectContext:self.managedObjectContext sectionNameKeyPath:nil cacheName:nil];

//configure fetched results controller
[self.fetchedResultsController setDelegate:self];

[self.fetchedResultsController.fetchRequest setPredicate:predicate];

//perform Fetch
NSError *error = nil;
[self.fetchedResultsController performFetch:&error];

if (error) {
    NSLog(@"Unable to perform fetch");
    NSLog(@"%@, %@", error, error.localizedDescription);
}

The entity and the sort descriptors of NSFetchRequest are defined as "properties"

@property(nonatomic, strong) NSEntityDescription *entity;
@property(nonatomic, strong) NSArray *sortDescriptors;

So while calling the setter methods still works

[fetchRequest setEntity:...];
[fetchRequest setSortDescriptors:...];

you should use the "dot-Syntax" for properties:

fetchRequest.entity = ...;
fetchRequest.sortDescriptors = ...;

and the same applies to the properties of NSFetchedResultsController.

The creation of the fetch request can be simplified with the
factory method

NSFetchRequest *fetchRequest = [NSFetchRequest fetchRequestWithEntityName:@"KFPSubItem"];

I usually assign the sort descriptors to separate variables first. This may
be a matter of taste, but it avoids too long statements if there is more
than one sort descriptor:

NSSortDescriptor *sortByName = [NSSortDescriptor sortDescriptorWithKey:@"name" ascending:YES];
fetchRequest.sortDescriptors = @[sortByName];

The

self.fetchedResultsController = [[NSFetchedResultsController alloc] ...

line is extremely long and should be split into several lines.

The correct way to check for success or failure of Cocoa (Touch) methods is
documented in
"Handling Error Objects Returned From Methods" in the "Error Handling Programming Guide":

  Important: Success or failure is indicated by the return value of the
  method. Although Cocoa methods that indirectly return error objects in
  the Cocoa error domain are guaranteed to return such objects if the
  method indicates failure by directly returning nil or NO, you should
  always check that the return value is nil or NO before attempting to
  do anything with the NSError object.

So instead of

NSError *error = nil;
[self.fetchedResultsController performFetch:&error];
if (error) { ... }

you should do

NSError *error = nil;
if (![self.fetchedResultsController performFetch:&error]) { .... }

Putting it all together:

NSFetchRequest *fetchRequest = [NSFetchRequest fetchRequestWithEntityName:@"KFPSubItem"];

NSSortDescriptor *sortByName = [NSSortDescriptor sortDescriptorWithKey:@"name" ascending:YES];
fetchRequest.sortDescriptors = @[sortByName];

NSPredicate *predicate = [NSPredicate predicateWithFormat:@"item = %@", self.record];
fetchRequest.predicate = predicate;

self.fetchedResultsController = [[NSFetchedResultsController alloc]
                                initWithFetchRequest:fetchRequest
                                managedObjectContext:self.managedObjectContext
                                  sectionNameKeyPath:nil
                                           cacheName:nil];
self.fetchedResultsController.delegate = self;

NSError *error = nil;
if (![self.fetchedResultsController performFetch:&error]) {
    NSLog(@"Unable to perform fetch");
    NSLog(@"%@, %@", error, error.localizedDescription);
}


JavaScript: Repeat a function X times at I intervals I've written a general purpose repeat function which allows you to repeat a callback function X times separated by I intervals with the option to start immediately or after the interval. It can also default to just looping infinitely, which is what setInterval does. Is there a better approach, or any improvements people can provide? Make sure to comment out all but one of the examples :)

function repeatXI(callback, interval, repeat, immediate) {
    repeat = typeof repeat == 'undefined' ? -1 : repeat;
    interval = interval <= 0 ? 1000 : interval;
    immediate = typeof immediate == 'undefined' ? false : immediate;
    var offset = immediate ? 0 : 1;
    var id = null;
    if (repeat > 0) {
        for (var i = 0; i < repeat; i++) {
            id = setTimeout(callback, interval * (i + offset));
        }
    } else {
        id = setInterval(callback, interval);
    }
    return id;
}

// Example
var someFunc = function () {console.log(1);}

// Repeats forever, 1 second apart
repeatXI(someFunc, 1000);
// Repeats 10 times, 1 second apart, beginning after 1 second
repeatXI(someFunc, 1000, 10);
// Same as above, but beginning now
repeatXI(someFunc, 1000, 10, true);

I don't see anything problematic with your code, but just for fun, here are some alternative approaches to various parts of the code. It's 100% DRY, and functionally identical to the original.

function repeatXI(callback, interval, repeats, immediate) {
  var timer, trigger;
  trigger = function () {
    callback();
    --repeats || clearInterval(timer);
  };

  interval = interval <= 0 ? 1000 : interval; // default: 1000ms
  repeats  = parseInt(repeats, 10) || 0;      // default: repeat forever
  timer    = setInterval(trigger, interval);

  if( !!immediate ) { // Coerce boolean
    trigger();
  }
}

And here's a jsfiddle demo

If you want, you can return timer from the function (as I do in the demo), so you can clear it (i.e. cancel the repeater) elsewhere in your code.

Regardless of the implementation-specifics it's worth noting that timers are unreliable at the best of times (since JS is single-threaded). Depending on your needs it may not be a good idea to set n timeouts at n * interval as you do for finite repeats. If a few of the timeouts get blocked long enough, they'll all queue up, and fire at once, once the thread clears. With setInterval though, you're guaranteed that there'll be at least interval time between the callbacks.

So it depends on your needs how you want to implement it.


C# class for translatable string I would like a class to store a default string, and also store translated values for that string. The results will be stored in json. I'm looking for a peer review on how this could be improved.

My biggest problem with this is to get the string value from the class you have to use the concrete type, using var will return the object.

Translatable String Class

namespace OpenPartsBook
{
    [Serializable()]
    public class TranslatableString
    {
        [JsonProperty]
        private string Text;
        public List<Translation> Translations { get; set; }

        public TranslatableString()
        {
            Translations = new List<Translation>();
        }
        public TranslatableString(string value)
        {
            Translations = new List<Translation>();
            Text = value;
        }

        /// <summary>
        /// Get a translation for the string by walking up the translation hierarchy
        /// if the requested language is not found it returns the default
        /// uses ISO 3166/ISO 3166-2
        /// </summary>
        /// <param name="LanguageCode">string - language code</param>
        /// <returns>string</returns>
        public string ToTranslatedString(string LanguageCode)
        {
            string temp = String.Empty;

            int i = LanguageCode.Length;

            if (this.Translations.Any(t => t.LanguageCode.ToLower().Equals(LanguageCode.ToLower())))
            {
                temp = this.Translations.First(t => t.LanguageCode.ToLower().Equals(LanguageCode.ToLower())).Text;
            }
            else
            {

                while ((i = LanguageCode.LastIndexOf("-", i)) != -1)
                {
                    var code = LanguageCode.Substring(0, i).ToLower();

                    if (this.Translations.Any(t => t.LanguageCode.ToLower().Equals(code.ToLower())))
                    {
                        temp = this.Translations.First(t => t.LanguageCode.ToLower().Equals(code.ToLower())).Text;
                        break;
                    }

                    --i;
                }
            }

            if (String.IsNullOrEmpty(temp)) temp = Text;

            return temp;
        }

        public static implicit operator TranslatableString(string s)
        {
            return s == null ? null : new TranslatableString(s);
        }

        public static implicit operator string(TranslatableString s) { return s.ToString(); }

        public override string ToString() { return Text; }

    }
}

Translation Class

namespace OpenPartsBook
{
    [Serializable()]
    [DataContract]
    public class Translation
    {
        [DataMember(Name = "languageCode", IsRequired = true)]
        public string LanguageCode { get; set; }
        [DataMember(Name = "text", IsRequired = true)]
        public string Text { get; set; }

        public Translation()
        {

        }
    }
}

Resulting Json:

"Title": {
    "Text": "Radiator",
    "Translations": [
      {
        "languageCode": "es",
        "text": "radiador"
      },
      {
        "languageCode": "fr",
        "text": "radiateur"
      }
    ]
  }

Localization

In my experience, translations are often provided per language, so I would normally store them per language rather than per piece of text.

I also prefer to have a single 'localization' object, rather than many 'translatable strings'. It gives me a clear entry-point for loading translations into memory and it provides a simple interface to other code: a single object with a GetText(string languageCode, string key) method.

Most applications only use a single language most of the time, so you may not even need to load all translations into memory. A central localization object makes it easier to perform such optimizations.

Some localization systems use the default (often English) text as a lookup key. But lately I've been using 'hierarchical' keys instead, such as MainWindow.Menu.FileButton or Settings.DataSources.DataSourceName. It gives translators slightly more context, which makes it easier for them to provide more accurate translations. And sometimes, where the default language uses the same word, another language uses different words depending on context. The hierarchical aspect of these keys allows me to organize the translations somewhat, so it's easier to prevent duplicate entries and all that.

Other notes

I don't think implicit conversion operators are a good idea here. They only really make sense when you're converting between similar types, and only when there's no risk of data loss. That's not the case here:

Converting a string to a TranslatableString gives you a translatable string that doesn't contain any translations. I don't see why that is useful, since the whole point of this class is to give access to translations.
Converting a TranslatableString to a string gives you the default text, but you lose all the translations. Instead, I would turn that Text field into a property with a public getter.

TranslatableString only needs one constructor if you give the value parameter a default value: (string value = null). Also, C# 6.0 introduced auto-property initializers, so you can further reduce code repetition in constructors: public List<Translation> Translations { get; set; } = new List<Translation>();
Making that Translations property publicly settable is probably not a good idea. I assume you're doing it for the free serialization, but it also allows any other code to modify it.
It's good to see some documentation! Perhaps you could add a small example with some hierarchical language codes for those that haven't memorized their ISO standards?

The ToTranslatedString method

It's doing double work. if (Any()) item = First() has to find a matching item twice. item = FirstOrDefault(); if (item != null) only has to find the item once.
A dictionary that maps language codes to translations would be a more appropriate data structure here.
Rather than repeatedly calling ToLower(), obtain a lower-case code beforehand, and make sure that your translations only use lower-case codes internally.
The main lookup code is duplicated. Instead of an if/else construction and a loop, you only need a loop. You just need to generalize the language code after you've tried to find a match.
You'll probably want to ignore a translation immediately if it's empty, rather than checking that at the end. Currently your code won't try to find a more general translation in such cases.
GetTranslation(string languageCode) is probably a better name - To implies some kind of conversion.

An improved version could look somewhat like this:

private Dictionary<string, Translation> _translations = new Dictionary<string, Translation>();

public string GetTranslation(string languageCode)
{
    languageCode = languageCode.ToLower();
    while (true)
    {
        if (_translations.TryGetValue(languageCode, out Translation translation) && !string.IsNullOrEmpty(translation.Text))
            return translation.Text;

        // Try again with a more general language code, if possible:
        var dashIndex = languageCode.LastIndexOf('-');
        if (dashIndex == -1)
            break;

        languageCode = languageCode.Substring(0, dashIndex);
    }
    return Text;
}


Game in Pythonista App I've written this Wack-a-mole like game on my iPad in the app Pythonista. Keeping that in mind, it uses a different graphic interface from the one I have usually seen used, Tkinter. I'm a beginner and this is my first major project, so any tips on how to code more efficiently and improve this program would be greatly appreciated.

import random
import ui
from time import sleep
import console
import sys

#turn button to "on" state
def turn_on(sender):
    sender.title = 'on'
    sender.tint_color = ('yellow')

#reverts button to off state
def turn_off(sender):
    sender.title = 'off'
    sender.tint_color = 'light blue'

#briefly turns button to "on" state then reverts button to       original     state
def blink(sender):
     turn_on(sender)
     sleep(.5)
     turn_off(sender)

#When the button is tapped, it reverses the current state of the button
def button_tapped(sender):
     turn_off(sender)

#Describes game
def game_Description():
    console.alert("Game objective:", 'Click the button to turn the   light off before the next light turns on', "Ready")

#Pops up when user loses game
def game_Over_Alert():
    play_Again = console.alert('Game Over!','','Play Again?')
    return play_Again

#Checks to see if all lights are off
def check_Lights():
    if button.title == 'off':
        if button2.title == 'off':
            if button3.title == 'off':
                if button4. title == 'off':
                        return True

#Turns off all lights
def all_Off():
    turn_off(button)
    turn_off(button2)
    turn_off(button3)
    turn_off(button4)

#Increase score by 1 and display
def increase_Score(x):
    x += 1
    score.title = 'Score: %d' % x
    return x

#setting UI and buttons
view = ui.View()
view.name = 'Light Panel'
view.background_color = 'black'

button = ui.Button(title = 'off')
button.center = (view.width*.2, view.height*.5)
button.flex = 'LRTB'
button.action = button_tapped
view.add_subview(button)

button2 = ui.Button(title = 'off')
button2.center = (view.width*.4, view.height*.5)
button2.flex = 'LRTB'
button2.action = button_tapped
view.add_subview(button2)

button3 = ui.Button(title = 'off')
button3.center = (view.width*.6, view.height*.5)
button3.flex = 'LRTB'
button3.action = button_tapped
view.add_subview(button3)

button4 = ui.Button(title = 'off')
button4.center = (view.width*.8, view.height*.5)
button4.flex = 'LRTB'
button4.action = button_tapped
view.add_subview(button4)

scoreCount = 0

score = ui.Button(title = 'Score: 0')
score.center = (view.width*.5, view.height*.75)
score.flex = 'LRTB'
view.add_subview(score)

#Set up display
view.present('sheet')

#Runs the game and handles the function
def play_Game():

    scoreCount = 0
    speed = 2.55
    game_Description()
    random.seed()
    while check_Lights():
     x = random.randint(0,3)
        if x == 0:
            turn_on(button)
        elif x == 1:
            turn_on(button2)
        elif x == 2:
            turn_on(button3)
        else:
            turn_on(button4)
        scoreCount = increase_Score(scoreCount)
        sleep(speed)
        if speed >= 1.5:
        speed-= .07

    if game_Over_Alert():
        all_Off()
        play_Game()

play_Game()
view.close()

Please follow PEP8, the Python style guide.

This function returns either True or None:

def check_Lights():
    if button.title == 'off':
        if button2.title == 'off':
            if button3.title == 'off':
                if button4. title == 'off':
                    return True

Make it a proper boolean. And, instead of the arrow shaped writing style, you can rewrite this as a single boolean expression, for example:

return button1.title == 'off' and button2.title == 'off' and ...

As @Caridorc pointed out in a comment, you can do even better:

return all(b.title == 'off' for b in (button, button2, button3, button4))

Don't repeat yourself. The way you create and add new buttons is very repetitive. Create a utility function to reduce the duplication, for example:

def add_new_button(wcoeff, hcoeff):
    button = ui.Button(title = 'off')
    button.center = (view.width * wcoeff, view.height * hcoeff)
    button.flex = 'LRTB'
    button.action = button_tapped
    view.add_subview(button)
    return button


Python Solution for Project Euler #2 (Fibonacci Sums) I'm a fairly new programmer (just started yesterday!). I decided to tackle Project Euler #2 today, as I did #1 yesterday without many problems. I came up with what seems to me to be a working solution, but I feel like I did it in an exceedingly ugly way. Could anyone suggest improvements to my code and/or logic?

Each new term in the Fibonacci sequence is generated by adding the previous two terms. By starting with 1 and 2, the first 10 terms will be:
1, 2, 3, 5, 8, 13, 21, 34, 55, 89, ...
By considering the terms in the Fibonacci sequence whose values do not exceed four million, find the sum of the even-valued terms.

fib = 1
fib2 = 2
temp = 0
total = 0

while temp <=4000000:
    temp = fib2
    if temp % 2 == 0:
        total += temp
    temp = fib + fib2
    fib = fib2
    fib2 = temp

print total

I just sort of used my temp variable as a holding ground for the results, and then juggled some variables around to make it do what I wanted... Seems like quite a mess. But it does result in 4613732, which seems to be correct!

I would use separately Fibonacci generator

def even_fib(limit):
    a, b = 0, 1
    while a < limit:
        if not a % 2:
            yield a
        a, b = b, a + b

And sum function to calculate sum of items

print sum(even_fib(4000000))


Hackerrank's Merging Communities Challenge can be found here

  People connect with each other in a social network. A connection
  between Person I and Person J is represented as M I J. When two persons
  belonging to different communities connect, the net effect is the
  merger of both communities which I and J belongs to.

  At the beginning, there are N people representing N communities. Suppose
  person 1 and 2 connected and later 2 and 3 connected, then 1,2, and 3 will
  belong to the same community.

  There are two type of queries:

  M I J => communities containing person I and J merged (if they belong to
  different communities).

  Q I => print the size of the community to which person  belongs.

  Input Format

  The first line of input will contain integers N and Q, i.e. the number
  of people and the number of queries. The next Q lines will contain the
  queries.

  Constraints :

  1 <= N <= 100000

  1 <= Q <= 200000

My code times out for 6 / 9 test-cases, so obviously this can be improved a lot.

Any suggestions would be much appreciated.

import java.io.*;
import java.util.*;

class Solution {
    public static void main(String[] args) {
        Scanner s = new Scanner(System.in);
        int N = s.nextInt();
        int Q = s.nextInt();
        DisjointSet ds = new DisjointSet();

        for(int i = 1; i <= N; i++) {
            ds.makeSet(i);
        }

        for(int i = 0; i < Q; i++) {
            int command = s.next().charAt(0);
            if(command == 'Q') {
                System.out.println(ds.getSetSize(s.nextInt()));
            } else {
                ds.union(s.nextInt(), s.nextInt());
            }
        }
    }
}

class DisjointSet {
    private List<Map<Integer, Set<Integer>>> disjointSet;

    DisjointSet() {
        disjointSet = new ArrayList<Map<Integer, Set<Integer>>>();
    }

    public void makeSet(int element) {
        Map<Integer, Set<Integer>> map = new HashMap<>();
        Set<Integer> set = new HashSet<>();
        set.add(element);
        map.put(element, set);
        disjointSet.add(map);
    }

    public void union(int a, int b) {
        int first = find(a);
        int second = find(b);
        Set<Integer> firstSet = null;
        Set<Integer> secondSet = null;

        for(int i = 0; i < disjointSet.size(); i++) {
            Map<Integer, Set<Integer>> map = disjointSet.get(i);
            if(map.containsKey(first)) {
                firstSet = map.get(first);
            } else if(map.containsKey(second)) {
                secondSet = map.get(second);
            }
        }

        if(firstSet != null && secondSet != null) {
            firstSet.addAll(secondSet);
        }

        for(int i = 0; i < disjointSet.size(); i++) {
            Map<Integer, Set<Integer>> map = disjointSet.get(i);
            if (map.containsKey(first)) {
                map.put(first, firstSet);
            } else if (map.containsKey(second)) {
                map.remove(second);
                disjointSet.remove(i);
            }
        }
    }

    public int find(int n) {
        for(int i = 0; i < disjointSet.size(); i++) {
            Map<Integer, Set<Integer>> map = disjointSet.get(i);
            Set<Integer> keySet = map.keySet();
            for(Integer key : keySet) {
                Set<Integer> set = map.get(key);
                if(set.contains(n)) {
                    return key;
                }
            }
        }
        return -1;
    }

    public int getSetSize(int n) {
        for(int i = 0; i < disjointSet.size(); i++) {
            Map<Integer, Set<Integer>> map = disjointSet.get(i);
            Set<Integer> keySet = map.keySet();
            for(Integer key : keySet) {
                Set<Integer> set = map.get(key);
                if(set.contains(n)) {
                    return set.size();
                }
            }
        }
        return -1;
    }
}

Simplifying

        for(int i = 0; i < disjointSet.size(); i++) {
            Map<Integer, Set<Integer>> map = disjointSet.get(i);
            if(map.containsKey(first)) {
                firstSet = map.get(first);
            } else if(map.containsKey(second)) {
                secondSet = map.get(second);
            }
        }

Consider instead

        for (Map<Integer, Set<Integer>> map : disjointSet) {
            if (firstSet == null) {
                firstSet = map.get(first);
            }

            if (secondSet == null) {
                secondSet = map.get(second);
            }

            if (firstSet != null && secondSet != null) {
                break;
            }
        }

You don't need i.  Java can iterate over a collection directly, which saves us a line of code.

You don't need to check containsKey before doing get.  If containsKey is false, then get will return null.

You might as well stop as soon as both sets are found.  You don't need to keep going.

A linear scan over all the values is something like \$\mathcal{O}(N \cdot Q)\$, assuming the number of merges is proportional to the number of queries.  Because you have to do one linear scan for each merge.  However, there is an alternative solution that is closer to \$\mathcal{O}(Q)\$.

Alternative data structure

        DisjointSet ds = new DisjointSet();

Consider

        DisjointSet ds = new DisjointSet(N + 1);

Now you can change from

    private List<Map<Integer, Set<Integer>>> disjointSet;

to something fixed size.  The normal representation for a disjoint set is a parent pointer tree with ranks added.  In this case, we need to track the sizes and can use those to represent the rank.  Consider

public class DisjointSet {

    private int[] parents;
    private int[] sizes;

    DisjointSet(int N) {
        parents = new int[N];
        sizes = new int[N];
    }

    public void makeSet(int i) {
        parents[i] = i;
        sizes[i] = 1;
    }

    public void union(int a, int b) {
        int first = find(a);
        int second = find(b);

        // if already part of the same set, no need to union
        if (first == second) {
            return;
        }

        if (sizes[first] < sizes[second]) {
            parents[first] = second;
            sizes[second] += sizes[first];
        } else {
            parents[second] = first;
            sizes[first] += sizes[second];
        }
    }

    public int find(int i) {
        // if not the root
        if (parents[i] != i) {
            // Make the parent the root, so that it will recurse at most once
            // on subsequent calls.
            parents[i] = find(parents[i]);
        }

        // Return the root (as the parent is always the root by this point).
        return parents[i];
    }

    public int getSetSize(int i) {
        return sizes[find(parents[i])];
    }

}

The parents array stores the immediate parent of any element.  If an element is alone in a set, it is its own parent.  Root nodes (terminators) are also their own parents.

For a root node, the sizes array holds the size of the set.  For other nodes, it holds junk data.

This uses both union-by-rank and path compression for an optimal runtime.  It's also reasonably efficient in space, requiring only two integers for each element.


Parse custom configuration file in C I have to parse a custom configuration file with this structure:

# Begin with core containers.

ID          = 0 # STX container
Name        = "" # No name is necessary for a STX container.
Flags       = {STX}
Attributes  = {Status=0}

So I've written the following code in order to remove those comments (starting with #) and the inline comments. I don't know if I'm doing it the right way, it just seems (for me, not too much experience) a stupid way of doing it with many while loops.

void NextToken() {
   // Let's begin taking comments away!
   FILE* Source = fopen("defs.dat", "r");
   char* Buffer = calloc(MAXBUFLEN, sizeof(char));
   while (fgets(Buffer, MAXBUFLEN, Source)) {
    // If line starts with '#' it is a comment, so it must be ignored. If it's a newine it just has to be skipped.
    if (Buffer[0] == 35 || Buffer[0] == '\n')
        continue;
    // Now we have to delete inline comments
    int Pos = 0;
    while (Pos < strlen(Buffer)) {
        if(Buffer[Pos] == '#') {
            Pos++;
            while(Buffer[Pos] != '\n')  {
                Pos++;
                continue;
            }
            continue;
        }
        //Cool! now just let's print the output.
        printf("%c", Buffer[Pos++]);
    }
   }
}

The max size of the buffer is 256 characters.

You do not need calloc() with fgets(), it adds nul terminator by itself: use malloc() instead. Also check for allocation errors and, maybe, use a meaningful name for Buffer (for example line).

char* line= malloc(MAXBUFLEN, sizeof(char));
if (line == NULL) {
    // Oops...
}

Also fopen() may fail then you should also check its return value:

if (fopen(...) == NULL) {
    // Oops
}

You should move the check for a line to ignore to a separate self-explicative function, that's why you felt the need to add a comment:

if (IsBlankOrComment(line))
    continue;

Implementing IsBlankOrComment() you should avoid hard-coded constants like 35, you should add #define and use a character instead of its ASCII code:

#define COMMENT_DELIMITER '#'

bool IsBlankOrComment(char* line) {
    char* trimmedLine = TrimWhiteSpaces(line);

    return trimmedLine[0] != '\0'
        && trimmedLine[0] != COMMENT_DELIMITER;
}

For a possible implementation you can read this SO post. Do not forget to include stdbool.h if you're using bool (instead of _Bool).

You now have to remove in-line comments, what's better than strchr()? No need to manually loop through the string:

char* inlineCommentStart = strchr(line, COMMENT_DELIMITER);
if (inlineCommentStart != NULL) {
    *inlineCommentStart = '\0';
}

Note that it also deals with line with leading and trailing spaces, if you do not need it then you definitely should go with Edward's lovely implementation (which uses strpbrk()).

To print the line (without comment) you then just need:

puts(line);

No need to print character-by-character while searching the delimiter (BTW do not use printf() when what you need to print is just a single character!) Also note that calling strlen() in a loop is (probably) extremely inefficient because, if compile can't determine that string is constant in the loop, it will emit code to call strlen() for each iteration.

Do not forget to close the file when you're finished:

fclose(stream);

And to free allocated memory:

free(line);

In this case you should ask yourself IF you need dynamically allocated memory. If MAXBUFSIZE is a preprocessor constant then you can directly declare a local variable (check also for stack size issues):

char line[MAXBUFSIZE];

If your application isn't multi-threaded and you call this function more than once you may want to also consider to make it static. Finally do not forget to add const here and there, where appropriate.


Can this JavaScript singleton be improved? I needed a singleton in JavaScript in order to make a DataManager which is called multiple times, but I just want it to load its data only the first time it is called, then give pieces of the data out view public methods, never loading the initial data again.

I found a number of approaches to building singletons in JavaScript but this one seems to have everything I need.

Does anyone see any kind of problem this singleton is going to have for the uses I need it for?

<!DOCTYPE html>
<html>
    <head>
        <title>test singleton</title>
        <script type="text/javascript">
            var DatapodManager = (function() {
                var instantiated;
                that = this;

                //DEFINE INTERNAL VARIABLES HERE:
                var message = '';

                function init() {

                    //LOAD DATA ONCE HERE:
                    that.message = 'singleton message defined at: ' + getMilliseconds();

                    //ALLOW PUBLIC ACCESS TO SINGLETON DATA HERE:
                    return {
                        getMessage : function() {
                            return that.message;
                        },
                        id : '1234'
                    }
                }

                return {
                    getInstance : function() {
                        if(!instantiated) {
                            instantiated = init();
                        }
                        return instantiated;
                    }
                }
            })()

            function dowait() {
                for( x = 1; x <= 10000000; x++) {
                    var y = 1000;
                    var z = 2000;
                    var b = y * z;
                }
            }

            function getMilliseconds() {
                var d = new Date();
                return d.getMilliseconds();
            }

            window.onload = function() {
                console.log('getting first singleton message at: '+getMilliseconds());
                console.log(DatapodManager.getInstance().getMessage());
                dowait();
                console.log('getting second singleton message at: '+getMilliseconds());
                console.log(DatapodManager.getInstance().getMessage());
                console.log('the static id is '+DatapodManager.getInstance().id);
            };
        </script>
    </head>
    <body></body>

Output:

Once you start to chain properties or function calls, you incur performance penalty due to scope differences. It's best you keep the property/function depth to a minimum. This ain't Java or AS3 that needs deep namespacing.
What's the idea of getInstance()? Can't the singleton module itself be the instance?
Regarding the output image, it is correct. You get the first message at 440ms, then the singleton created the message at 442ms during init(). The next call you made is at 482ms. Since the instance was already created, the same message (the one created at 442ms) was used.

Here's a very short version of it. Basically, I just removed getInstance() and made DatapodManager the instance. No visible speed difference but you have shorter code. I also preserved the idea of it being a closure in case you ever needed private properties:

var DatapodManager = (function () {
    return {
        message: 'singleton message defined at: ' + getMilliseconds(),
        getMessage: function () {
            return this.message
        },
        id: '1234'
    }
}());


using  in generic delegates? Here is my problem.  Do I need to give a new identifier to each delegate I write of the following delegate type? like so: or could i use one delegate that accounts for any Datatype I need to use so i don't have to keep repeating the code?

Something that may or may not be relevant: I'm calling these functions to prompt user's input for new fields to add for records in a database.  So database field types are the data types I'm dealing with, but I'm using this approach for learning purposes to try and minimize redundancy.

public static class stcHelper  // helper class
{
    public delegate T DelReturnType<T>();
    static public DelReturnType<int> UserInt = () =>
    {
        Console.Write("Enter integer: ");
        return int.Parse(Console.ReadLine());
    };
    static public DelReturnType<string> UserString = () =>
    {
        Console.Write("Enter string: ");
        return Console.ReadLine();
    };
    static public DelReturnType<AnotherDataType> UserAnotherDataType = () =>
    {
        Console.Write("Enter AnotherDataType: ");
        return SomeKindOfConversionFunction(Console.ReadLine());
    };
    // and so on for what could be 15 other data types ..
}

or is there a better way to reduce redundant code?

The code below is also part of the same class.

public static class stcHelper
{
    static public T InsistValidInput<T>(DelTypeReturn<T> MyInputAction)
    {
        bool bSuccess = true;
        do
        {
            try
            {
                return MyInputAction();
            }
            catch (Exception ex)
            {
                Console.WriteLine("Invalid input!" + ex.ToString());
                bSuccess = false;
            }
        } while (!bSuccess);
        return default(T);
    }
}

If done cleverly, you can get away with a single method to get the input of any type:

static T GetInput<T>(string message, Converter<string, T> transform)
{
    Console.WriteLine(message);
    return transform(Console.ReadLine());
}

All you need to do is pass in a transformation function (the Converter<TInput,TOutput> delegate is equivalent to Func<T,TResult>):

int parsedInt = GetInput("Type in a number", int.Parse);
float parsedFloat = GetInput("Type in a float", float.Parse);

And ta-da, you've got your input, parsed as the desired type.

But as you also noted in the second half of your question, it's dangerous to assume that the input will always be valid. There's a universal version of your insist-on-valid-input function too. However, you should avoid using exceptions for normal control flow. Use the xxx.TryParse methods instead.

First, define the following delegate that matches the TryParse method defined in all the primitive types:

delegate bool TryParse<T>(string input, out T output);

Then, you can implement the loop:

static T InsistOnValidInput<T>(string message, string errorMessage, TryParse<T> validator)
{
    Console.WriteLine(message);
    T result;
    while (!validator(Console.ReadLine(), out result))
    {
        Console.WriteLine(errorMessage);
    }
    return result;
}

And call it:

bool parsedBool = InsistOnValidInput<bool>("Type in a bool", "Try again", bool.TryParse);

Unfortunately, C# doesn't infer the type parameter of the output argument, so you need to specify it manually (in this case, <bool>).


Optimizing a python script with large arrays I have a python script that I'd like to speed up. I'm hoping some of the gurus here can offer some advice.

The script reads in some data from file: 2 arrays with lat/lon vals, 3 arrays with image data and one quality mask. Each array is typically around 22M datapoints.
An output array exists and I convert the lat/lon values into array subscripts, mask the image data and then put into the correct locations in the output array.

I've got a feeling that my current method, whilst successful, is far from optimized!

mainarr =   np.zeros((4,1800,3600))
#Loop over all input files
for infile in files:

    #Code for reading data goes here. Limited by disk I/O so no optimisation here.
    #Location arrays are: lats,lons
    #Image data arrays are: c550,c670,c870
    #Quality mask is: cldy
    #Each of the above is a 1d numpy array, all are the same size
    #Function for reading the input data is not available, have called it 'readdata()' here.
    #lats, lons, c550, c670, c870 are all 32 bit floats. cldy is 8bit int.

    lats    =   np.array(readdata()).ravel()
    lons    =   np.array(readdata()).ravel()
    c550    =   np.array(readdata()).ravel()
    c670    =   np.array(readdata()).ravel()
    c870    =   np.array(readdata()).ravel()
    cldy    =   np.array(readdata(),dtype=np.int8).ravel()

    lats        =   np.array(np.round((lats+90)*10),dtype=np.int16)
    lons        =   np.array(np.round((lons+180)*10),dtype=np.int16)

    lats[(lats>=1800).nonzero()]=1799
    lons[(lons>=3600).nonzero()]=3599

    #The function below is already optimized
    masker  =   getmask(cldy, 1, 1, 0)
    pts     =   (masker != 1).nonzero()
    lats[pts]   =   0
    lons[pts]   =   0

    mainarr[0,lats,lons]    =   mainarr[0,lats,lons]+c550
    mainarr[1,lats,lons]    =   mainarr[1,lats,lons]+c670
    mainarr[2,lats,lons]    =   mainarr[2,lats,lons]+c870
    mainarr[3,lats,lons]    =   mainarr[3,lats,lons]+1

    i       =   i+1

You can get minor performance improvements if you use the += synthax of numpy, e.g.

mainarr[0, lats, lons] = mainarr[0, lats, lons] + c550

would reduce to

mainarr[0, lats, lons] += c550

Though it is not really in-place, you still gain some speed.

Another idea I had, was to use numpy.clip as it might be faster than setting the upper bound for your indices manually. You may be able to gain further improvements using numpy.put with mode=clip instead of really clipping the values. Positive effects for numpy.take (which is the opposite of numpy.put) are described here.

I put up a little program to test the gain compared to your proposed code. As you did not provide much additional information, I had to do some assumptions, e.g. for the way your data looks like (see mock_data).

The result is something like the following

Experiment (10 files, 10000 points)
Original: 10.093s   Refactored: 9.964s

Experiment (10 files, 1000 points)
Original: 2.399s   Refactored: 2.362s

Experiment (10 files, 100 points)
Original: 0.349s   Refactored: 0.342s

Experiment (100 files, 10000 points)
Original: 47.823s   Refactored: 47.672s

Experiment (100 files, 1000 points)
Original: 10.888s   Refactored: 10.781s

Experiment (100 files, 100 points)
Original: 3.306s   Refactored: 3.195s

Experiment (1000 files, 10000 points)
Original: 423.727s   Refactored: 420.922s

Experiment (1000 files, 1000 points)
Original: 58.546s   Refactored: 56.579s

Experiment (1000 files, 100 points)
Original: 20.227s   Refactored: 18.260s

Conclusion

In my oppinion, you will not be able to gain a great speed-up by fiddling around with numpy here. I made a small test not included in the listing above with 1 file and 1M points

Experiment (1 files, 1000000 points)
Original: 51.245s   Refactored: 48.703s

Because of this I think numpy is not really the problem in your case. If your mask hits a lot of points, it might be a good idea to remove the corresponding indices from lats and lons with numpy.delete, because both lists will get shorter and mainarr[0][0] will not be messed up with all the values you put there.

As the operations on the files do not really depend on the other files, one could come up with the idea to split up the process and merge their results later. Your task would be to re-implement your function, so that you can run it in parallel in different processes for parts of your file list. But even this might not help you that much, if your I/O capacity is the bottleneck.

Test script

import numpy as np
import timeit

def mock_data(n_samples=1000):
    """Mock data generation"""
    lats = np.random.uniform(-90.0, 90.0, (n_samples, ))
    lons = np.random.uniform(-180.0, 180.0, (n_samples, ))
    c550 = np.random.rand(n_samples)
    c670 = np.random.rand(n_samples)
    c870 = np.random.rand(n_samples)
    cldy = np.random.rand(n_samples)

    return lats, lons, c550, c670, c870, cldy

def mock_files(n_files=500, n_data=1000):
    """Mock files for benchmarking"""
    np.random.seed(42)
    return [mock_data(n_data) for _ in range(n_files)]

def getmask(cldy, tresh, max_val, min_val):
    """Mock getmask"""
    return cldy > (tresh / 2.0)

def original(files):
    """Original function by @Simon"""
    mainarr = np.zeros((4, 1800, 3600))
    i = 0

    #Loop over all input files
    for infile in files:
        lats, lons, c550, c670, c870, cldy = infile

        lats = np.array(np.round((lats+90)*10), dtype=np.int16)
        lons = np.array(np.round((lons+180)*10), dtype=np.int16)

        lats[(lats>=1800).nonzero()]=1799
        lons[(lons>=3600).nonzero()]=3599

        #The function below is already optimized
        masker = getmask(cldy, 1.0, 1.0, 0.0)
        pts = (masker != 1).nonzero()
        lats[pts] = 0
        lons[pts] = 0

        mainarr[0, lats, lons] = mainarr[0, lats, lons]+c550
        mainarr[1, lats, lons] = mainarr[1, lats, lons]+c670
        mainarr[2, lats, lons] = mainarr[2, lats, lons]+c870
        mainarr[3, lats, lons] = mainarr[3, lats, lons]+1

        i = i+1

    return mainarr

def refactored(files):
    """refactored function by @Alex Vorndran"""
    mainarr = np.zeros((4, 1800, 3600))

    #Loop over all input files
    for i, infile in enumerate(files):
        lats, lons, c550, c670, c870, cldy = infile

        lats = np.array(np.round((lats+90)*10), dtype=np.int16)
        lons = np.array(np.round((lons+180)*10), dtype=np.int16)

        np.clip(lats, 0, 1799, out=lats)
        np.clip(lons, 0, 3599, out=lons)

        #The function below is already optimized
        masker = getmask(cldy, 1.0, 1.0, 0.0)
        pts = (masker != 1).nonzero()
        lats[pts] = 0
        lons[pts] = 0

        # make use of "in-place" additions
        mainarr[0, lats, lons] += c550
        mainarr[1, lats, lons] += c670
        mainarr[2, lats, lons] += c870
        mainarr[3, lats, lons] += 1.0

    return mainarr

def validate_refactoring(n_files, n_data):
    """Validate the refactoring with comparison by value"""
    files = mock_files(n_files, n_data)
    mainarr_o = original(files)
    mainarr_r = refactored(files)
    return np.allclose(mainarr_o, mainarr_r)

def test_functions():
    """Test different versions"""
    n_runs = 100
    n_files_all = (10, 100, 1000)
    n_data_all = (100, 1000, 10000)

    # a list of tuples where the first element is the number of files
    # and the second one is the number of elements
    files_and_points = [(  10, 10000), (  10, 1000), (  10, 100),
                        ( 100, 10000), ( 100, 1000), ( 100, 100),
                        (1000, 10000), (1000, 1000), (1000, 100)]

    assert validate_refactoring(10, 100)

    imports = 'from __main__ import original, refactored, mock_files;'

    for n_files, n_data in files_and_points:
        data = 'files = mock_files({}, {});'.format(n_files, n_data)

        time_original = timeit.timeit(
            'original(files)', setup=imports+data, number=n_runs)
        time_refactored = timeit.timeit(
            'refactored(files)', setup=imports+data, number=n_runs)

        print('Experiment ({} files, {} points)'.format(n_files, n_data))
        print('Original: {:.3f}s   Refactored: {:.3f}s\n'.format(
            time_original, time_refactored))

if __name__ == '__main__':
    test_functions()


Rendering nested array in a  with headings I have an array with the following structure

Array
(
    [0] => Array
        (
            [area] => Area ABC
            [activity] => Array
                (
                    [0] => Activity 1
                    [1] => Activity 2
                )
        )
)

I'm trying to render this array in the following way

Area ABC

Activity 1
Activity 2

And am currently using the following code to achieve this

$status = 'closed';
foreach($suggestions as $suggestion)
    {
    if($currentarea != $suggestion['area'])
        {
        echo "
";
        echo $suggestion['area'];
        echo "
";
        if($status == 'closed')
            {
            echo "
";
            $status = 'open';
            }
        }
    foreach($suggestion['activity'] as $activity)
        {
            echo "
";
            echo $activity;
            echo "
";
        }
    if($status == 'open')
        {
            echo "
";
            $status = 'closed';
        }
    $currentarea = $suggestion['area'];
    }

Is this the best way of doing it, or is there a better way, I will have more areas and activities, but the structure stays the same.

This won't work if each suggestion does not start a new area.

foreach($suggestions as $suggestion)
    {
    if($currentarea != $suggestion['area'])
        {
        echo "
";
        echo $suggestion['area'];
        echo "
";
        if($status == 'closed')
            {
            echo "
";
            $status = 'open';
            }
        }
    foreach($suggestion['activity'] as $activity)
        {
            echo "
";
            echo $activity;
            echo "
";
        }
    if($status == 'open')
            {
            echo "
";
            $status = 'closed';
            }
    $currentarea = $suggestion['area'];
    }

You close on every iteration that it's open and add list items on every iteration, but you only open if the area changes.

foreach($suggestions as $suggestion)
{
    echo '
';
    echo $suggestion['area'];
    echo "
\n";
    echo "
\n";

    foreach($suggestion['activity'] as $activity)
    {
        echo '
';
        echo $activity;
        echo "
\n";
    }

    echo "
\n";
}

This code is simpler and will work more reliably.

This is the wrong place to discuss how to write new code.  But if you want to get the previous concept working, you'll want to open and close inside the if that checks if the area has changed.  Or pull off the first element of the list so that you can treat it differently.  Or make a new list where there's only one suggestion per area, combining multiple suggestions with the same area into one.


Search engine scraper-aggregator For some reason, I completely lost my reasoning with this code. I can't hold this simple logic for too long in my head. Actually, I want to improve this code, making it a little concise. The if (so.scan_complete and force) or not so.scan_complete: is just not a KISS thing. Please any suggesion to optimize and concise it?

source = request.POST.get('source')
queries = request.POST.get('term')

force = request.POST.get('rescan', False) # if checkbox is checked this becomes True

queries = [query.strip().lower() for query in queries.split(',')]
engines = ['alibaba', 'madeinchina', 'google', 'bing']
for src in engines:
    for query in queries:
        so = Source.objects.get(term__term=query, engine=src)
        if src == 'alibaba':
            if (so.scan_complete and force) or not so.scan_complete:
                ali_results.delay(src, query)
        elif src == 'madeinchina':
            if (so.scan_complete and force) or not so.scan_complete:
                made_results.delay(src, query)
        elif src == "google" or src == "bing":
            if (so.scan_complete and force) or not so.scan_complete:
                search_engine_scraper.delay(query=query, source=src)

The test if (so.scan_complete and force) or not so.scan_complete: only fails when so.scan_complete is True and force is False. So it can be simplified to if force or not so.scan_complete:.

Also, since the test is identical for every src, it only needs to be done once:

for src in engines:
    for query in queries:
        so = Source.objects.get(term__term=query, engine=src)
        if force or not so.scan_complete:
            if src == 'alibaba':
                ali_results.delay(src, query)
            elif src == 'madeinchina':
                made_results.delay(src, query)
            elif src == "google" or src == "bing":
                search_engine_scraper.delay(query=query, source=src)


Simple comment system This is a comment system that I wrote, is it secure?

<!doctype html>

<meta charset="utf-8">
<link rel="stylesheet" type="text/css" href="style.css">
<?PHP

//Turn off error reporting (Not Necessary)
    error_reporting(E_ALL ^ E_NOTICE);

//Connect to the database
    if (!@mysql_connect('localhost', 'user', '') or !@mysql_select_db('comments')) {
        die('Could not connect, please check back later.');
    }

//Variables
    $guest_name = htmlentities(str_replace(' ', '',$_POST['guest_name']));
    $comment = htmlentities($_POST['comment']);
    $time = date('g:i A', time());
    $date = date('n/j/Y');
    $query = "INSERT INTO `user_comments` VALUES(
        '".mysql_real_escape_string('')."',
        '".mysql_real_escape_string($guest_name)."',
        '".mysql_real_escape_string($comment)."',
        '".mysql_real_escape_string($date)."',
        '".mysql_real_escape_string($time)."'
        )";
    $array_query = mysql_query("SELECT * FROM `comments` ORDER BY `ID` DESC");

//Insert comment into the database
    if (isset($_POST['submit_comment']) && !empty($comment) && !empty($guest_name)) {
        if(mysql_query($query)) {
            header('Location: index.php');
        }
    }

//Check if user has filled in all fields
    if (isset($_POST['submit_comment']) && empty($guest_name)) {
        echo '*Please enter a Guest Name*
';
    }

    if (isset($_POST['submit_comment']) && empty($comment)) {
        echo '*Please enter a Comment*';
    }

//List comments
    function list_comments () {
        $query = "SELECT * FROM `user_comments` ORDER BY `ID` DESC";
        $query_run = mysql_query($query);
        while ($result = mysql_fetch_array($query_run)) {
            echo '
' . $result['Guest Name'] . ' - ' . $result['Date'] . ' - ' . $result['Time'] . '
' . $result['Comment'] . '

';
        }
    };

?>
<title>Main_Page</title>

<form method="post" action="index.php">

    <input type="text" name="guest_name" maxlength="15" placeholder="Guest Name">

    <textarea cols="70" rows="5" name="comment" maxlength="512" placeholder="Comment"></textarea>
    <input type="submit" name="submit_comment">

</form>

Comments

<?PHP

    list_comments();

?>

<?PHP

Normally it's all lower case, just saying.

//Turn off error reporting (Not Necessary)
error_reporting(E_ALL ^ E_NOTICE);

This is one prime example of confusing and not helpful commenting. If this call is not necessary, why is it there at all?

if (!@mysql_connect('localhost', 'user', '') or !@mysql_select_db('comments')) {

Stop using the mysql_* functions, seriously.

$guest_name = htmlentities(str_replace(' ', '',$_POST['guest_name']));

Why are you replacing entities here? You should not replace stuff when saving it into your database. Ideally the data in your database would be as unbiased as possible. Only replace entities if you need to, f.e. you display it on a webpage.

$time = date('g:i A', time());
$date = date('n/j/Y');
$query = "INSERT INTO `user_comments` VALUES(
    '".mysql_real_escape_string('')."',
    '".mysql_real_escape_string($guest_name)."',
    '".mysql_real_escape_string($comment)."',
    '".mysql_real_escape_string($date)."',
    '".mysql_real_escape_string($time)."'
    )";

From this query I can tell you a few things:

Your database layout is sub-optimal, it uses VARCHAR fields if it should use a DATETIME field.
You like to copy and paste code, that's bad.
You don't do things explicitly, consider adding a fields list to your queries.

First, you should be using a DATETIME type if you're gonna save date and time. MySQL supports also a 'pure' DATE and TIME format, there's absolutely no reason and no excuse to use use a VARCHAR field for that.

And then there's your missing fields list, I made a habit out of adding a fields list to my queries for two reasons:

It let's the reader know exactly what is getting set to where.
It makes your code more robust, the query will not fail if you add a field. Removing a field is also easier, as a simple grep for the field will yield all queries that use it.

So, your query should look like this:

$query = "
    INSERT INTO
        user_comments
    SET
        name = :name,
        comment = :comment;
";

It's easy to read and you know exactly what ends where. Additionally the date/time stuff is here completely missing, because I changed the database layout:

CREATE TABLE user_comments
    name VARCHAR,
    comment VARCHAR,
    timestamp TIMESTAMP DEFAULT CURRENT_TIMESTAMP;

The TIMESTAMP type is optimal for this kind of operation, as it is able to save automatically the time when the row is created.

$array_query = mysql_query("SELECT * FROM `comments` ORDER BY `ID` DESC");

Same here, your usage of the * makes it easily broken by adding new fields. Additionally you don't know what fields are fetched until you look at the table declaration.

header('Location: index.php');

I'm 99% sure that this should not work. Headers can not be changed after data was send, you set the header in the middle of the file, but your php script should be moved to the top of the file. Additionally if you want to redirect people, it would be good to die() afterwards to make sure that your script stops executing at this point:

<?php
    if (condition) {
        header("Location: othersite.php");
        die("Redirect: <full address here>");
    }
?>

    ...

This will also allow everyone who is not automatically redirected to see where to go to now.

echo '*Please enter a Guest Name*
';

You're outputting stuff while inside the , that's bad practice, content should go into the .

What is this?!

And just to make sure:

Stop using the mysql_* functions and start using prepared queries!


Data types

The number of consecutive digits you can support is limited not by the length of the input, but by the maximum value that can be stored in a uint64_t.  While 913 is comfortably smaller than 264 - 1, you should be cautious about overgeneralizing your claims.

It's weird that you initialize uint64_t integers with floating-point literals 1.0 and 0.0.

Loop and conditional style

Never omit the optional braces in conditional and loop blocks like that.  By doing so, you are being a contributing factor to a future coding accident.  If you want to omit the braces, then put the statement on the same line so that it cannot be misinterpreted.  If you feel that braces take up too much vertical space, then please switch to the K&R brace style so that you don't feel compelled to omit them.

Your while-loops fit the classic for-loop pattern, and should be written as for-loops to make them easy for other programmers to recognize.

uint64_t max = 0;
int data[DIGITNUM];
for (int i = DIGITNUM - 1; number[i] != '\0'; ++i)
{
    for (int k = 0; k < DIGITNUM; ++k)
    {
        data[k] = char2int(number[i - k]);
    }

    uint64_t product = getProduct(data);
    if (product > max) max = product;
}

Algorithm

You transfer almost every digit to the temporary data array 13 times.  It would be smarter to treat the data array as a circular-buffer.

uint64_t max = 0;
int consec[DIGITNUM];
for (int n = 0, c = 0; number[n] != '\0'; ++n, ++c) {
    if (c == DIGITNUM) {
        c = 0;        // Circular buffer wraparound
    }
    consec[c] = char2int(number[n]);
    if (n < DIGITNUM - 1) {
        continue;     // Circular buffer not fully initialized yet
    }

    uint64_t product = getProduct(consec);
    if (product > max) {
        max = product;
    }
}

Here, I've used n and c as array indices, which are mnemonics for the respective arrays number and consec that they index into.


Linked list remove() and free() How can I improve my remove function?  Could someone also give me a solid explanation of the free() function?

Here is my remove function:

void removeData(void *data, struct accList *theList)
{
  if(theList->head == NULL)                  //nothing can be deleted
    return;
  else if(theList->head == theList->tail)          //there is only one element in the    list
  {
    free(theList->head);
    theList->head = theList->tail = NULL;
  }
  else if(data == theList->head->data)           //the node to be deleted is the head
  {
    struct accListNode *temp = theList->head;
    free(theList->head);
    theList->head = temp;
    theList->head->next = temp->next;
  }
  else if(data == theList->tail->data)      //the node to be deleted is the tail
  {
    struct accListNode *cur;
    for(cur = theList->head; cur->next->next != NULL; cur = cur->next);
    theList->tail = cur;
    free(cur->next);
    cur->next = NULL;
  }
  else                                     //the node to be deleted is any other node
  {
    struct accListNode *cur;
    for(cur = theList->head; cur != NULL; cur = cur->next)
    {
      if(cur->data == data)     //this is the node we must delete from theList
      {
        struct accListNode *temp = cur->next->next;
        free(cur->next);
        cur->next = temp;
        break;
      }
    }
  }
}

First, about free(). If you need memory, you call malloc() and it will give you a pointer that points to a block of memory that is yours now. The OS will (hopefully) take care that no one else will use this memory. But, as with all resources, be it sockets, open files or threads, you will have to relinquish this memory at one time. At the latest, when your application exits. Better, before that, ideally the moment you don't need it anymore. And that's what free() is for. That pointer you got from malloc()? Give it to free() and you're telling the OS that you're done with this particular block of memory. This will allow other programs to reutilize the memory and keeps your memory footprint small. However, that means that you shouldn't use this memory yourself again. The mean thing is that using the memory anyway might not crash your application right away. That's why it's always a good idea to set a pointer to NULL right after freeing it. That way, if you try to use this pointer again, it will crash directly, making it more obvious where the error is.

If you don't free memory you allocated, it's a memory leak. Especially if this happens periodically, it can be a real problem, because the longer your program runs, the more memory it consumes. Memory it doesn't use, but other programs might need. So always free your memory. Use tools like valgrind to check for leaks and other problems.

Now, to your function. I spotted one little error:

struct accListNode *temp = theList->head;
free(theList->head);
theList->head = temp; // Does nothing
theList->head->next = temp->next; // Potential crash

I think the first line should read

struct accListNode *temp = theList->head->next;

Otherwise, the third line does nothing (it's just the first line in reverse) and the fourth will sooner or later lead to a crash because you're accessing free'd memory.

Then, there's the second case, where there's only one element in the list. Are you sure you want to remove that element without first checking if it's actually the data that was requested to be removed?

And a small optimization in the last case:

for(cur = theList->head->next; cur != theList->tail; cur = cur->next)

You've already established that the data to remove isn't in the tail or head, so you don't need to check them again.
The rest is just stylistic:

if(theList->head == NULL)
    return;

Even if it's only one line, it's good practice to put braces around it:

if(theList->head == NULL) {
    return;
}

Also, you use struct accList a lot. You can make a typedef like so:

typedef struct accList accList

It looks a little weird, but after that, you can replace all your struct accList with just accList. To better distinguish it from a variable name, I also recommend renaming it to AccList.


Implementing a queue using three stacks This is just for fun so you will see tons of print statements for trouble shooting. I was hoping on slimming this down some and gain new ideas on what I have. This was a challenge question in a class I'm not in. I know the code i wrote is nightmare-ish anything to slim it down would be nice.

import java.util.*;

public class StackQueue {

    Stack stack1 = new Stack();
    Stack stack2 = new Stack();
    Stack temp   = new Stack();

    public void enqueue(String element) {
        stack1.push(element);
        System.out.println(stack1.toString() + "Stack1");
    }

        public void dequeue(){
            System.out.println(stack1.toString() + "\n\nStack1");

            if(!stack1.empty() & stack1.size()> 1 & !stack2.empty()) {           // if there are elements in both stacks
                System.out.println("IM IN THE BLOCK");
                while (!stack2.empty()) {
                    temp.push(stack2.pop());
                    System.out.println(temp.toString() + " TEMPP----");
                    System.out.println(stack2.toString() + " STACK 2---");
                }
                while (!temp.empty()) {
                    stack1.push(temp.pop());
                    System.out.println(stack1.toString() + " STACK1 ----");
                }
                System.out.println(stack1.pop() + " Has been removed");
                System.out.println(stack1.toString() + "STACK 1----");
                System.out.println(stack2.toString()+ "STACK 2----");

                if(stack2.empty()){
                    while (!stack1.empty()) {
                        temp.push(stack1.pop());
                        System.out.println(temp.toString() + " temp ----");
                    }
                    System.out.println(stack1.addAll(temp));
                    System.out.println(stack1.toString()+ "STACK ! ADD");
                    temp.removeAllElements();
                    System.out.println(temp.toString() + "TEMP _++++_+_+_+_");
                }
                }

           else if(stack1.empty()){

                System.out.println("\n\nstack1 is empty ");
                System.out.println(stack2.pop() + " Has been removed");
            }else {
                System.out.println("\n\n");
                while (!stack1.empty()) {
                    System.out.println(stack2.toString() + "Stack2 - second part");
                    stack2.push(stack1.pop());

                }
            System.out.println(stack1.toString()+ "stack1");
            System.out.println(stack2.toString()+"stack2");
            System.out.println(stack2.pop() + " Has been removed");
                    }

        }
    }

public class Main {
    public static void main(String[] args){
        StackQueue queue = new StackQueue();

        queue.enqueue("hey");
        queue.enqueue("I");
        queue.enqueue("am");
        queue.enqueue("Jeremiah");
        queue.enqueue("now");
        queue.dequeue(); //removes hey
        queue.dequeue(); //removes i
        queue.dequeue();// removes am
        queue.enqueue("one ");
        queue.enqueue("more");
        queue.enqueue("time");
        queue.dequeue();  // removes jeremiah

        // i had to reverse the stack1 order here
        queue.dequeue(); // removes now
        //queue.dequeue(); // removes one
        queue.enqueue("FIRST");
        queue.dequeue();//removes FIRST

        System.out.println(queue.stack1.toString()  + " END S1");
        System.out.println( queue.stack2.toString() + " END S2");
        System.out.println(queue.temp.toString() + " END temp");

    }

}

import java.util.Stack;

public class StackQueue<T> {
    private Stack<T> writeStack = new Stack<T>();
    private Stack<T> readStack = new Stack<T>();

    public void enqueue(T element) {
        writeStack.push(element);
    }

    public T dequeue(){
        if (readStack.empty()) {
            while (!writeStack.empty()) {
                readStack.push(writeStack.pop());
            }
        }
        return readStack.pop();
    }
}

Algorithm

You only need two stacks. Always write to the writeStack, read from the readStack. If the latter is empty, copy everything over.

Naming

Use meaningful names, such as writeStack and readStack instead of stack1 and stack2. Declare them private. A queue's user doesn't care about the internals. Import only what you need, in this case java.util.Stack.

Generics

The Stack<T> is a generic datatype, make use of it. Make your StackQueue generic as well. Instanciate it using

StackQueue<String> queue = new StackQueue<String>();

Return value

dequeue() should return a value. Print it with

System.out.println(queue.dequeue());


Yet another implementation of malloc() in C I'm really curious to know someone else's opinion on this implementation. Specifically, I would be interested to know the way you would have implemented it. Here the list of issues I've found with in this primitive implementation:

It's slow. You have to traverse the entire list in order to find a free block of memory.
Overuse of sbrk().
Very high possibility of internal fragmentation.

mem.h

#ifndef __MEM__
#define __MEM__
#include<stdio.h>
#include<stdlib.h>

typedef enum { false, true } bool;

typedef struct page
{
  size_t size;
  bool free;
  struct page* next;
  struct page* prev;
} page_t;

extern void* memalloc(size_t size);
extern void  memfree(void* pointer);

#endif

mem.c

#include "include/mem.h"
#include <assert.h>
#include <sys/types.h>
#include <unistd.h>

#define SBRK_ERROR (void*)(-1)
#define UNDEFINED 0

page_t* global;
page_t* last;

page_t* allocate(size_t size)
{
  page_t* node = sbrk(0);
  void* pointer = sbrk(size + sizeof(page_t));
  if(pointer == SBRK_ERROR)
    return NULL;
  if(last != NULL)
    last-> next = node;
  last = node;
  last-> size = size;
  last-> free = false;
  last-> next = NULL;
  return last;
}

void* search(size_t size)
{
  page_t* node = global;
  while(node != NULL){
    if(node-> size >= size || node-> size == UNDEFINED){
      if(node-> free)
        return node;
    }
    node = node-> next;
  }
  return NULL;
}

void* memalloc(size_t size)
{
  page_t* result = NULL;
  if(size >= 0){
    if(global != NULL){
      result = search(size);
      if(result == NULL)
        result = allocate(size);
    }else {
      global = allocate(size);
      result = global;
    }
  }
  return result != NULL ? (result + 1) : NULL;
}

page_t* to_page(void* pointer){
  return pointer - sizeof(page_t);
}

void memfree(void* pointer)
{
  if(pointer != NULL) {
    page_t* page = to_page(pointer);
    page-> size = UNDEFINED;
    page-> free = true;
  }
}

int main(int argc, char** argv)
{
  char* pointer = memalloc(5);
  memfree(pointer);
  return 1;
}

What are the most common techniques (implementations) to solve the issues above? Just rough ideas of it.

Design.

The trouble with your design is that you have to search a list. Where none of the sizes may match. Even if there is a block in the list of the correct size you have to search through all the blocks that don't match your size.

Step 1

Use a list of lists. The top level list is an ordered list of sizes. So you can loop across this list quickly looking for the size you want. And stop if it does not exist.

The second level list is a list of all blocks of a specific size. So if you find the list of 24 bytes. That list contains all the 24 byte blocks that have been freed (so you just take the first one).

When an object is used you remove it from a list and when it is free'd you add it back to the list of free blocks.

global->********* -->********* -->********* -->*********
        *   8   * |  *   16  * |  *  24   * |  *  32   *
        * next  *-|  * next  *-|  * next  *    * next  *
        *********    *********    *********    *********
            |            |            |            |
            \/           \/           \/           \/
        *********    *********    *********    *********
        * block *    * block *    * block *    * block *
        * next  *    * next  *    * next  *    * next  *
        *********    *********    *********    *********
            |            |            |            |
            \/           \/           \/           \/
        *********       null      *********    *********
        * block *                 * block *    * block *
        * next  *                 * next  *    * next  *
        *********                 *********    *********
            |                         |            |
            \/                        \/           \/
        *********                    null      *********
        * block *                              * block *
        * next  *                              * next  *
        *********                              *********
            |                                      |
            \/                                     \/
        *********                                  null
        * block *
        * next  *
        *********
            |
            \/
            null

Step 2 Simple Optimizations

If you don't find a block of the exact size you can re-use a block that is too large. There are two version of this. a) slightly too large and just waste a small amount of space b) twice as big as you need and split the block into two pieces.

Searching the size list can still be expensive but that can be simplified by using a skip list. This should significantly reduce the time you need to search the list.

Alternatively rather than a top level list you could use a balanced tree to hold the top level structure. That way finding a value is always O(ln(x)) where x is the number of different sizes.

Code Review

Your code is fine for handling string data. But you don't take into account the alignment of the object you are allocating for. If you allocate a block of 5 you don't guarantee that the object is aligned for a structure of 5 bytes (only aligned for an array of 5 characters).

Search across blocks that have already been allocated seems like a waste of time. Either simply remove allocated blocks from the global chain or if you absolutely must track them then keep two lists (one for allocated objects and one for freed objects).

There is no need to add blocks onto the end. It is far simpler to add new blocks to the front of a list.

Your code does not use the prev pointer!!!!