Implementing my own Integer.toBinaryString(int n) method Our senior developer gave us (trainees/jr. developers) a small exercise and that is to make our own Integer.toBinaryString(int n) implementation. This is the answer I came up with. I would just like to hear comments/suggestions/opinions on this. Especially, if there is a way to optimize my answer.

public static String toBinaryString(int n){

 String binary = "";

 if(n == 0) return "0";

 // I know, I'm desperate :(
 if(n == Integer.MIN_VALUE) return "10000000000000000000000000000000";

 if(n < 0){

     // Get the positive equivalent
     int val = n * -1;

     // Convert into binary
     String initial = toBinaryString(val);
     String inverted = "";

     // Get 1's complement
     for(char chars : initial.toCharArray()){
        inverted += String.valueOf(((chars == '1') ? '0' : '1'));
     }

     int carry = 0;

     /*Check least significant bit.
     If 0, simply change it to 1.
     If 1, perform addition of 0b1*/
     if(inverted.charAt(inverted.length()-1) == '1'){

         boolean carriedOver = false;

         for(char chars : new StringBuilder(inverted).reverse().toString().toCharArray()){

             if(carriedOver){
                 binary = chars + binary;
                 continue;
             }

             if(carry > 0){
                 if(chars == '1'){
                     binary = "0" + binary;
                     continue;
                 }else{
                    binary = "1" + binary;
                    carriedOver = true;
                    continue;
                 }
             }

             binary = "0" + binary;
             carry += 1;
         }
     }else{
          StringBuilder sb = new StringBuilder(inverted);
          sb.setCharAt(inverted.length()-1, '1');

          binary = sb.toString();
     }

     return String.format("%32s", binary).replace(" ", "1");
 }

 // Convert to binary
 while(n > 0){
     binary = (n & 1) + binary;
     n /= 2;
 }

 return binary;
 }

If you were our senior developer, would you accept this as a valid answer? Why? Why not?

The exercise calls for bit shifting. Only bit shifting, nothing else, really. Your main tools are:

checking if the last bit is 0 or 1 with: num & 1
then shift by one bit to the right: num >> 1

A naive implementation could go like this:
    String result = "";
    while (num > 0) {
        result = (num & 1) + result;
        num >>= 1;
    }
    return result;

But that won't work for negative numbers. A simple tweak can fix that:
    String result = "";
    while (num != 0) {
        result = (num & 1) + result;
        num >>>= 1;
    }
    return result;

Instead of the signed bit shift operator >>, we need to use the unsigned bit shift operator >>>, to shift the negative bit just like all the others. And we changed the condition to != 0 instead of > 0.
But this won't work for 0. But only for 0. So you can add a simple condition to handle that.
Lastly, string concatenation is inefficient. We can do better using a StringBuilder.
But a StringBuilder only has an append method, doesn't have prepend. It has an insert method, but that won't be efficient.
A simple solution is to append the bits and reverse at the end.
String toBinaryString(int num) {
    if (num == 0) {
        return "0";
    }

    StringBuilder builder = new StringBuilder(32);
    while (num != 0) {
        builder.append(num & 1);
        num >>>= 1;
    }
    return builder.reverse().toString();
}

In any case, the StringBuilder is not a critical piece here.
You could use a char[] with 32 elements to store the digits,
and transform that to a string to return.


Encapsulation preserving operator= overloading in C++ I have made a facility to perform operator= dispatching to a member setter function. The purpose is to enable accessing members (like using object.member = value;) while at the same time respecting encapsulation.

My implementation looks like this:

#include <iostream>

#define OPEQ_SETTER_GETTER( OwnerType, Type, Function, Name ) \
    accessor< Type, OwnerType, decltype(&OwnerType::Function) > Name = \
    accessor< Type, OwnerType, decltype(&OwnerType::Function) >(this, &OwnerType::Function); \
    friend class accessor< Type, OwnerType, decltype(&OwnerType::Function) >;

// A generic accesor class
template <typename T, class Owner, class Setter> struct accessor {
    accessor ( Owner* o, Setter &&f ): owner( o ), setter( f ) { }
    accessor &operator=( const T &value ) { setter( owner, data, value ); return *this; }
    operator T() const { return data; }
private:
    Owner *owner;
    Setter setter;
    T      data;
};

// An example of applying the idiom
class example
{
    private:
    static void setx( example *this_handle, double &data, const double &value )
    {
        std::cout << "Setting data to: " << value << '\n';
        this_handle->another_member = 85656;
        std::cout << "another_member is: " << this_handle->another_member << '\n';
        data = value;
    }

    std::size_t another_member = 0;

    public:
    example( )  {}

    OPEQ_SETTER_GETTER( example, double, setx, x );
    // Will expand to:
    //accessor< double, example, decltype(&example::setx) > x =
    //      accessor< double, example, decltype(&example::setx) >(this, &example::setx);
    //friend class accessor< double, example, decltype(&example::setx) >;

};

int main()
{
    example e;
    e.x = 2*3.14159;
    std::cout << "New value: " << e.x << '\n';

    return 0;
}

Demo

The questions I have questions are:

Does anybody see any issues with the above?
Does anybody have any suggestions in implementing this without using the preprocessor and still keep the interface similarly compact?

Design
I think it obfuscates the code with no real advantage.
// This adds an object `x` to the class `example` that
// when assigned a double will forward the
// value to the private method `setx()`
OPEQ_SETTER_GETTER( example, double, setx, x );

I think it would be easier to add a public function that forwards the value.
public:
void publicSetX(double v) {setx(v);}  // ? easier to read.

Also I add my usual rant about getters/setters breaking encapsulation by exposing internal implementation details.
Code Review
Make sure this is noted as a non-owning pointer.
Owner *owner;

When you construct the object. Owner should never be nullptr
accessor ( Owner* o, Setter &&f ): owner( o ), setter( f ) { }

So pass owner by reference to indicate this. Its OK to store as a pointer (as its hard to copy objects with references without additional work). But your interface should be as clean as possible so there is no misunderstanding. Pass by reference to indicate it can't be nullptr and that this object is NOT taking ownership.
The setter value f can be bound to an r-value reference. But the parameter f will not pass it on as an r-value reference. Named objects can not be r-value references. So you need to use std::forward to activate perfect forwarding. Otherwise you are just activating the copy constructor (not the move constructor).
accessor ( Owner& o, Setter &&f ): owner( &o ), setter( std::forward<Setter>(f) ) { }

OK. For doubles passing a reference around is fine. But this is supposed to be generic. You also need to allow for move assignment.
// What if T is big.
// I would want to move the `value` into the object using assignment.
accessor &operator=( const T &value ) { setter( owner, data, value ); return *this; }

Reading the value generates a copy. This is not usually desirable. Especially if T is large.
operator T() const { return data; }

I would do:
operator T const&() const { return data; }


Parsing Bitcoin binary data file with Python The script parses Bitcoin database (blkXXXXX.dat) files directly from raw binary to txt human readable view. And I think about how to encrease the speed of processing.

Can anyone suggest how to improve the performance? I know that the best way to encrease performance is to write this on C or C++, but what about Python?  Also, is this style of programming is good?

Example cut from code (full code avaible here):

import os
import hashlib

def reverse(input):
    L = len(input)
    if (L % 2) != 0:
        return None
    else:
        Res = ''
        L = L // 2
        for i in range(L):
            T = input[i*2] + input[i*2+1]
            Res = T + Res
            T = ''
        return (Res);

f = open('blk01234.dat','rb')
tmpHex = ''
fSize = os.path.getsize(t)
while f.tell() != fSize:
    for j in range(4):
        b = f.read(1)
        b = b.encode('hex').upper()
        tmpHex = b + tmpHex
    tmpHex = ''
    for j in range(4):
        b = f.read(1)
        b = b.encode('hex').upper()
        tmpHex = b + tmpHex
    resList.append('Block size = ' + tmpHex)
    tmpHex = ''
    tmpPos3 = f.tell()
    while f.tell() != tmpPos3 + 80:
        b = f.read(1)
        b = b.encode('hex').upper()
        tmpHex = tmpHex + b
    tmpHex = tmpHex.decode('hex')
    tmpHex = hashlib.new('sha256', tmpHex).digest()
    tmpHex = hashlib.new('sha256', tmpHex).digest()
    tmpHex = tmpHex.encode('hex')
    tmpHex = tmpHex.upper()
    tmpHex = reverse(tmpHex)
    resList.append('SHA256 hash of the current block hash = ' + tmpHex)
    f.seek(tmpPos3,0)
    tmpHex = ''
    for j in range(4):
        b = f.read(1)
        b = b.encode('hex').upper()
        tmpHex = b + tmpHex
    resList.append('Version number = ' + tmpHex)
    tmpHex = ''
#.....next part of code
f.close()
f = open('blk01234.txt','w')
for j in resList:
    f.write(j + '\n')
f.close()

Some suggestions:

Every single line should at least be comprehensible on its own. Single letter variables destroy that comprehension, meaning that anyone reading the code needs to read the entire code and mull it over for a while to even understand what it's meant to do. Something like current_byte = file_handle.read(1) is a lot more readable than b = f.read(1).
All the top-level code should be in either functions, methods or classes, and the file should have a main function, so that the pieces of this code can be understood and reused.
Running this through at least one linter such as flake8 and pycodestyle will give several tips to produce more pythonic code. For example, return (Res); should be written as return res - no parentheses or semicolon, and snake_case variable names.
There are several magic values like 80 and 4 which are hard to guess the meaning of. They should be pulled out as named variables or constants.
Instead of closing the file handle explicitly you should use a context manager like with open(…) as file_handle:. This ensures that the file handle will be closed even if there is an exception.
Rather than checking whether the current position in the file is at the end you should read until reading returns nothing.
There are assignment operators such as //= which can simplify some of your expressions.
A helpful exercise for detecting "hacky" code is to add type hints to it, and verifying it with mypy using strict settings (like not allowing Any). For example, it looks like the type of b would be Union[byte, str], which is a code smell - the variable is literally used for two different types of things.


Determine the height of a Tetris game's board after a sequence of moves I have written a program that will determine the height of a Tetris board after a sequence of moves are made. These inputs are in the form of a comma-delimited list, and look like <piece><position>. List of pieces:

I - this is a 1x4 piece lying on its side
Q - this is a 2x2 square piece
T - this is a T-shaped piece
Z - this is a left-facing 2x2 offset
S - this is a right-facing 2x2 offset
L - this is a right-facing L
J - this is a left-facing L

Image (source) of the pieces. Pieces are always in the same orientation as below.

I've diagrammed them out below as well. Rotation is not in scope for this problem (e.g. a vertical I is out of scope).
I - xxxx
Q - xx
    xx
T - xxx
     x
Z - xx
     xx
S -  xx
    xx
L - x
    x
    xx
J -  x
     x
    xx

Positions are 0-indexed, and represent a location from the left side of the board (the board is 10-wide).
Example 1:
Input: I0,Q4
Output: 2
Board:
bbbbQQbbbb
IIIIQQbbbb

(b represents a blank space, and the blank lines above this are left out)
Example 2
Input: Q0,Q2,Q4,Q6,Q8
Output: 0
Board (intentionally left blank):

Explanation: Using normal Tetris rules, a row is removed whenever every block in a row is filled. This sequence would place 5 square cubes evenly spaced along the bottom, which then removes those two rows.
class Tetris:
    def __init__(self):
        self.board =[]
        self.pieces = {
            'I' : [[1,1,1,1]],

            'Q' : [[1,1],
                   [1,1]],

            'T': [[1,1,1],
                  [0,1,0]],

            'Z':[[1,1,0],
                 [0,1,1]],

            'S':[[0,1,1],
                 [1,1,0]],

            'L':[[1,0],
                 [1,0],
                 [1,1]],

            'J':[[0,1],
                 [0,1],
                 [1,1]]}

    def newRow(self):
        return [0 for _ in range(10)]

    def doesThePieceFit(self,row,pieceName,pos):
        #checks to see if a piece fits on the row at given position
        #check bottom to the top
        piece = self.pieces[pieceName]
        for i in range(len(piece)):
           pieceRow = piece[-1*(1+i)]
           if i+row == len(self.board): return True
           boardRow = self.board[i+row]
           for j in range(len(pieceRow)):
               if pieceRow[j] and boardRow[pos+j]: return False
        return True

    def removeFullRows(self,startRow,numRows):
        #removes full rows from the board
        #only checks rows between startRow and startRow+numRows
        fullRows = [i+startRow
                    for i in range(numRows)
                    if all(self.board[i+startRow])]
        for fullRow  in sorted(fullRows,reverse=True):
            del self.board[fullRow]

    def addPieceAt(self,row,pieceName,pos):
        #Adds piece at this row.
        piece = self.pieces[pieceName]
        for i in range(len(piece)):
           pieceRow = piece[-1*(1+i)]
           if i+row == len(self.board):
               self.board+=self.newRow(),
           boardRow = self.board[i+row]
           for j in range(len(pieceRow)):
               if pieceRow[j]:
                   boardRow[pos+j] = pieceRow[j]
        self.removeFullRows(row,len(piece))

    def addPiece(self,pieceName,pos):
        #1.find the first row where piece is blocked
        #2.Add the piece at the row above it
        blockedByRow = None
        for row in range(len(self.board)-1,-1,-1):
            if not self.doesThePieceFit(row,pieceName,pos):
                blockedByRow = row
                break

        targetRow = 0 if  blockedByRow == None else blockedByRow+1
        self.addPieceAt(targetRow,pieceName,pos)

    def addPieces(self,pieces):
        for piece in pieces.split(','):
            self.addPiece(piece[0],int(piece[1]))
        return len(self.board)

The first thing I did was use Black to reformat the code - yours is pretty good, but there are some minor style complaints I had (generally around the lack of whitespace in a few places). Additionally, PEP8 defines the naming conventions in python - generally, prefer_this notThis.
Lastly, all of your methods should have docstrings. I haven't added this b/c it isn't as pertinent to the code review, but it is good practice in general.
From there, I thought about your actual approach. At a high level you:

Create a new instance of the object
Pass it a string, parse the string, and process each token
Attempt to fit pieces
Clear full rows

None of that is inherently bad, but I think it can be tightened up a bit.
User Input
Right now you don't have any validation of the user inputs - we're being very trusting that the values that are provided will be usable. We probably want to do this validation
Additionally, I don't think that the Tetris class should be responsible for handling the comma-delimited string - it should just take a piece and a position, and something else should be responsible for taking the input and translating it into arguments. If you're feeling friendly, a @classmethod might be appropriate. Lastly, I think this class method should return the board, not the height, so I added a new height property to the class. I ended up with something like this:
pieces = {
    "I": ((True, True, True, True)),
    "Q": ((True, True), (True, True)),
    "T": ((True, True, True), (False, True, False)),
    "Z": ((True, True, False), (False, True, True)),
    "S": ((False, True, True), (True, True, False)),
    "L": ((True, False), (True, False), (True, True)),
    "J": ((False, True), (False, True), (True, True)),
}

@classmethod
def add_pieces(cls, user_input):
    board = Tetris()
    for piece in user_input.split(","):
        if len(piece) > 2:
            raise ValueError(f"Piece {piece} is malformed")
        piece_id = piece[0]
        drop_position = piece[1]
        if not Tetris.is_valid_piece(piece_id):
            raise ValueError(f"Piece {piece_id} is not a valid Tetris piece")
        if not Tetris.is_valid_drop_location(drop_position):
            raise IndexError(
                f"Drop location {drop_position} is not a valid board location"
            )
        board.add_piece(piece_id, drop_position)
    return board

@classmethod
def is_valid_piece(cls, piece_id):
    return piece_id in cls.pieces

@classmethod
def is_valid_drop_location(drop_position):
    try:
        int(drop_position)
    except ValueError:
        return False

    return drop_position >= 0 and drop_position < 10

@property
def height(self):
    return self.board.length

You'll also notice that I moved Tetris.pieces into a class attribute instead of an instance attribute - this is because it should be the same everywhere. I also changed 0/1 to True/False because it is a binary value (I think an enum is probably best to be explicit, e.g. boardState.FULL and boardState.EMPTY). Lastly, I changed from nested lists to nested tuples - this is because tuples are immutable, and you never need to change the shape definition.
OOP
I wonder if it is worthwhile making a separate class to represent the pieces, and then you can do something like TetrisPiece.fitsAtLocation(board, location). I haven't fully thought about what this would look like or if it is actually better, but it might be a nice way to encapsulate that functionality.
This would also be a convenient way to extend this to handle rotations as well, as you would just do TetrisPiece.rotate(Direction.LEFT) and handle it all under the hood.
If you want to extend this to a full game, then instead of just having a "drop position" you also need a relative location on the board, handling T-spins, etc. The more complicated this gets, the more I think a separate class is going to improve readability.
General nitpicks

doesThePieceFit seems really weird - I get how it works, but you should definitely introduce some constants to replace the magic method, and maybe consider if there is a better way to model the data.

In particular, perhaps we should store the block state for a different shape in reverse order (e.g. bottom-to-top instead of top-to-bottom)?

removeFullRows creates a list, then sorts it - I think you can probably come up with a different approach for this
addPieceAt has the same magic as doesThePieceFit - is there a way that we can either combine their functionality, or use a common helper method?
addPiece I think you can use for-else to handle this a bit more elegantly than using the ternary, but my mood on the for-else swings every time I use it


knockout binding handler for custom components I am using a combination of knockoutJS and jQuery. I have a number of jQuery plugins which perform particular re-usable functions, such as a numeric spinbox.

I have written a binding handler to allow me to bind an observable property to the spinbox, it looks like this:

ko.bindingHandlers.spinbox = {
    init: function (element, valueAccessor, allBindings, viewModel, bindingContext) {
        var value = valueAccessor();
        $(element).spinbox({
            min: value.min === undefined ? -5 : ko.unwrap(value.min),
            max: value.max === undefined ? 5 : ko.unwrap(value.max),
            scale: value.scale === undefined ? 2 : ko.unwrap(value.scale),
            step: value.step === undefined ? 0.1 : ko.unwrap(value.step),
            bigStep: value.bigStep === undefined ? 0.5 : ko.unwrap(value.bigStep)
        }).change(function() {
            value.data(parseFloat($(this).val()));
        });
    },
    update: function (element, valueAccessor, allBindings, viewModel, bindingContext) {
        var value = valueAccessor();
        var valueUnwrapped = ko.unwrap(value.data);
        $(element).val(valueUnwrapped.toFixed(1));
    }
};

and requires the binding to be set up something like:

data-bind="spinbox: {data: myProperty, min: -100, max: 100}"

As you can see, I have used the object notation to sent multiple parameters to the binding (min, max etc) - this is similar to the syntax that the knockout template binding uses. However I am not sure this is a sensible way to do this. Should I have instead used allBindings and passed these along as separate values like:

data-bind="spinbox: myProperty, min: -100, max: 100"

I am interested in opinions on complex knockout custom bindings.

I like your approach much better than using separate values.

The code reads really well, I only have 1 minor suggestion.

If you had a helper function like this:

function unwrapOrDefault( gift , defaultValue )
{
    return gift === undefined ? defaultValue : ko.unwrap( gift ),
}

then you could have

        min: unwrapOrDefault(value.min, -5),
        max: unwrapOrDefault(value.max, 5 );
        scale: unwrapOrDefault(value.scale, 2);
        step: unwrapOrDefault(value.step, 0.1);
        bigStep: unwrapOrDefault(value.bigStep, 0.5);


Dealing with multiple types in an excel sheet I need to parse an excel sheet in order to produce an statistic, and some of the columns in the sheet can have different value types within the same column. For example:

A        B
String  String
String  String
Date    String

I cannot force the columns to have a consistent type because the input sheet belongs to one of our clients (the ones with the money), so I'm stuck with possible multiple types of values in the same column. To address this issue I made this class:

public final class MultiTypeValue {

private Map<Class<?>, Optional<?>> values;

public static MultiTypeValue newInstance(Class<?>... classes) {
    return new MultiTypeValue(classes);
}

private MultiTypeValue(Class<?>... classes) {
    if (classes == null || classes.length == 0) {
        throw new IllegalArgumentException("The classes cannot be empty");
    }

    values = new ConcurrentHashMap<>();
    for (Class<?> clazz : classes) {
        values.put(clazz, Optional.empty());
    }
}

public void put(Class<?> clazz, Object value) {
    values.put(clazz, Optional.ofNullable(value));
}

@SuppressWarnings("unchecked")
public <T> T value(Class<T> clazz) {
    return (T) values.get(clazz);
}

}

The idea was that for each type a column can have, to have an Optional associated to that type which may or may not have a value.

In terms of performance I imagine this is pretty bad, but I couldn't think another solution. Does anyone know a framework or a design pattern better than this solution?

A few nitpicks in general:

Your indentation is off

I'm not sure whether that's just a copy paste-error or actually intended, but it's convention to indent the class body by 4 Spaces.

public class MultiTypeValue {

    private Map //...

Eager initialization saves you some pain

Instead of initializing your values in the constructor, you might prefer to eagerly initialize the Map as a final field:

public class MultiTypeValue {

    private final Map<Class<?>, Optional<?>> values =
      new ConcurrentHashMap<>();
    // ...

Why do you disallow an empty map?

I don't think it's a wise choice to disallow an empty Map if you don't validate the class-object pairs you put in anyways. As such initializing the map with an empty Optional seems... unhelpful.

Either you stop requiring classes for construction or you make use of the classes by validating you're only updating a value of an allowed class instead of just silently taking it:

public void put(Class<?> clazz, Object value) {
    if (values.containsKey(clazz)) {
       values.put(clazz, Optional.ofNullable(value));
    }
}

Use generics when putting a value

You should really do some more checking when you put a value. Consider the following:

public void <T> put(Class<T> clazz, T value) {

this is where it gets really interesting, since you can just extract the relevant class from your argument:

public void <T> put(T value) {
    Class<T> clazz = value.getClass();

I'm not quite clear on how your requiremnts / usage of this class is though, so this may just completely miss your usecase.

The factory method is unnecessary

As of now you're simply misusing your factory method as a proxy for your constructor. Usually you'd implement a factory method to allow for caching and instance control. Since you don't do either of that here, the factory method isn't necessary and you should just expose the constructor directly to simplify things

Missing documentation

One of the reasons I wasn't sure what exactly you intend this class for is that it's completely undocumented.

You explain nothing on this class and as an external developer on your project I'd be very confused by it I daresay. Then again I haven't seen it in usage context, but either way:

This seems like a core class for your functionality and it should be documented accordingly


JavaScript bookmarklet with a 24 hour cache I'm working on a bookmarklet that will be cached for 24 hours only, after that time it will be reloaded from the original source.
Is there something I could improve?

javascript:(function () {var currentTime=new Date();var month=currentTime.getMonth()+1;var day=currentTime.getDate();var year=currentTime.getFullYear();document.getElementsByTagName('head')[0].appendChild(document.createElement('script')).src = 'http://bookmarklet.example.com/js/bookmarklet.js?v='+month+'-'+day+'-'+year}());

Use timestamps.

It's faster since you don't need to get the date, month and year. In the following script, the only function called for the date is getTime(). Additionally, if you use the newer Date.now(), you skip creating a date object.

In the example, we get the current timestamp and modulo it by a day. The remainder is the milliseconds that have passed since midnight. We then subtract it from the current timestamp so that the timestamp for the url is always based on the day. The only time it changes is when it's the next day.

Write to read

Sure, you're building a bookmarklet. But that does not mean making it unreadable. Write to read. Let the minifier do the shrinking for you. Besides, the bookmarklets themselves are copy-pasted, and not loaded. There's no overhead in size.

So in the following script, we pluck out the url so it's easily editable. We also make some parts verbose for easy reading.

The script

var url = 'http://bookmarklet.example.com/js/bookmarklet.js?v='

var currentTimestamp = new Date().getTime(); // or Date.now()
var currentDayTimestamp = currentTimestamp - (currentTimestamp % 86400000);

var script = document.createElement('script');
document.getElementsByTagName('head')[0].appendChild(script);

script.src = url + currentDayTimestamp;

Perf tests

Well, if you are bent on performance, then here's some perf tests.


Displaying pagination links, maybe with links to first and last page I've been researching if this is possible, but I've drawn a blank, I'm wondering if it's possible to optimize these for and if statements together?

<?php
// calculate total number of pages
$total_pages = ceil($total_rows / $records_per_page);

// range of links to show
$range = 2;

// display links to 'range of pages' around 'current page'
$initial_num = $page - $range;
$condition_limit_num = ($page + $range)  + 1;
?>

<?php if ($page>1) : ?>

            <a href='<?php echo $page_url; ?>' title='Go to the first page.'>
                First Page

<?php endif; ?>

<?php
for ($x = $initial_num; $x<$condition_limit_num; $x++) :
    if (($x > 0) && ($x <= $total_pages)) :
        if ($x == $page) : ?>

            <?php echo $x; ?> (current)

        <?php else : ?>

            <a href='<?php echo $page_url; ?>page=<?php echo $x; ?>'><?php echo $x; ?>

        <?php
        endif;
    endif;
endfor;
?>

<?php if ($page<$total_pages) : ?>

            <a href='<?php echo $page_url; ?>page=<?php echo $total_pages; ?>' title='Last page is <?php echo $total_pages; ?>'>
                Last Page

<?php endif; ?>

The main loop code lacks the prior validation. So in the business logic part

verify that $x is positive. if not, make it the least number possible (1 I suppose)
verify that $x is <= $total_pages. Make them equal otherwise
prepare the page numbers in the form of array using $pages = range($initial_num, $condition_limit_num);

So you will have a single loop with a single condition

<?php foreach ($pages as $x): ?>
    <?php if ($x == $page) : ?>

            <?= $x ?> (current)

    <?php else: ?>

            <a href="<?= $page_url ?>page=<?= $x ?>"><?= $x ?>

    <?php endif ?>
<?php endfor ?>


Class for holding person's information I have written another program in C++. Excluding the point that the class definition should be in a separate header file, is there anything that needs to be improved?

/*
    Date:5th January 2011
    Programmer:Fahad
*/
#include <iostream>
#include <string>
using namespace std;
class Persons     //A class that will store the name,addresses and id numbers of the users
{
    private:
        string name_;
        string address_;
        int id_number_;

    public:
        static int count;    //This is the count of the objects created
        Persons();
        void getData(int n);
        void displayData(int n);
        ~Persons();
};

int Persons count;//initializing the static member

int main()
{
    cout << "Enter Number Of Persons:";
    int n;     //This is the number of objects that the user wants to make.
    cin >> n;
    Persons *ptr;    //A pointer that will be used for the dynamic memory allocation.

    /*Exception Handling*/
    ////////////////////////////////////////////////////////////////////
    try
    {
        //ptr=new [sizeof(Persons) * n];
        ptr=new Persons[n];
    }
    catch(bad_alloc xa)
    {
        cout<<"Sorry,Program Can Not Continue";
        cin.get();
        exit(1);
    }
    /////////////////////////////////////////////////////////////////////
    for(int i = 0; i< n; i++)
    {
        ptr[i].getData(n);
    }
    for(int j = 0; j< n; j++)
    {
        ptr[j].displayData( n );
    }
    cin.get();
    delete[] ptr;
    return 0;
}

/*Function Definitions*/

Persons::Persons()
{
    name_="";
    address_="";
    id_number_=0;
    count++;
}
void Persons::getData(int n)
{

        cout<<"Enter Name (Press '$' To Exit):";
        getline(cin,name_,'$');
        cout<<endl<<"Enter Address (Press '$' To Exit):";
        getline(cin,address_,'$');
        cout<<endl<<"Enter Identitiy Card Number:";
        cin>>id_number_;

}
void Persons::displayData(int n)
{

        cout<<"Name:"<<name_;
        cout<<endl<<"Address:"<<address_;
        cout<<endl<<"Identitiy Card Number:"<<id_number_;

}

I avoid using namespace std;.

cout << "Enter Number Of Persons:";
int n;     //This is the number of objects that the user wants to make.
cin >> n;

Personally, I always use getline to capture a line at a time when using std::cin for interactive input. The fact that >> stops at whitespace means that it is very easy for the user to enter two fields in a single line and then the second field gets used at a subsequent point in the program without the user being able to respond to a later prompt.

Also you should always check whether the input operation succeeded.

std::string inputline;
if( std::getline( cin, inputline ) )
{
    std::istringstream iss( inputline );
    if( iss >> n )
    {
        // success
    }
    else
    {
    }   // fail
}
else
{
    // fail
}

catch(bad_alloc xa)
{
    cout<<"Sorry,Program Can Not Continue";
    cin.get();
    exit(1);
}

For all that you actually do in the exception handler, you may as well let the expection propogate. The runtime might actually give a more meaningful error message for bad_alloc. Also, unless you have a good reason, always catch exception by const reference. e.g. catch( const std::bad_alloc& ba ).

Looking at your definition of Persons, I think that Person is a better name for the class. I see one name and one address member, if the class represented multiple people (e.g. a household) you would need at least multiple names, I would have thought.

Always try and avoid needing to use delete[] explicitly. Here a vector would be much simpler and safer.

std::vector<Person> persons(n);

int Persons count;//initializing the static member

Class static variables are normally not advisable. This isn't valid, it should be:

int Persons::count;

In this program it isn't giving you anything that n isn't so I would just remove it.

Persons::Persons()
{
    name_="";
    address_="";
    id_number_=0;
    count++;
}

Always prefer to initialize member variables. Assigning "" to a newly constructed std::string is redundant so you could do either.

Persons::Persons()
    : name_()
    , address_()
    , id_number(0)
{
    count++;
}

or even this (although some people prefer an explicit initializer for all members and bases).

Persons::Persons()
    : id_number(0)
{
    count++;
}

for(int i = 0; i< n; i++)
{
    ptr[i].getData(n);
}
for(int j = 0; j< n; j++)
{
    ptr[j].displayData( n );
}

Why do getData and displayData take an int? They don't use the parameter, and I don't understand why you are passing n.

cin.get();

This seems redundant.

void Persons::getData(int n)
{

        cout<<"Enter Name (Press '$' To Exit):";
        getline(cin,name_,'$');
        cout<<endl<<"Enter Address (Press '$' To Exit):";
        getline(cin,address_,'$');
        cout<<endl<<"Enter Identitiy Card Number:";
        cin>>id_number_;

}

Again, my comments about the unused parameter, always using getline and always checking the success of input operations all apply.


Reversing a String in Java I'm new to Java and I want to know if this code is efficient, and if there a shorter and faster way to reverse a string.

package Excercises;

import java.util.Scanner;

public class Main {
    public static void main(String[] args) {
        Scanner scan = new Scanner(System.in);
        System.out.print("Please enter a string you want to reverse: ");
        String text = scan.next();
        String[] letters = text.split("");
        char[] reversed = new char[letters.length];
        int c = letters.length - 1;
        for(String s : letters) {
            reversed[c] = s.charAt(0);
            c = c - 1;
        }

        System.out.println(new String(reversed));
    }
}

is there a shorter and a faster way than below to reverse a string?

Sure,

new StringBuilder(string).reverse().toString()

is generally used. Here is code to reverse a string that's also quite nice and that might even be faster in some situations.

As to your code:

naming: avoid short variable names: c and s could be index and letter.
c = c - 1 is often written as c--.
you shouldn't just write your code in main, but in a function so that you can easily test it. And some unit test can never hurt.
your code throws a StringIndexOutOfBoundsException for the empty string.

  is this code efficient?

No, not really. I profiled it, and the result is:

name                    per call (mean, ns)     per call (median, ns)     95th percentile (ns)     total (ms)     runs
reverseCharToString     897                     892                       816                      5.7408         8000000
reverseBuilder          132                     132                       101                      0.8448         8000000
reverseArray            126                     125                       95                       0.8064         8000000

Where reverseCharToString is your code, reverseBuilder is the code I pasted above, and reverseArray is the code I linked to. The main reason for this is that you are creating a lot of new objects.


Selecting results as a comma-separated string The below code is working correctly, however, I am not sure if the combination of while, foreach and implode is really necessary here or if the same can be achieved much simpler.

What I do here is fetching some data (text) from a db and the only thing I need is the results as a comma separated string.

Now even though this is working I was wondering if there is a direct way to get the results as a comma separated string since the arrays here are not needed for anything else.

$stmt = $conn->prepare("SELECT " . $languageFrm . " FROM TranslationsMain WHERE location LIKE ? ORDER BY sortOrder, " . $languageFrm);
$stmt->bind_param("s", $location);
$stmt->execute();
$result = $stmt->get_result();
$strTranslations = array();
while($arrTranslations = $result->fetch_assoc()){
    $translations[] = $arrTranslations;
}
foreach($translations as $translation){
    array_push($strTranslations, $translation[$languageFrm]);
}
echo implode(",", $strTranslations);

Using one of the below queries removes the need to iterate over the result sets to construct the CSV strings. When a single result is expected, a ->fetch() instead of ->fetchAll() would do.

Conclusion: implode goes away, while and for loops go away the only iteration needed would be in case of multiple CSV result sets.

SELECT GROUP_CONCAT(columnName) FROM Table1; # for single column

SELECT CONCAT(columnA,",",columnB) from Table2; # for multi cols

You might have issues creating prepared statements for this; I don't know.

example for the first query:

<?php

$conn = new PDO("mysql:host=localhost;dbname=test", "root", "");

$stmt = $conn->query("SELECT GROUP_CONCAT(columnName) FROM Table1");

$result = $stmt->fetch(PDO::FETCH_ASSOC);

var_dump($result);


Randomized response generator for number-guessing game I am currently learning C and playing around with the rand() and if/else statement. This code looks long, just curious if there is another way to make the code shorter?

The code below works fine, it's just randomize the output every time someone answers the question incorrectly.

while(input != c){
    srand(time(NULL));
    int i;
    i = rand() % 5 + 1;
    if (i == 1)
    {
        printf("No. Please try again.\n");
        scanf("%d", &input );
    }
    else if(i == 2){
        printf("Wrong. Try once more.!\n");
        scanf("%d", &input );
    }
    else if(i == 3){
        printf("Don't give up!\n");
        scanf("%d", &input );
    }
    else if(i == 4){
        printf("No. Keep trying.!\n");
        scanf("%d", &input );
    }
}

Is there any way to make it shorter, instead of using if/else over and over?

A standard technique is to setup an array of strings and index it with a random number:

char * answers[] = { "Wrong!", "No", "Try again"};
....
if (guess_is_wrong) {
    int i = random() % (sizeof(answers) / sizeof(answers[0]));
    printf("%s\n", answer[i]);
}


Using Java to implement a hash table (dictionary) with only arrays I'm new to hash codes/hash tables, so I'm very unsure of my implementation. I want to create a hash table, but I can only use arrays. Specifically, I want to be able to insert things like a dictionary (a word and its meaning). Here is what I have:

public class HashTable {
    String[][] table;
    int tableSize;

    HashTable(int size) {
        table = new String[size][];
        tableSize = size;
    }

    public void add(String key, String value) {

        if (key == null || value == null) {
            System.out.println("Cannot input null values");
        }

        int iter = 0;
        int code = Math.abs(key.hashCode()) % tableSize;

        if (table[code] == null) {
            table[code] = new String[]{key, value};
        } else {

            while (table[code] != null) {
                if (table[code][0].equals(key)) {
                    System.out.println("Already submitted submitted the word " + key);
                    return;
                }

                if (iter == tableSize) {
                    System.out.println("Table is full. Cannot add word \"" + key + "\"");
                    return;
                }

                code++;
                code %= tableSize;
                iter++;
            }

            table[code] = new String[]{key, value};

        }
    }

    public void remove(String key) {

        if (key == null) {
            System.out.println("Cannot input null value");
        }

        int iter = 0;
        int code = Math.abs(key.hashCode()) % tableSize;

        if (table[code][0].equals(key)) {
            table[code] = null;
        } else {
            while (!table[code][0].equals(key)) {
                if (iter == tableSize) {
                    System.out.println("Could not find word \"" + key + "\"");
                    return;
                }
                code++;
                code %= tableSize;
                iter++;
            }

            table[code] = null;

        }
    }

    public String get(String key) {

        if (key == null) {
            return "Cannot input null value.";
        }

        int iter = 0;
        int code = Math.abs(key.hashCode()) % tableSize;

        if (table[code][0].equals(key)) {
            return table[code][1];
        } else {
            while (!table[code][0].equals(key)) {
                if (iter == tableSize) {
                    return "Could not find word \"" + key + "\"";
                }
                code++;
                code %= tableSize;
                iter++;
            }

            return table[code][1];

        }
    }

}

Is this an okay implementation? How can I make it better?

Thanks!

Do not special case an immediate success. The test for table[code] == null in

    if (table[code] == null) {
        table[code] = new String[]{key, value};
    } else {
        while (table[code] != null) {
        ....

does not do anything useful, but only adds noise. Dive into the loop immediately.

Ditto for table[code][0].equals(key) in remove and get.
Such low level utility functions shall not System.out.println (in any case, an error message shall go to System.err). Prefer returning an error code, and let the caller decide what to do.
It is unclear how well String.hashCode() is suited for this design of a hash table. Depending on the table size, you may get plenty of collisions. Using Math.abs on a hash code also biases the distribution. Need to measure.


C program that reverses a string As practice with the (ever so painful) C Strings I wrote a string reversal program. I am new to C, so I wanted to make sure that I am not using bad coding practices. It runs just fine (at least with expected input), but I am not sure if the logic is as good as it could be.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
char* strreverse(char* c)
{
    char input[500], output[500] = {}, buffer[2] = {' ', '\0'};
    strcpy(input, c);
    int i;
    for(i = strlen(input); i >= 0; i--)
    {
        buffer[0] = input[i];
        strcat(output, buffer);
    }
    return (char*) output;
}

int main(void)
{
    char* in = malloc(256);
    printf("Enter a string: ");
    scanf("%s", in);
    char* out = strreverse(in);
    printf("%s", out);
    return 0;
}

Implementation:

As of now, the code is not practical, because the limit is 500 chars including zero termination. It does unnecessary copying. You need to determine the actual length of the string by relying on the fact that C strings are null terminated.

size_t length = 0;
while (*(str + length) != 0)
{
    ++length;
}

Notice that I'm using size_t, because it is meant to be used to describe size of the objects in memory. Despite it is easy to do it manually, you should always use strlen when possible:

size_t length = strlen(str);

It should be noted that the function returns the length without null terminator. Then we allocate memory for result, since we are creating new string, rather than making changes in place:

char* result = malloc(length + 1);
if (result == NULL)
{
    return NULL;
}

We check for succession of allocation, as was suggested in the comments. If allocation failed, further operations will definitely invoke undefined behavior, so we return NULL. length + 1 is for null terminator. Then, we immediately null terminate it:

result[length] = 0;

Now we need to capture the position of the last character in the original string, to start copying from there. It is right before null terminator, which is on index length, so needed position is length - 1. There is an edge case of length 0, so we should check for that first

if (length == 0)
{
    return result;
}
size_t last = length - 1;

Then we write algorithm that writes contents of the first string into the second in reverse order:

size_t it = 0;
while (it <= last)
{
    result[it] = str[last - it];
    ++it;
}

then simply return the result:

return result;

Although the implementation has good performance, there are some opportunities to make micro optimizations. The function input type should be const char*, since we don't modify original string. Additionally, it should be noted that the caller is responsible for freeing up malloc'd memory.

Cosmetics:

Name of the function is a little hard to read, so I'd suggest str_reverse. Also, c is not good name at all, so it would be better if it's name str

Put together:

#include <stdio.h>
#include <string.h>

char* str_reverse(const char* str)
{
    size_t length = strlen(str);

    char* result = malloc(length + 1);
    if (result == NULL)
    {
        return NULL;
    }

    result[length] = 0;

    if (length == 0)
    {
        return result;
    }

    size_t last = length - 1;
    size_t it = 0;
    while (it <= last)
    {
        result[it] = str[last - it];
        ++it;
    }

    return result;
}

int main(void) {
    const char* str = "ABCD";
    char* reversed = str_reverse(str);

    printf("%s\n", reversed);
    if (str[4] != 0)
    {
        printf("problems\n");
    }

    free(reversed);
    return 0;
}


Functional prime factor generator I have this prime factor generator for some number n. It returns list of all prime factors. In other words, product of the list equals n.

def prime_factors_generate(n):
    a = []
    prime_list = sorted(primes())
    pindex = 0
    p = prime_list[pindex]
    num = n
    while p != num:
        if num % p == 0:
            a.append(p)
            num //= p
        else:
            pindex += 1
            p = prime_list[pindex]
    return a

The primes() function uses the Sundaram Sieve from here and returns a set of all primes below some limit, by default 106.

How can I make this code functional? I want to get rid of the while-loop, list appending and mutated state.

The implementation of sundaram's sieve that you've mentioned returns primes in sorted order. So, sorted() is not needed.

Also, it is best to define prime_list = primes() outside the definition of prime_factors_generate (This will reduce calls to primes()). You can argue that that will make the code less functional but that would be nitpicking [Even the definition of primes() isn't functional]

EDIT: As pointed out in a comment by mteckert, my earlier solution will not provide the desired result. This EDIT will work though (It'll return [2, 2, 2, 3] instead of [2, 3] for prime_factors_generate(24))

def max_pow(n, x):
    if n%x == 0:
        return max_pow(n/x, x)+1
    else:
        return 0

def prime_factors_generate(n): #Variables added just for convenience. Can be removed.
    prime_list = primes()
    nested_ans = map(lambda x: [x]*max_pow(n, x), prime_list)
    ans = [item for sublist in nested_ans for item in sublist]
    return ans

One liners (if you're interested in them) for max_pow and prime_factors generator:

max_pow = lambda n, x: max_pow(n/x, x)+1 if n%x==0 else 0
prime_factors_generate = lambda n: [item for sublist in map(lambda x: [x]*max_pow(n, x), primes()) for item in sublist]

End of EDIT. I'm leaving my earlier solution intact (which returns [2, 3] for prime_factors_generate(24)).

This should work:

def prime_factors_generate(n): #Variables added just for convenience. Can be removed.
    prime_list = primes()
    prime_factors = filter(lambda x: n % x == 0, prime_list)
    return prime_factors

Or if you're into one liners:

prime_factors_generate = lambda n: filter(lambda x: n % x == 0, primes())


Java class: for getOrder, getData, updateData operation Problem Statement :

Design a class for three operations for given set of integers :

int getOrder(int data) /* order meaning rank in list */
int getData(int order)
void updateData(int oldData, int newData) /* [Most used operation] */

My solution

class App {

    Set<Integer> appData;

    public App(List<Integer> data) {
        this.appData = new TreeSet<Integer>(new Comparator<Integer>() {
            @Override
            public int compare(Integer o1, Integer o2) {
                if(o1 > o2) {
                    return -1;
                } else if (o1 < o2) {
                    return 1;
                } else {
                    return 0;
                }
            }
        });
        appData.addAll(data);
    }

    public int getOrder(int m) {
        int counter = 1;
        for(Integer i : this.appData) {
            if(i == m) {
                return counter;
            }
            counter ++ ;
        }
        return -1;
    }

    public int getdata(int order) {
        int counter = 1;
        for(Integer data : this.appData) {
            if(counter == order) {
                return data;
            }
            counter++;
        }
        return -1;
    }

    public void updatedata(int olddata, int newdata) {
        if(appData.contains(olddata)) {
            this.appData.remove(olddata);
        }
        this.appData.add(newdata);
    }
}

(Please post working code next time.)

So first off, there are imports missing, so this doesn't compile as it
is, and the variable mark doesn't exist.

import java.util.List;
import java.util.Set;
import java.util.TreeSet;
import java.util.Comparator;

Either you import by name, or the whole package, just stick with one.

...
        if(counter == order) {
            return data;
        }
...

Then you should generally specify the visibility on all things,
otherwise you end up with package visibility, which is probably not what
you want.

public class App {
...
private Set<Integer> appData;

Use of generics is good, the constructor is okay except for the
unnecessary manual comparison.  This should be either using
TreeSet.descendingSet, or Comparator.reverseOrder, e.g.:

public App(List<Integer> data) {
    this.appData = new TreeSet<Integer>(Comparator.<Integer>reverseOrder());
    appData.addAll(data);
}

Some whitespace is also off and the spelling of methods should be
consistent, i.e. getdata should be get getData and updatedata
should be updateData as is mentioned in your spec.

That just to get you started here.


Can this numpy code be vectorized? I've written the following function to produce n realizations of a CIR process for a given set of parameters:
def cir_simulations(alpha, mu, sigma, delta_t, n, num_sims):

    x = np.reshape(np.array([mu] * num_sims), (-1, 1))

    for i in range(0, n):
        x = np.concatenate((x, np.reshape(x[:, -1], (-1, 1)) + alpha * (
                    mu - np.reshape(x[:, -1], (-1, 1))) * delta_t + sigma * np.sqrt(
            np.reshape(x[:, -1], (-1, 1))) * np.sqrt(delta_t) * np.random.normal(0, 1, size=(num_sims, 1))), axis=1)

    return x

This code works, but I am now wondering whether it would be possible to remove the loop and fully vectorize it. I've struggled to find a way of doing this, as the operation being performed in the loop is recursive (the values in the next column of the matrix are non-linearly dependent on the previous columns values).
Additionally, could this code be simplified? In particular, I feel like there may be needless complexity in the way that I am accessing the last column of the array using
np.reshape(x[:, -1], (-1, 1))

Since the output of each step depends on the previous step, it is impossible to entirely eliminate the loop unless the formula can be simplified to allow a multi-step computation. However, the code can definitely still be improved.

Calling np.concatenate is generally inefficient since it requires
memory reallocation and data copying. As long as possible, one should
preallocate all the needed memory and update it iteratively.

Some computation can be moved out of the loop to improve efficiency.
E.g., the np.random.normal call can be done only once. Same with alpha * delta_t.

More meaningful variable names could be chosen.

It is always a good practice to have docstrings and type hints in your code.

Here is one version of improved code:
def cir_simulations(alpha: float, mu: float, sigma: float, delta_t: float, sim_steps: int, num_sims: int):
    """
    Simulate the CIR process.

    Parameters:
    Input
    ...

    Output
    ...
    """
    output_shape = (num_sims, sim_steps + 1)
    sim_results = np.empty(output_shape)
    sigma_dW = np.random.normal(0, sigma * np.sqrt(delta_t), size=output_shape)
    alpha_dt = alpha * delta_t
    sim_results[0, :] = r_prev = mu

    for r_t, sigma_dWt in zip(sim_results[1:], sigma_dW):
        r_t[:] = r_prev = (mu - r_prev) * alpha_dt + np.sqrt(r_prev) * sigma_dWt

    return sim_results

I'm not very familiar with the meaning of all the parameters in the formula. There are two things that I have doubts in your computation:

The rate \$r\$ in the formula is initialized with the same parameter mu used in the iterative computation. Is it expected?
The increment \$dW_t\$ used a fixed variance of 1. Is this supposed to be the case or should it be another function parameter?


Codingbat maxMirror challenge I request you to provide insightful suggestions to improve the following code as well as an alternative algorithm to solve it.
Problem Specification:

We'll say that a "mirror" section in an array is a group of contiguous elements such that somewhere in the array, the same group appears in reverse order. For example, the largest mirror section in {1, 2, 3, 8, 9, 3, 2, 1} is length 3 (the {1, 2, 3} part). Return the size of the largest mirror section found in the given array.
maxMirror({1, 2, 3, 8, 9, 3, 2, 1}) → 3
maxMirror({1, 2, 1, 4}) → 3
maxMirror({7, 1, 2, 9, 7, 2, 1}) → 2

Solution:
public class MaxMirror {
    public static void main(String[] args) {
        MaxMirror max = new MaxMirror();
        int[] nums = { 1, 2, 1, 4 };
        System.out.println("The maximum length of the mirror is "
                + max.maxMirror(nums));
    }

    public int maxMirror(int[] nums) {
        if (nums.length == 0) {
            return 0;
        }
        // Reverse order of the array
        int[] revNums = new int[nums.length];
        for (int numIndex = nums.length - 1, revIndex = 0; numIndex >= 0; numIndex--, revIndex++) {
            revNums[revIndex] = nums[numIndex];
        }
        // Hope the sub array of maximum length is the array itself
        int subArraysToBeChecked = 1;
        for (int len = nums.length; len > 1; len--) {
            if (mirrorOfLengthLenExists(nums, revNums, len,
                    subArraysToBeChecked)) {
                return len;
            }
            // Increase the number of sub-arrays to be checked
            subArraysToBeChecked++;
        }
        // In the worst case return 1
        return 1;
    }

    // Choose start and end i.e. sub-arrays to be checked
    public boolean mirrorOfLengthLenExists(int[] nums, int[] revNums, int len,
            int subArraysToBeChecked) {
        int start = 0;
        int end = len - 1;
        for (int i = len; i <= nums.length; i++) {
            if (mirrorExistsFromStartToEnd(nums, revNums, start, end,
                    subArraysToBeChecked)) {
                return true;
            }
            start++;
            end++;
        }
        return false;
    }

    // Check from start to end whether mirrored in the revNums
    public boolean mirrorExistsFromStartToEnd(int[] nums, int[] revNums,
            int start, int end, int subArraysToBeChecked) {
        int revStart = 0;
        for (int subArraysChecked = 1; subArraysChecked <= subArraysToBeChecked; subArraysChecked++) {
            int noOfElementsEqual = 0;
            for (int numIndex = start, revIndex = revStart; numIndex <= end; numIndex++, revIndex++) {
                if (revNums[revIndex] != nums[numIndex]) {
                    break;
                } else {
                    ++noOfElementsEqual;
                }
            }
            if (noOfElementsEqual == (end - start + 1)) {
                return true;
            }
            revStart++;
        }
        return false;
    }
}

My approach is to have two indexes. The i index starts at the left and the j index starts at the right of the array. For a length of 4, we would do these comparisons in this order.
(0, 3), (0, 2), (0, 1), then (1, 3), (1, 2), then, (2, 3) This is basically a pairwise comparison. (Note the initialization part of the for loop is empty)

int frontIndex = 0;
int endIndex = sequence.length - 1;
for( ; frontIndex < endIndex; frontIndex++)
{
    for(int i = endIndex ; i > frontIndex; i--)
    {
    //stuff here
    }
}

Once a match is found, we start counting the max length, with one index moving to the left and one to the right. The while condition makes sure we don't go out of bounds. Once we break out of that loop, we may update the max length.

while(frontIndex + currentLength < sequence.length && i - currentLength >= 0)
{
    if(sequence[frontIndex + currentLength] == sequence[i - currentLength])
    {
        currentLength++;
    }
    else break;
}
if (currentLength > maxLength) maxLength = currentLength;
currentLength = 0;

This is the entire class

public class Mirrors
{
    public static void main(String[] args)
    {
        int maxLength = 1;
        int currentLength = 0;
        int[] sequence = {1, 2, 1, 2, 1};
        int frontIndex = 0;
        int endIndex = sequence.length - 1;
        for( ; frontIndex < endIndex; frontIndex++)
        {
            for(int i = endIndex; i > frontIndex; i--)
            {
                if(sequence[frontIndex] == sequence[i])
                {
                    currentLength++;
                    while(frontIndex + currentLength < sequence.length && i - currentLength >= 0)
                    {
                        if(sequence[frontIndex + currentLength] == sequence[i - currentLength])
                        {
                            currentLength++;
                        }
                        else break;
                    }
                    if (currentLength > maxLength) maxLength = currentLength;
                    currentLength = 0;
                }
            }
        }
        System.out.print("Max length is " + maxLength);
    }
}


Flight booking system I'm attempting to improve my OOP design skills through solving sample problems. Below is an implementation of a flight booking system where the passenger can only request a seat class, but no number. Also, flight statistics must be printed.

In your review, please focus on design: How can I design the classes better (or design better classes) to lower coupling, increase cohesion and maintainability?

One source of 'developer anxiety' I get is when I send arguments to an instance method that mutate both the instance and the argument. For example, Passenger.book() mutates both the Passenger and Flight instances at the same time. Is this bad design?

In case you are wondering, this is not homework.

#!/usr/bin/env python3
import random

letters = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'

class BookingClass:
    ECONOMY = 'Economy'
    BUSINESS = 'Business'

class Seat:
    def __init__(self, row_number: int, letter: str):
        self.row_number = row_number
        self.letter = letter

    def __str__(self):
        return '{}{}'.format(self.row_number, self.letter)

class SeatingArea:
    def __init__(self, booking_class: str, start_row: int, row_count: int, seats_per_row: int):
        self.booking_class = booking_class
        self.seat_count = row_count * seats_per_row
        self.seats_remaining = []

        for row_number in range(start_row, row_count + start_row):
            for seat_number in range(seats_per_row):
                new_seat = Seat(row_number, letters[seat_number])
                self.seats_remaining.append(new_seat)

class Flight:
    def __init__(self, economy_seats: SeatingArea, business_seats: SeatingArea):
        # should I use the SeatingArea.booking_class attribute as the key?
        self.seating_areas = {
            BookingClass.ECONOMY: economy_seats,
            BookingClass.BUSINESS: business_seats}

    def print_statistics(self):
        for booking_class, seating_area in self.seating_areas.items():
            print('{}: {}% available'.format(
                booking_class,
                len(seating_area.seats_remaining) / seating_area.seat_count * 100))

    def remaining_seat_count(self, booking_class: str):
        return len(self.seating_areas[booking_class].seats_remaining)

    def get_seat(self, booking_class: str):
        return self.seating_areas[booking_class].seats_remaining.pop()

class Passenger:
    def __init__(self, name: str):
        self.name = name
        self.booking_id = None
        self.seat = None

    def has_booked(self):
        return self.booking_id is not None

    def book(self, flight: Flight, booking_class: str):
        if flight.remaining_seat_count(booking_class) != 0:
            self.seat = flight.get_seat(booking_class)
            self.booking_id = random.Random().randrange(1000, 10000)
            return True

        return False

# Example usage
def main():
    business = SeatingArea(BookingClass.BUSINESS, start_row=1, row_count=5, seats_per_row=4)
    economy = SeatingArea(BookingClass.ECONOMY, start_row=6, row_count=20, seats_per_row=6)

    flight = Flight(economy, business)

    john = Passenger('John')
    john.book(flight, BookingClass.ECONOMY)
    print(john.seat)
    print(john.booking_id)

    flight.print_statistics()

if __name__ == '__main__':
    main()

Your hardcoded letters is easier to take from string (just in case the alphabet ever changes or to save you some typing in some future code you might write):

from string import ascii_uppercase as letters

I don't see the advantage of

class BookingClass:
    ECONOMY = 'Economy'
    BUSINESS = 'Business'
...
    self.seating_areas = {
            BookingClass.ECONOMY: economy_seats,
            BookingClass.BUSINESS: business_seats}

over

self.seating_areas = {
        "economy": economy_seats,
        "business: business_seats}

The __str__ of Seat could be slightly simplified:

def __str__(self):
    return '{0.row_number}{0.letter}'.format(self)

In the constructor of SeatingArea you don't need the intermediate variable:

    for row_number in range(start_row, row_count + start_row):
        for seat_number in range(seats_per_row):
            self.seats_remaining.append(Seat(row_number, letters[seat_number]))

Assigning the booking ID like this:

self.booking_id = random.Random().randrange(1000, 10000)

will mean a roughly a 1:9000 chance of a collision. I don't know how many customers this is supposed to deal with, but that does not seem too unrealistic for e.g. a year of running. Maybe give the Flight a name and date/time like this and use that together with the seat and hash it with md5 to get a more unique id:

from datetime import datetime
from hashlib import md5
...

class Flight:
    def __init__(self,
                 path: str,
                 date: datetime,
                 economy_seats: SeatingArea,
                 business_seats: SeatingArea):
        ...
        self.path = path
        self.date = date

...

class Passenger:
    ...
    def book(self, flight: Flight, booking_class: str):
        ...
        id_str = "{0.path}{0.date}{1.seat}".format(flight, self)
        self.booking_id = md5(id_str.encode()).hexdigest()

...

    flight_time = datetime.strptime("2016-08-12 8:00:00 -0800" "%Y-%m-%d %H:%M:%S %z")
    flight = Flight("LAX->JFK", flight_time, economy, business)

This way every seat on every flight between two airports for every different starting time has a different id (at least the chance of collisions for md5 are a lot less than 1:9000).

In reality you would probably also have a Plane class, which has the SeatingArea's as parameters and just pass that on to Flight as the plane servicing that particular flight.


String parsing/stripping code I am mostly interested in how the parse_string function could be improved.  But any comments on my test strategy also welcome.

The input is a string which can be either all digits in which case the result should be the digits string OR there can be string formatting outside angle brackets which must be removed.  the digit string is enclosed within the angle brackets in that case.

#include <stdio.h>
#include <string.h>

char* parse_string(const char* s, char* parsed) {
   /* sample string for angle brackets */
   const char* p = s;
   int lbrack = -1, rbrack = -1;
   while(*p) {
      if(*p == '<' && lbrack == -1)  /* grab first < */
         lbrack = p - s;
      else if(*p == '>') /* and last > char */
         rbrack = p - s;

      ++p;
   }

   /* strip everything BEFORE < and AFTER > if have brackets and in correct order */
   if(lbrack != -1 && rbrack != -1 && lbrack < rbrack) {
      memcpy(parsed, s+lbrack+1, rbrack-1-lbrack);
      parsed[rbrack-1-lbrack] = '\0';
   }
   else {
      strcpy(parsed, s ? s : "");
   }

   return parsed;
}

int main() {

   char buf[100];
   /* 4 normal cases, error cases  */
   const char* input[] = { "N<737867>Sales Line 1", "123456", "", "<123>", "<", ">", "", "<<<<123>", "<123>>>>", "<123", "123>" };
   const char* output[] = { "737867", "123456", "", "123", "<", ">", "", "<<<123", "123>>>", "<123", "123>" };
   size_t sz = sizeof(input) / sizeof(input[0]);

   size_t i;
   for(i = 0; i < sz; ++i) {
      printf("Test string \"%s\" => \"%s\", %s\n", input[i], output[i], strcmp(output[i], parse_string(input[i], buf)) == 0 ? "passed" : "failed");
   }
   return 0;
}

Add a buffer length parameter and then use strncpy to avoid overflow.
Your testing code hides the usage of the function. It's clearer to put it on a separate line:

for(i = 0; i < sz; ++i) {
    parse_string(input[i], buf)
    printf("Test string \"%s\" => \"%s\", %s\n",
                 input[i], output[i], strcmp(output[i], buf) == 0 ? "passed" : "failed");
}

You missed the out of order cases like >123<


ASP.NET MVC return early from an action that returns a model Consider the following code segment written by a coworker of mine:

public ActionResult Index()
    {

        DataRetrieveModel dataRetrieveModel = new DataRetrieveModel();

        using (spc_web_trunkEntities db = new spc_web_trunkEntities())
        {
            DataRetrieveDAL dataRetrieveDAL = new DataRetrieveDAL(db);

            dataRetrieveModel.CustomerList = dataRetrieveDAL.GetCustomerList();

            dataRetrieveModel.LineList = dataRetrieveDAL.GetLineList();
            if (dataRetrieveModel.LineList.Count > 0)
            {
                dataRetrieveModel.ModelList = dataRetrieveDAL.GetModelList(dataRetrieveModel.LineList[0].Value);
            }

            if (dataRetrieveModel.LineList.Count > 0 &&
                dataRetrieveModel.ModelList.Count > 0)
            {
                dataRetrieveModel.LotList = dataRetrieveDAL.GetLotList(dataRetrieveModel.LineList[0].Value, dataRetrieveModel.ModelList[0].Value);
            }

            if (dataRetrieveModel.LineList.Count > 0 &&
                dataRetrieveModel.ModelList.Count > 0)
            {
                dataRetrieveModel.EquipmentList = dataRetrieveDAL.GetEquipmentList(dataRetrieveModel.LineList[0].Value, dataRetrieveModel.ModelList[0].Value);
            }
        }

        return View(dataRetrieveModel);
    }

It's a very simple controller that returns a model.

My question is given this code, how would you go about cleaning/refactoring it?

Here's my approach. First, I identified a lot of short circuiting points along with empty spaces, so I went ahead and cleaned those up first:

public ActionResult Index()
    {
        DataRetrieveModel dataRetrieveModel = new DataRetrieveModel();

        using (spc_web_trunkEntities db = new spc_web_trunkEntities())
        {
            DataRetrieveDAL dataRetrieveDAL = new DataRetrieveDAL(db);

            dataRetrieveModel.CustomerList = dataRetrieveDAL.GetCustomerList();
            dataRetrieveModel.LineList = dataRetrieveDAL.GetLineList();
            if (!dataRetrieveModel.LineList.Any())
                return View(dataRetrieveModel);

            dataRetrieveModel.ModelList = dataRetrieveDAL.GetModelList(dataRetrieveModel.LineList[0].Value);

            if (!dataRetrieveModel.ModelList.Any())
                return View(dataRetrieveModel);

            dataRetrieveModel.LotList = dataRetrieveDAL.GetLotList(dataRetrieveModel.LineList[0].Value, dataRetrieveModel.ModelList[0].Value);
            dataRetrieveModel.EquipmentList = dataRetrieveDAL.GetEquipmentList(dataRetrieveModel.LineList[0].Value, dataRetrieveModel.ModelList[0].Value);
        }

        return View(dataRetrieveModel);
    }

I'm also wondering why multiple queries have to be made to initialize dataRetrieveModel, but for now, I only want to focus on this method.

But then all those return View(dataRetrieveModel) seems too verbose, so I thought about defining a return label and using a goto. I'm not sure how others feel about it.

I'd like to hear any suggestions on this matter.

I will review the first method.

Code duplication

I see 3 times checking for dataRetrieveModel.LineList.Count > 0 and 2 times checking for dataRetrieveModel.ModelList.Count > 0 where Count > 0 should be changed to .Any() (thanks to @krillgar) and then be refactored to

if (dataRetrieveModel.LineList.Any())
{
    dataRetrieveModel.ModelList = dataRetrieveDAL.GetModelList(dataRetrieveModel.LineList[0].Value);

    if (dataRetrieveModel.ModelList.Any())
    {
        dataRetrieveModel.LotList = dataRetrieveDAL.GetLotList(dataRetrieveModel.LineList[0].Value, dataRetrieveModel.ModelList[0].Value);
        dataRetrieveModel.EquipmentList = dataRetrieveDAL.GetEquipmentList(dataRetrieveModel.LineList[0].Value, dataRetrieveModel.ModelList[0].Value);
    }
}

This still uses 2 times dataRetrieveModel.LineList[0].Value and dataRetrieveModel.ModelList[0].Value so let's refactor again and add 2 vars holding these two values

if (dataRetrieveModel.LineList.Any())
{
    dataRetrieveModel.ModelList = dataRetrieveDAL.GetModelList(dataRetrieveModel.LineList[0].Value);

    if (dataRetrieveModel.ModelList.Any())
    {
        var lineListValue = dataRetrieveModel.LineList[0].Value;
        var modelListValue = dataRetrieveModel.ModelList[0].Value;

        dataRetrieveModel.LotList = dataRetrieveDAL.GetLotList(lineListValue , modelListValue);
        dataRetrieveModel.EquipmentList = dataRetrieveDAL.GetEquipmentList(lineListValue , modelListValue);
    }
}

The remaining part of this method I would just keep it like it is, because it is more readable than yours and also uses only one return point at the end.
Looking at your approach, I would suggest either adding {} brackets at your if statements, or at least write them in one line.


Given a string of brackets, return the count of brackets that are properly matched A string of brackets is considered correctly matched if every opening bracket in the string can be paired up with a later closing bracket, and vice versa. For instance, “(())()” is correctly matched, whereas “)(“ and “((” aren’t. For instance, “((” could become correctly matched by adding two closing brackets at the end, so you’d return 2.

Given a string that consists of brackets, write a function bracketMatch that takes a bracket string as an input and returns the minimum number of brackets you’d need to add to the input in order to make it correctly matched.

My approach:

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

 class Solution {

  static int bracketMatch(String text) {
    // your code goes here
    //Maintain 2 variables
    int ob = 0;//ob
    int cb = 0;//cb
    int wb= 0;//wb

    for( int i = 0; i < text.length(); i++ )
      {
          //Check for openBracket
          if( text.charAt(i) == '(')
             ob++;

          //Check for closeBracket
          if( text.charAt(i) == ')')
             cb++;

          //Check if closeBracketct > openBracketct
          if( cb > ob )
          {
            wb++;
            ob = 0;
            cb = 0;
          }
      }
    return (wb + (ob - cb));
  }

  public static void main(String[] args) {
    //System.out.println(bracketMatch("())("));
    //System.out.println(bracketMatch("(((())))"));
    System.out.println(bracketMatch("(((())"));
  //  System.out.println(bracketMatch("))))"));
  }

}

I have a few questions regarding the above code:

1) How can I further optimize this code?

2) Am I using too many variables?

3) Is there a better approach than mine?

Reference

We dont need close variable if we can decrease open by 1 when close bracket is found.

public class MatchBrackets{

    public static int matchBrackets(String brackets){

      int open = 0;
      int count =0;

     for(int i=0;i<brackets.length();i++){
        if(brackets.charAt(i)=='('){
        open++;
        }else if(brackets.charAt(i)==')'){
         open--;
        }
        if(open<0){
        count++;
        open++;
     }
  }
  return count+open;
}

  public static void main(String[] args) {

  System.out.println(matchBrackets("())("));
  System.out.println(matchBrackets("(((())))"));
  System.out.println(matchBrackets("))))"));
}}


Merge two list of dicts with same key I have two lists of dictionaries. Both lists always have the same ids.
I want to get the following result:

{9: {'av': 4, 'nv': 45}, 10: {'av': 0, 'nv': 0}, 8: {'av': 0, 'nv': 30}}

The following code is working but I wonder if this is the best solution?

l1 = [{'id': 9, 'av': 4}, {'id': 10, 'av': 0}, {'id': 8, 'av': 0}]

l2 = [{'id': 9, 'nv': 45}, {'id': 10, 'nv': 0}, {'id': 8, 'nv': 30}]

l3 = {x['id']: {'av': x['av']} for x in l1}
l4 = {x['id']: {'nv': x['nv']} for x in l2}

{key: value.update(l4[key]) for key, value in l3.items()}

>> {9: {'av': 4, 'nv': 45}, 10: {'av': 0, 'nv': 0}, 8: {'av': 0, 'nv': 30}}

Your approach is rather good but your implementation is hardly extensible.

For starter, you don't need l4 as you can update l3 directly instead:

l3 = {x['id']: {'av': x['av']} for x in l1}
for d in l2:
    l3[d['id']].update(nv=d['nv'])

Second, you can pop the id so you don't have to know the other keys in the various dictionaries:

l3 = {d.pop('id'): d for d in l1}
for d in l2:
    l3[d.pop('id')].update(d)

But this approach has the drawback of modifying all input dictionaries. To mitigate that, we can start with an empty dictionary, update with every keys (including id) and pop the extra key afterwards. This is easily done using a defaultdict:

from collections import defaultdict

result = defaultdict(dict)
for sequence in (l1, l2):
    for dictionary in sequence:
        result[dictionary['id']].update(dictionary)
for dictionary in result.values():
    dictionary.pop('id')

There are few overheads using this approach compared to the first version, but it is way easier to generalize so you are able to merge more than 2 lists. Speaking of which, in such case, you should define a function taking a variable number of lists to merge:

import itertools
from collections import defaultdict

def merge(shared_key, *iterables)
    result = defaultdict(dict)
    for dictionary in itertools.chain.from_iterable(iterables):
        result[dictionary[shared_key]].update(dictionary)
    for dictionary in result.values():
        dictionary.pop(shared_key)
    return result

Usage be like

merge('id', l1, l2)


Fluent Interface for a XmlQueryBuilder In this answer I suggested using a fluent interface "builder" to replace all the hard-coded, repetitive and quite error-prone inline XML string concatenations.

This code might need a bit of tweaking to work perfectly with the OP's code in the question this code is following-up on, but it seems to work as expected. Thoughts?

public enum XmlQueryOperator
{
    Equals,
    LessThan,
    LessThanOrEqual,
    GreaterThan,
    GreaterThanOrEqual
}

public class XmlQueryBuilder<TEntity>
    where TEntity : class
{
    private readonly XDocument _xDoc = new XDocument();
    private readonly XElement _queryNode = new XElement("query");

    public XmlQueryBuilder()
    {
        var rootNode = new XElement("queryxml");
        rootNode.SetAttributeValue("version", "1.0");

        var entityNode = new XElement("entity");
        entityNode.Value = typeof (TEntity).Name;

        rootNode.Add(entityNode);
        _xDoc.Add(rootNode);
    }

    public XmlQueryBuilder<TEntity> Where<TProperty>(Expression<Func<TEntity, TProperty>> property, XmlQueryOperator operation, string value)
    {
        var xCondition = new XElement("condition");
        var xField = new XElement("field");
        xField.Value = GetPropertyName(property);

        var xExpression = new XElement("expression");
        xExpression.SetAttributeValue("op", operation.ToString());
        xExpression.Value = value;

        xField.Add(xExpression);
        xCondition.Add(xField);

        _queryNode.Add(xCondition);
        return this;
    }

    public override string ToString()
    {
        var parent = _xDoc.Element("queryxml");
        if (parent == null) throw new InvalidOperationException("root node was not created.");

        parent.Add(_queryNode);
        return _xDoc.ToString();
    }

    private string GetPropertyName<TSource, TProperty>(Expression<Func<TSource, TProperty>> propertyLambda)
    {
        // credits http://stackoverflow.com/questions/671968/retrieving-property-name-from-lambda-expression/672212#672212 (modified to return property name)
        var type = typeof(TSource);

        var member = propertyLambda.Body as MemberExpression;
        if (member == null)
            throw new ArgumentException(string.Format("Expression '{0}' refers to a method, not a property.", propertyLambda));

        var propInfo = member.Member as PropertyInfo;
        if (propInfo == null)
            throw new ArgumentException(string.Format("Expression '{0}' refers to a field, not a property.", propertyLambda));

        if (type != propInfo.ReflectedType && !type.IsSubclassOf(propInfo.ReflectedType))
            throw new ArgumentException(string.Format("Expresion '{0}' refers to a property that is not from type {1}.", propertyLambda, type));

        return propInfo.Name;
    }
}

Usage - given a TestEntity:

public class TestEntity
{
    public string Test1 { get; set; }
    public int Test2 { get; set; }
    public DateTime Test3 { get; set; }
}

This program produces the expected output:

class Program
{
    static void Main(string[] args)
    {
        var builder = new XmlQueryBuilder<TestEntity>()
            .Where(t => t.Test1, XmlQueryOperator.Equals, "abc")
            .Where(t => t.Test2, XmlQueryOperator.GreaterThan, "def")
            .Where(t => t.Test3, XmlQueryOperator.LessThanOrEqual, DateTime.Now.ToString());

        Console.WriteLine(builder.ToString());
        Console.ReadLine();
    }
}

Output:

<queryxml version="1.0">
  <entity>TestEntity</entity>
  <query>
    <condition>
      <field>Test1<expression op="Equals">abc</expression></field>
    </condition>
    <condition>
      <field>Test2<expression op="GreaterThan">def</expression></field>
    </condition>
    <condition>
      <field>Test3<expression op="LessThanOrEqual">2014-05-05 14:14:57</expression></field>
    </condition>
  </query>
</queryxml>

ToString() does too many things, it should only be calling _xDoc.ToString(); it has side-effects that make it dangerous to call more than once:

var foo = builder.ToString()
Console.WriteLine(builder.ToString());
Console.ReadLine();

Will not produce the expected output, and will not throw an exception either, resulting in an XML document with _queryNode showing up as many times as ToString() was called.


Longest Collatz sequence in Python I came across this problem:

  The following iterative sequence is defined for the set of positive
  integers:

    n → n/2 (n is even)
    n → 3n + 1 (n is odd)

  Using the rule above and starting with 13, we generate the following
  sequence:

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

  It can be seen that this sequence (starting at 13 and finishing at 1)
  contains 10 terms. Although it has not been proved yet (Collatz
  Problem), it is thought that all starting numbers finish at 1.

  Which starting number, under one million, produces the longest chain?

and to this I gave this solution:

def number(n):
    ctr = 1
    if n % 2 == 0:
       k = n / 2
       ctr += 1
    else:
       k = 3 * n + 1
       ctr += 1
    while k > 1:
       if k % 2 == 0:
           k /= 2
           ctr += 1
       else:
           k = k * 3 + 1
           ctr += 1
    return ctr

def k(upto):
    largest_number=0
    last_ctr=0
    for num in range(1,upto):
         if number(num)>last_ctr:
            last_ctr=number(num)
            largest_number=num
    return largest_number

print k(999999)

I am new to Python and don't think this is a  good solution to this problem and  maybe its complexity is \$\mathcal{O}(n^2)\$. So what is the best code for this problem if the number is really huge? How do I reduce the complexity?

And yes I read other questions related to this topic but they were not satisfactory answers to this in Python.

The key to solving this problem (and many others) is using memoization: The numbers of the collatz sequence converge in a kind of tree-like structure. Once you have calculated the value of, say collatz(100) on one path, you do not need to calculate it again when you encounter it again on another path.

One way to do this would be to have (a) a cache and (b) a list of all the numbers on the current path. As soon as you encounter a number that is already in the cache, go your path backwards and add all the numbers to the cache.

cache = {1: 1}
def collatz(n):
    path = [n]
    while n not in cache:
        if n % 2:
            n = 3 * n + 1
        else:
            n = n / 2
        path.append(n)
    for i, m in enumerate(reversed(path)):
        cache[m] = cache[n] + i
    return cache[path[0]]

Another, maybe more elegant way would be using recursion and a decorator function. You can define a @memo decorator and use it again and again for all sorts of function you want to apply memoization to. This decorator automatically replaces your function with a version that looks in the cache, first.

def memo(f):
    f.cache = {}
    def _f(*args):
        if args not in f.cache:
            f.cache[args] = f(*args)
        return f.cache[args]
    return _f

@memo
def collatz(n):
    if n == 1:
        return 1
    if n % 2 == 0:
        return 1 + collatz(n / 2)
    if n % 2 == 1:
        return 1 + collatz(3 * n + 1)

In both cases, call like this and after a few seconds you should get your result.

print(max(range(1, 10**6), key=collatz))


Multiple overloads for message handling We have a base interface:

        public interface IMessage
        {
            //Some properties
        }

then we have derived messages that implement this interface such as:

        public class AlarmEventMessage: IMessage
        {   //Some properties  }

Now, these messages get routed to a message handler that receives them.
So we have multiple overloads for this Handle(IMessage message) to handle the specific derived messages.

        public class MessageHandler
        {
             public bool Handle(IMessage message)
             {
                 if (message is AlarmEventMessage)
                 {
                     return Handle(message as AlarmEventMessage);
                 }
                 // some more message routing here
             }

             public bool Handle(AlarmEventMessage message)
             {
                 // Some business logic here
             }

             //Handle more messages here
        }

I believe it violates the Single-Responsibility Principle.  This class constantly keeps changing because of new messages being added and also the business logic that relates with this new message.

Are there any other SOLID principles it violates? Does it look like an Anti-Pattern?

You are correct when you say that the MessageHandler class violates the Single Responsibility Principle.

What you're looking for here is generics. First of all, you need to define a contract for messages and their respective handlers. This can be done with the following:

public interface IMessage
{
    // properties
}

public interface IMessageHandler<in TMessage>
    where TMessage : IMessage
{
    bool Handle(TMessage message);
}

So with the AlarmEventMessage, you can do this:

public class AlarmEventMessage : IMessage
{
    // properties
}

public class AlarmEventMessageHandler : IMessageHandler<AlarmEventMessage>
{
    public bool Handle(AlarmEventMessage message)
    {
        // stuff
    }
}

This design allows you to do some cool stuff with decorator chains, dispatching, registration by convention (if you're using an IoC container that supports it), among other things. For more information, check out the following two articles (they demonstrate how to implement the command/query pattern, but the design is identical):

Meanwhile... on the command side of my architecture
Meanwhile... on the query side of my architecture


Arduino: Change the color & color intensity of a LED periodically Prerequisites:

A RGB-LED is attached and wired to the Arduino Uno-board.

Process description:

In the beginning the LED is turned off.

A loop starts and an output-signal is put on the red-part of the LED.

With every iteration the output-signal becomes increased in the following way: 0, 1, 2, 3, ...

The loop stops after 255 (maximal red) is reached. After a delay the output-signal becomes decreased (by using a loop again) in the way: 255, 254, 253 ... until 0 is reached.

Then the increasing and decreasing starts again but this time with the green-part of the LED.

After the blue-part of the LED has been increased and decreased the whole process starts again with the red-part.

GitHub-README with Image of the board-setup

Implementation:

   // -------- Declarations --------------
const int GREEN_LED_PIN = 9;
const int RED_LED_PIN = 11;
const int BLUE_LED_PIN = 10;
const int DELAY_WITHIN_LOOP = 10;
// ---------------------------------------

// ------- Own Functions -----------------
void setLed(int valRed, int valGreen, int valBlue) {
  analogWrite(RED_LED_PIN, valRed);
  analogWrite(GREEN_LED_PIN, valGreen);
  analogWrite(BLUE_LED_PIN, valBlue);
}

void changeColorIntensity(int position, int step, int i, int stop) {
  while (i != stop) {
    switch (position) {
      case 0:
        setLed(i, 0, 0);
        break;
      case 1:
        setLed(0, i, 0);
        break;
      case 2:
        setLed(0, 0, i);
        break;
    }

    i += step;
    delay(DELAY_WITHIN_LOOP);
  }
}
// -------------------------------------------

void setup() {
  pinMode(GREEN_LED_PIN, OUTPUT);
  pinMode(RED_LED_PIN, OUTPUT);
  pinMode(BLUE_LED_PIN, OUTPUT);
}

void loop() {
  for (int i = 0; i < 3; i++) {
    changeColorIntensity(i, 1, 0, 256);
    changeColorIntensity(i, -1, 255, -1);
  }
}

The sketch works just fine and solves exactly the described task.

What annoys me is the part with the switch-statement and the multiple function-invocations.

If someone has an idea how to solve that part then his/her answer would be very appreciated.

Generally: What would you have done different and why?

Looking forward to reading your comments, recommendations and hints!

I agree that the switch statement smells. One way to avoid it is to abstract LED values into the array, and let change_color_intensity operate on it:

void change_color_intensity(int which, int start, int stop, int delta)
{
    int values[] = { 0, 0, 0 };
    for (values[which] = start; values[which] != stop; values[which] += delta) {
        set_leds(values[0], values[1], values[2]);
        delay(DELAY_WITHIN_LOOP);
    }
}

Note that I change a while loop into a IMHO more readable for. I also changed some names for the same reason.

Another approach is to abstract LED names, and get rid of setLed altogether:

void change_color_intensity(int which, int start, int stop, int delta)
{
    for (in value = start; value != stop; value += delta) {
        analogWrite(which, value);
        delay(DELAY_WITHIN_LOOP);
    }
}

with the caller defining a pin array:

    int LED = { RED_LED_PIN, GREEN_RED_PIN, BLUE_RED_PIN };
    for (pin = 0; pin < 3; pin++) {
        change_color_intensity(LED[pin], 0, 256, 1);
        ....
    }

I am not sure I understand what you mean by multiple function invocations. If you are concerned with invoking change_color_intensity twice in a loop, I see no problem with it.


Hacker Rank Challenge : Find count of substrings which are special palindrome I am getting timeout for test cases having very large input.

-= Challenge Link =-

-= Test case example =-

Code is working fine as can be checked on ideone. (Note: Here fout has been replaced with cout.)

  Problem Statement :

  A string is said to be a special palindromic string if either of two
  conditions is met:

  All of the characters are the same, e.g. aaa.
  All characters except the middle one are the same, e.g. aadaa.

  A special palindromic
  substring is any substring of a string which meets one of those
  criteria. Given a string, determine how many special palindromic
  substrings can be formed from it.

  For example, given the string s = mnonopoo , we have the following special
  palindromic substrings: {m, n, o, n, o, p, o, o, non, ono, opo, oo}.

  Function Description

  Complete the substrCount function in the editor below. It should
  return an integer representing the number of special palindromic
  substrings that can be formed from the given string.

  substrCount has the following parameter(s):

  n: an integer, the length of string s
  s: a string

  Input Format

  The first line contains an integer, n , the length of s.  The second line
  contains the string .

  Constraints

  1 ≤ n ≤ 10^6

  Each character of the string is a lowercase alphabet, ascii[a-z].

  Output Format

  Print a single line containing the count of total special palindromic
  substrings.

  Sample Input 0

5
asasd

  Sample Output 0

7

  Explanation 0

  The special palindromic substrings of s = asasd are {a, s, a, s, d, asa, sas}.

  Sample Input 1

7
abcbaba

  Sample Output 1

10

  Explanation 1

  The special palindromic substrings of s = abcbaba are {a, b, c, b, a, b, a, bcb, bab, aba}.

  Sample Input 2

4
aaaa

  Sample Output 2

10

  Explanation 2

  The special palindromic substrings of s = aaaa are {a, a, a, a, aa, aa, aa, aaa, aaa, aaaa}.

Code:

#include <bits/stdc++.h>

using namespace std;

// Complete the substrCount function below.
long substrCount(int n, string s) {

    long int length_sub = 2;
    long int count = n;
    while(length_sub <= n){
        for(long int i = 0; i <= n - length_sub ; i++){
            string sub = s.substr(i, length_sub);
            //cout << sub << " ";
            string rev_sub(sub);
            reverse(rev_sub.begin(), rev_sub.end());
            //cout << rev_sub;;
            char c = sub[0];
            int flag = 0;
            for(long int j = 0; j < sub.length() / 2; j++){
                if(sub[j] != c || rev_sub[j] != c){
                    flag = 1;
                    break;
                }
            }
            if(flag == 0){
                //cout << " - Special\n";
                count++;
            }
            // else{
            //     cout << "\n";
            // }
        }
        length_sub++;
    }
    return count;

}

int main()
{
    ofstream fout(getenv("OUTPUT_PATH"));

    int n;
    cin >> n;
    cin.ignore(numeric_limits<streamsize>::max(), '\n');

    string s;
    getline(cin, s);

    long result = substrCount(n, s);

    fout << result << "\n";

    fout.close();

    return 0;
}

This

#include <bits/stdc++.h>
using namespace std;

is given by the submission template from HackerRank (so it is not your fault),
but note that both lines are generally considered as bad practice.
See for example

Why should I not #include <bits/stdc++.h>?
Why is “using namespace std” considered bad practice?

on Stack Overflow.

With respect to your

long substrCount(int n, string s)

function:

The outer while loop could be made a for loop as well – why should it
be different from the inner for(long int ...) loop?
The proper data type for string lengths and indices is string::size_type
aka size_t.

One step to increase the efficiency would be to avoid the creation (and reversal)
of the additional strings sub and rev_sub. All tests can be done directly on
the original string s. As an example,

if (sub[j] != c || rev_sub[j] != c)

is equivalent to

if (s[i + j] != c || s[i + length_sub - 1 - j] != c)

Your method checks all \$ n (n-1)/2 \$ substrings of length 2 or more if
it is a “special palindromic string.”. The following approach seems more
efficient to me:

The number of substrings with all identical characters (e.g. "aaa")
can be determined with a single traversal of the string. All sequences
of \$ k \$ contiguous identical characters contain \$ k(k+1)/2 \$
substrings with identical characters.
To determine the number of substrings where characters except the middle one
are the same (e.g. "aadaa") traverse the string, and check for each character
(e.g. "d") how many identical characters exist to the left and to the right
of this character.

Example: The string "mnonopoo" has the following sequences of contiguous
identical characters

m n o n o p oo

which gives a total

1+1+1+1+1+1+3 = 9

substrings with all identical characters. The number of substrings where
all characters except the middle one are the same around each character are

m n o n o p o o
0+0+1+1+0+1+0+0 = 3


Recursive Fibonacci with Generic delegates Here is a fast recursive Fibonacci-like for loop. How can it be more readable, and is it possible remove TArgs?

public static class Fibonacci
{
    public static BigInteger Calculate(BigInteger number)
    {
        var fibo = AnonRecursiveFiboFunc<BigInteger>(
            func => (step, fibo1, fibo2) => step == number
            ? fibo2
            : func(++step, fibo1 + fibo2, fibo1));
        return fibo(0, 1, 0);
    }

    delegate Func<TArg1, TArg2, TArg3, TArg4>
        Recursive<TArg1, TArg2, TArg3, TArg4>(
        Recursive<TArg1, TArg2, TArg3, TArg4> r);

    private static Func<TArg, TArg, TArg, TArg>
        AnonRecursiveFiboFunc<TArg>(Func<Func<TArg, TArg, TArg, TArg>,
                                         Func<TArg, TArg, TArg, TArg>> function)
    {
        Recursive<TArg, TArg, TArg, TArg> recursive =
            rec => (step, fibo1, fibo2) =>
                function(rec(rec))(step, fibo1, fibo2);
        return recursive(recursive);
    }
}

I like to say T1 T2 instead of TArg1, it just makes it more clear to me what is going on. Your code would look more like this.

delegate Func<T1, T2, T3, T4> Recursive<T1, T2, T3, T4>( Recursive<T1, T2, T3, T4> r);

private static Func<T, T, T, T> AnonRecursiveFiboFunc<T>(Func<Func<T, T, T, T>, Func<T, T, T, T>> function)
{
    Recursive<T, T, T, T> recursive = rec => (step, fibo1, fibo2) => function(rec(rec))(step, fibo1, fibo2);
    return recursive(recursive);
}

Other than that... this code seems pretty solid, except for the crazy lack of immediate readability :D and like mentioned in the comments the fact that you are using delegates at all here.