Making a function which takes char* exception-proof I should preface this question by saying that I'm a Java programmer working on my first C++ application, so perhaps there are certain obvious things I don't know.

Some background on the problem:

Our application had data read from a socket stored in a character array, which was a fixed length message header, containing various fixed length fields i.e.:

char[15] data;

The field positions were 0-5,6-7,8-10,11-13 (the last char is a delimiter). The class which wrapped this header offered convenience functions to retrieve the values for each field:

    int ApiHeader::msg_size(void)
    {
        return std::atoi(std::string(data, 6).c_str());
    }

    int ApiHeader::msg_type(void)
    {
        return std::atoi(std::string(data,6,2).c_str());
    }

    int ApiHeader::msg_subtype(void)
    {
        return std::atoi(std::string(data,8,3).c_str());
    }

    int ApiHeader::seqno(void)
    {
        return std::atoi(std::string(data,11,3).c_str());
    }

I was trying to leverage the std::atoi which requires a c-string, but I needed to "slice" the character array into the sub-arrays which contained each separate field, which I did by constructing a string built from the positions in the data array for each field.

The code worked fine, but a user who was doing some code analysis pointed that the msg_subtype() function (and the 2 other similar ones), were actually unintentionally creating an extra string object.

They were invoking this constructor:

basic_string(const basic_string& other,
              size_type pos,
              size_type count = std::basic_string::npos,
              const Allocator& alloc = Allocator() );

So the code was first constructing a new string to turn the char[] to a string&, and then it was constructing another string invoking the string(string&, int, int) constructor.

I redesigned the code to be more efficient:

int atoi(char* string, int start, int length)
{
    double result = 0;
    char* index = string + start*sizeof(char);
    for(int i=length;i>0; i--)
    {
        result += (*index++ - '0') * pow((float) 10, (float) (i-1));
    }
    return (int) result;
}

There are no strings constructed, and it just iterates the part of the data array specified.  The problem is that this code requires that there is no way to detect whether the user passed bad values for start and length.  (I'm not exactly sure what happens if user passes a length outside of the array.  In my unit test, I just saw it got an uninitialized value).

What can be done to make this code more exception proof?

Some ideas I have are:

If *index is not within valid range ('0' to '9'), break out of the loop.
Same as above, except throw an Exception (which at least lets user know something went wrong).
Just document the method to tell user that if values are bad, behavior is "undefined" (I have seen some methods documented this way).

What would be the best C++ way of handling this issue? Or is there a completely different approach which should be used for this function?

First a question: why do you care that an extra string is being created?  Is this function used a lot.  If not, then I see no need to change.

But beyond that, although I don't know about C++ cleverness, is your replacement function so much more efficient?  You are calling pow repeatedly and using floating point to do an integer job.  Here is an alternative that would be cheaper.  It returns 0 on success, 1 on failure (bad string) and the number desired is output in result.   Notice that string is const.

int my_atoi(const char* string, int start, int length, long *result)
{
    char copy[length+1];
    memcpy(copy, string + start, length);
    copy[length] = '\0';
    char *end;
    *result = strtol(copy, &end, 0);
    return (*end != '\0');
}

You might also omit the start parameter and call with

my_atoi(string+start, length, &res);


Raising an error if a variable is not created during class initalization I would like to properly raise an error if self._header was not created when I initialize a NewFile object.

This is working now, but perhaps there is more formal way of catching the error below:

class NewFile(object):
    """ Docstring """

    def __init__(self, filename):
        self.localdir = 'C:\\Users\\User\\Documents'
        self.filename = filename
        self.filepath = os.path.join(self.localdir, self.filename)
        self._data = []

        try:
            with open(self.filepath, 'rU') as c:
                rows = csv.reader(c)
                for row in rows:
                    row[:] = [r.lower().strip() for r in row]

                    if row[0].startswith('name'):
                        self._header = row     # Create Header Variable

                    self._data.append(row)

        except IOError:
            raise IOError(
                '"{}" not found in "{}"'.format(self.filename, self.localdir))

        # I would like to raise error if "self._header" was not created
        try:
            self._header
        except AttributeError:
            raise AttributeError(
                'No Header not found in "{}"'.format(self.filename))

def main():
    f = 'samplefile.csv'
    new = NewFile(f)

if __name__ == '__main__':
    main()

You got the arrow anti-pattern:

    try:
        with open(self.filepath, 'rU') as c:
            rows = csv.reader(c)
            for row in rows:
                row[:] = [r.lower().strip() for r in row]

                if row[0].startswith('name'):
                    self._header = row     # Create Header Variable

                self._data.append(row)

    except IOError:
        raise IOError(
            '"{}" not found in "{}"'.format(self.filename, self.localdir))

4 levels of nesting..., instead, perform exception handling before:

try:
    with open(self.filepath, 'rU') as c:
        rows = csv.reader(c)
except IOError:
    raise IOError(
       '"{}" not found in "{}"'.format(self.filename, self.localdir))

Data processing comes later, so now you have 2 levels of nesting before and 2 levels of nesting after, that is much better than 4 levels of nesting in one place.


Integer to English challenge I have a challenge, which is to create a JavaScript function that turns a given number into the string representation. For example:

console.log(inToEnglish(15)) should print fifteen

console.log(inToEnglish(101)) should print one hundred one

and so on...

This challenge covers Non-Negative, greater than zero Integer numbers.

I have accomplished this objective with the following code:

var b4Twenty = ["one", "two", "three", "four", "five", "six", "seven", "eight",
    "nine", "ten", "eleven", "twelve", "thirteen", "fourteen", "fifteen", "sixteen",
    "seventeen", "eighteen", "nineteen"
];

var b4Hundred = ["twenty", "thirty", "forty", "fifty", "sixty", "seventy",
    "eighty", "ninety"
];

function intToEnglish(n) {
    return translator(n).trim();
}

function translator(n) {
    if( n == 0)
        return "";
    else if (n <= 19)
        return b4Twenty[n - 1] + " ";
    else if (n <= 99)
        return b4Hundred[Math.floor(n / 10 - 2)] + " " + translator(n % 10);
    else if (n <= 199)
        return "one hundred " + translator(n % 100);
    else if (n <= 999)
        return translator(Math.floor(n / 100)) + "hundred " + translator(n % 100);
    else if (n <= 1999)
        return "one thousand " + translator(n % 1000);
    else if (n <= 999999)
        return translator(Math.floor(n / 1000)) + "thousand " + translator(n % 1000);
    else if (n <= 1999999)
        return "one million " + translator(n % 1000000);
    else if (n <= 999999999)
        return translator(Math.floor(n / 1000000)) + "million " + translator(n % 1000000);
    else if (n <= 1999999999)
        return "one billion " + translator(n % 1000000000);
    else if (n <= 999999999999)
        return translator(Math.floor(n / 1000000000)) + "billion " + translator(n % 1000000000);
    else if (n <= 1999999999999)
        return "one trillion " + translator(n % 1000000000000);
    else if(n <= 999999999999999)
        return translator(Math.floor(n / 1000000000000)) + "trillion " + translator(n % 1000000000000);
    else if (n <= 1999999999999999)
        return "one quadrillion " + translator(n % 1000000000);
    else
        return translator(Math.floor(n / 1000000000000000)) + "quadrillion " + translator(n % 1000000000000000);
}

This is my recursive function to achieve the given goal. It works, and it takes 65ms to complete the battery of 50 tests.

However, I have some concerns regarding its performance:

Given that recursive functions are usually slower than iterative ones, is there a way to make this iterative?
Should I use a switch case, or is my if statement OK?
I keep using Math.floor to round the numbers and find the indexes. Perhaps there is a better solution out there without using this technique?
I have to wrap the translator function (who does all the work) into another function because I need to trim the final result of extra spaces. Is there a way I can avoid this?

I am open to suggestions on how to improve this. Thanks!

intToEnglish(-1) returns "undefined", which might not be what you want. Same goes for fractional values, although I guess that case could be waved... if you only support integers. Even so, you might want to add a check. At the very least, add a comment - problem descriptions are kept separate from your code right now, and that makes it non-obvious that it doesn't support such things!

Negative numbers could be supported via prepending "minus", then translator(Math.abs(n)).


Extract logic applied in a sequence of integers I've done it in C# before but thought that a functional language would be a good fit for this kind of thing.

From this ascending list of integers:

  [100,103,106,112,118,121]

I had to calculate the difference between two consecutive numbers and group them while the result doesn't change. The final output must be a string like this:

  "(group.length x difference)+(next group.length + next diff)+.."

Note that a same difference can appear multiple times in the sequence if it's not consecutive.

For this sample: "2x3+2x6+1x3"

import Data.List

countDelta :: [[a]] -> [(Int,a)]
countDelta [] = []
countDelta [[]] = []
countDelta ( (y:zs) : (bs) ) =  (length zs + 1, y) : countDelta bs

groupByDelta :: [Int] -> [(Int,Int)]
groupByDelta (xs) = countDelta ( group [b-a | (a:b:_) <- tails xs] )

printTuples :: [(Int, Int)] -> String
printTuples [] = ""
printTuples [(x,y)] = show x ++ "x" ++ show y
printTuples (x:xs) = printTuples [x] ++ "+" ++ printTuples xs

getSequenceLogic :: [Int] -> String
getSequenceLogic [] = ""
getSequenceLogic [x] = printTuples [(1,0)]
getSequenceLogic (xs) = printTuples (groupByDelta xs)

Sample call:

getSequenceLogic [100,103,106,112,118,121]

I get the expected result but is there any obvious error?

I managed to simplify a pair of your functions:

countDelta

countDelta = map (\(x:xs) -> (length xs + 1, x))

In fact most of the logic in your countDelta was just reinventing map, so actually using map makes it much easier.

ShowTuples

(You named it printTuples but it does not print the tuples, printing to the screen is an action, while this function is pure, so I named it showTuples as show is conventionally used in Haskell when a String is the output)

import Data.List

showTuples = intercalate "+" . map (\(x, y) -> (show x ++ "x" ++ show y))

Here you were just putting "x" in between the two elements of each tuple and "+" in between the tuples. Half of the code was reinventing map, the other half intercalate using these built-ins makes the code much shorter and self-explanatory.

getSequenceLogic

A minor simplification only here: you can drop the [] case as the xs case already handles it correctly:

getSequenceLogic [x] = showTuples [(1,0)]
getSequenceLogic xs = showTuples (groupByDelta xs)


Try a speculative, concurrent, lock free, atomic update until abort condition matches Please let me know if you see any performance improvements, bugs, or anything you'd change and why.

    public static bool TrySpeculativeUpdate(ref int field, out int result,
        Func<int, int> update, Func<int, bool> shouldAbort)
    {
        SpinWait spinWait = new SpinWait();
        while (true)
        {
            int snapshot = field;
            if (shouldAbort(field))
            {
                result = 0;
                return false;
            }
            else
            {
                int calc = update(snapshot);
                if (Interlocked.CompareExchange(ref field, calc, snapshot) == snapshot)
                {
                    result = calc;
                    return true;
                }
            }

            spinWait.SpinOnce();
        }
    }

Can be used like this

    private bool TryIncreaseCapacity(out int newCapacity)
    {
        return TrySpeculativeUpdate(ref _currentCapacity, out newCapacity,
            (currentCapacity) => currentCapacity + 1,
            (currentCapacity) => currentCapacity == _maxCapacity);
    }

    if (this.TryIncreaseCapacity(out newCapacity))
    {
        ...
    }
    else
    {
        ...
    }

What I'm really trying to accomplish is the fastest thread safe version of Interlocked.Increment that will stop incrementing at a max value and I will have some way of detecting it stopped incrementing.

I'm not sure I like the method's signature:

public static bool TrySpeculativeUpdate(ref int, out int, Func<int, int>, Func<int, bool>)

It's probably just me, but I like pushing ref and out parameters to the end of the parameters list, so I'd write it like this:

public static bool TrySpeculativeUpdate(Func<int, int>, Func<int, bool>, ref int, out int)

I'd use var to ease reading, and FWIW it might be clearer to pass snapshot instead of field to shouldAbort:

public static bool TrySpeculativeUpdate(Func<int, int> update, Func<int, bool> shouldAbort, ref int field, out int result)
{
    var spinWait = new SpinWait();
    while (true)
    {
        var snapshot = field;

        if (shouldAbort(snapshot))
        {
            result = 0;
            return false;
        }
        else
        {
            var calc = update(snapshot);
            if (Interlocked.CompareExchange(ref field, calc, snapshot) == snapshot)
            {
                result = calc;
                return true;
            }
        }

        spinWait.SpinOnce();
    }
}

That's all I could see. The naming looks about right, but I don't write enough multithreaded code to know for a fact whether this code is optimal or not - it does look good though, I don't see how nesting could be reduced, other than by extacting a tiny little specialized function responsible for this part:

if (Interlocked.CompareExchange(ref field, calc, snapshot) == snapshot)
{
    result = calc;
    return true;
}

But that clearly would be overkill IMO.


Project Euler #45 in Haskell I've got a solution for Project Euler's problem 45.

  Triangle, pentagonal, and hexagonal numbers are generated by the following formulae:

  $$\begin{array}{lll}
\textrm{Triangle}   & T_n=n(n+1)/2   & 1, 3, 6, 10, 15, \ldots \\
\textrm{Pentagonal}   & P_n=n(3n−1)/2   & 1, 5, 12, 22, 35, \ldots \\
\textrm{Hexagonal}   & H_n=n(2n−1)   & 1, 6, 15, 28, 45, \ldots \\
\end{array}$$

  It can be verified that \$T_{285} = P_{165} = H_{143} = 40755\$.

  Find the next triangle number that is also pentagonal and hexagonal.

It runs in under a second (so performance isn't an issue), but I can't help but feel that I'm doing it in an ugly way; I feel like I'm missing some sort of builtin like zipWith3.

Here's my current code:

tris  = scanl (+) 1 [2..]
pents = scanl (+) 1 [4,7..]
hexes = scanl (+) 1 [5,9..]

These are just the infinite lists of triangle, pentagonal, and hexagonal numbers.

findSame :: [Int] -> [Int] -> [Int] -> [Int]
findSame (x:xs) (y:ys) (z:zs)
    | (x==y) && (x==z) = x:(findSame xs ys zs)
    | (x<=y) && (x<=z) = findSame xs (y:ys) (z:zs)
    | (y<=x) && (y<=z) = findSame (x:xs) ys (z:zs)
    | (z<=x) && (z<=y) = findSame (x:xs) (y:ys) zs

This goes through the 3 lists at the same time and puts the numbers in a new list if they're equal (this also creates an infinite list).

This is the part where I think I could improve - this ended up looking relatively ugly.

main = do
    print $ last $ take 3 $ findSame tris pents hexes

This is just the main method and is pretty straightforward.

Any feedback on this code, especially the middle portion, would be very appreciated.  Creating code that looks clean is what motivates me to keep doing these, so ending up with this more brute-force approach leaves me hanging a bit.

The first observation that I would make is that your type signature is too specific:

findSame :: [Int] -> [Int] -> [Int] -> [Int]

The function should work for any Ord a:

findSame :: Ord a => [a] -> [a] -> [a] -> [a]

The second observation is that you don't need to work on three lists at a time.  You could just take two lists at a time:

findSame tris $ findSame pents hexes

But what is the findSame operation then?  It's the intersection of two ordered lists.  A quick search reveals an isect function in Data.Ordered.List.

So, the solution could just be

import Data.List.Ordered (isect)
head $ filter (> 40755) $ isect tris $ isect pents hexes

Or, more elegantly:

head $ filter (> 40755) $ foldr1 isect [hexes, pents, tris]

The implementation of isect is simple enough that you could just "steal" it.

Minor note: Personally, I would define the sequences so that the subscripts match the notation in the problem statement.

tris  = scanl (+) 0 [1..]
pents = scanl (+) 0 [1, 4..]
hexes = scanl (+) 0 [1, 5..]

That also makes it easier to generalize:

polygonalSeq :: Int -> [Int]
polygonalSeq s = scanl (+) 0 [1, (s - 1)..]
head $ filter (> 40755) $ foldr1 isect $ map polygonalSeq [6, 5, 3]


function addHandlers I'm experimenting with text editing with fabricjs objects and have the following code that works but that I'd like reviewed. My writing feels really clunky to me for some reason and as I'm new to handlers I'm sure what to change if anything?

function addHandler(id, fn, eventName) {
  document.getElementById(id)[eventName || 'onclick'] = function() {
    var el = this;
    if (obj = canvas.getActiveObject()) {
      fn.call(el, obj);
      canvas.renderAll();
    }
  };
}

function setStyle(object, styleName, value) {
  if (object.setSelectionStyles && object.isEditing) {
    var style = { };
    style[styleName] = value;
    object.setSelectionStyles(style);
  }
  else {
    object[styleName] = value;
  }
}

function getStyle(object, styleName) {
  return (object.getSelectionStyles && object.isEditing)
    ? object.getSelectionStyles()[styleName]
    : object[styleName];
}

// underline
addHandler('underline', function(obj) {
  var isUnderline = (getStyle(obj, 'textDecoration') || '').indexOf('underline') > -1;
  setStyle(obj, 'textDecoration', isUnderline ? '' : 'underline');
});

// bold
addHandler('bold', function(obj) {
  var isBold = (getStyle(obj, 'fontWeight') || '').indexOf('bold') > -1;
  setStyle(obj, 'fontWeight', isBold ? '' : 'bold');
});

// italic
addHandler('italic', function(obj) {
  var isItalic = (getStyle(obj, 'fontStyle') || '').indexOf('italic') > -1;
  setStyle(obj, 'fontStyle', isItalic ? '' : 'italic');
});

You should avoid setting your event handlers like that (by assigning a function to the onclick property) because you can only have a single function assigned to that property so anything else you assign to it will overwrite the previous event. Instead you should use addEventListener.

function addHandler(id, fn, eventName) {
  document.getElementById(id).addEventListener(eventName || 'click', function() {
    if (obj = canvas.getActiveObject()) {
      fn.call(this, obj);
      canvas.renderAll();
    }
  });
}

There is a line of code (i.e. var isUnderline = (getStyle(obj, 'textDecoration') || '').indexOf('underline') > -1;) which is repeated a lot. It is probably enough to justify giving it it's own function...

function hasStyle(obj, prop, value){
    return !!~(getStyle(obj, prop) || '').indexOf(value);
}

This saves you from having to put isUnderline into memory and then garbage collect it later..

addHandler('bold', function(obj) {
  setStyle(obj, 'fontWeight', hasStyle(obj, 'fontWeight', 'bold') ? '' : 'bold');
});

Finally, you could remove redundancy (for the D.R.Y. principle) by using an object..

var styles = {textDecoration: 'underline', fontWeight: 'bold', fontStyle: 'italic'};
for(let prop in styles){
    if(!styles.hasOwnProperty(prop)) continue;
    addHandler(styles[prop], function(obj) {
        setStyle(obj, styles[prop], hasStyle(obj, prop, styles[prop]) ? '' : styles[prop]);
    });
}


Polymorphic STL foreach without passing the container type I was trying to figure out how to make a breakable foreach macro for STL containers and I came up with this method that uses templates to recognize the container type automatically. Are there any performance / code-wise improvements you can think of?

// polymorphic break-able foreach test that works with nested calls on the same type of container

#include <vector>
#include <list>
#include <iostream>
#include <unordered_map>

std::unordered_map<void *, bool> _advance_iterator__instances_done;

template <class Tc, class Ti>
bool advance_iterator(Tc &container, Ti &iterator)
{
   static typename Tc::iterator it;

   if (_advance_iterator__instances_done[&container])
   {
      it = container.begin();
      _advance_iterator__instances_done[&container] = false;
   }

   if (it == container.end())
   {
      _advance_iterator__instances_done[&container] = true;
      return false;
   }

   iterator = *it;
   it++;

   return true;
}

#define foreach(value_type, iterator, container) \
_advance_iterator__instances_done[&container] = true; \
for (value_type iterator; advance_iterator(container, iterator); )

int main(int argc, char *argv[])
{
   std::vector<int> vec;
   std::list<int> lis;
   std::list<std::list<int>> matrix;

   // populate containers
   for (int i = 0; i < 10; i++)
   {
      static std::list<int> row;

      vec.push_back(i);
      lis.push_back(i + 10);

      for (int j = 1; j <= 10; j++)
         row.push_back(j * i);

      matrix.push_back(row);
      row.clear();
   }

   // regular foreach test
   std::cout << "vec:" << std::endl;

   foreach(int, value, vec)
      std::cout << value << std::endl;

   std::cout << std::endl << "half of lis:" << std::endl;

   // breaking test
   foreach(int, value, lis)
   {
      std::cout << value << std::endl;

      if (value == 14)
         break;
   }

   // regular again
   std::cout << std::endl << "full lis:" << std::endl;

   foreach(int, value, lis)
      std::cout << value << std::endl;

   // nested test
   std::cout << std::endl << "matrix:" << std::endl;

   foreach(std::list<int>, row, matrix)
   {
      foreach(int, value, row)
         std::cout << value << " ";

      std::cout << std::endl;
   }

   return 0;
}

This line gives my nightmares:

std::unordered_map<void *, bool> _advance_iterator__instances_done;

First up the name is reserved. Prefer not to use identifiers without a leading underscore (the rules are not trivial and everybody gets them wrong so just avoid them).

Secondly the size of the map is unbound. If you have a really large application (runs a long time) this is going to continuously grow.

This construct

bool advance_iterator(Tc &container, Ti &iterator)
{
   static typename Tc::iterator it;

makes the whole thing not thread safe.

Your naming convention makes it confusing to read:

#define foreach(value_type, iterator, container) \
                           ^^^^^^^^^^^
                     This is never the iterator it is the value.

Overall I think the boost FOREACH is a better choice for C++03 and C++11 now has a built in version


Refactoring a Telnet parser I'm writing a Telnet parser in Haskell that can run within any monad that implements some event-handling functions. When it finishes processing a block of input - whether because it read it all or because it was interrupted by an event handler - it returns the remainder of the text and a continuation for more input.

Right now, the code works, but it's pretty ugly. I've tried to make the patterns I saw in the program as obvious as possible, but I'm stymied at the actual refactoring stage.

I'm still pretty new to Haskell, so any feedback and advice on best practices is appreciated!

import qualified Data.ByteString as BS
import           Data.ByteString (ByteString)
import Data.Word (Word8)
import Control.Monad

{-
 - I suspect I can change these datatypes to use something like
 -   data Parser m r = Parser (ByteString -> m r)
 - but I'm not sure how to go about doing that.
 -}
data TelnetParser m = TelnetParser (ByteString -> m (TelnetState m))
type TelnetState m = (ByteString, TelnetParser m)

{-
 - These are the handler functions that the parser requires.
 - These allow you to execute other monadic actions in response to
 - Telnet events.
 -}
class (Monad m) => Telnet m where
  onText    :: Word8 ->          m ()
  onCommand :: Word8 ->          m Bool
  onOption  :: Word8 -> Word8 -> m Bool

  onText _     = return ()
  onCommand _  = return True
  onOption _ _ = return True

readTelnet :: (Telnet m) => ByteString -> TelnetParser m -> m (TelnetState m)
readTelnet str (TelnetParser f)
  | BS.null str = return (str, TelnetParser f)
  | otherwise   = f str

isOption :: Word8 -> Bool
isOption = (`elem` [250..254])

decideContinue :: (Telnet m) => Bool -> ByteString -> TelnetParser m -> m (TelnetState m)
decideContinue True = readTelnet
decideContinue False = curry return

{-
 - These functions implement the various states of the Telnet parser.
 -}

readOption :: (Telnet m) => Word8 -> ByteString -> m (TelnetState m)
readOption cmd str = do
    action <- decideContinue `liftM` onOption cmd ch
    action rest (TelnetParser readText)
  where (ch, rest) = (BS.head str, BS.tail str)

readIAC :: (Telnet m) => ByteString -> m (TelnetState m)
readIAC str
    | ch == 255    = onText ch >> (rest `readTelnet` TelnetParser readText)
    | isOption ch  =              (rest `readTelnet` TelnetParser (readOption ch))
    | otherwise    = do
        action <- decideContinue `liftM` onCommand ch
        action rest (TelnetParser readText)
  where (ch, rest) = (BS.head str, BS.tail str)

readCR :: (Telnet m) => ByteString -> m (TelnetState m)
readCR str
    | ch == 10    = onText 13 >> onText 10 >> (rest `readTelnet` TelnetParser readText)
    | ch ==  0    = onText 13 >>              (rest `readTelnet` TelnetParser readText)
    | otherwise   = onText 13 >>              (rest `readTelnet` TelnetParser readText)
  where (ch, rest) = (BS.head str, BS.tail str)

readText :: (Telnet m) => ByteString -> m (TelnetState m)
readText str
    | ch == 255   =              (rest `readTelnet` TelnetParser readIAC)
    | ch == 13    =              (rest `readTelnet` TelnetParser readCR)
    | otherwise   = onText ch >> (rest `readTelnet` TelnetParser readText)
  where (ch, rest) = (BS.head str, BS.tail str)

I think one of the reason why functional programming is usually easier is its natural modularity. Consider to design your solution in a more layered way. I.e. separate parser and invocations. Avoid callbacks in favor of lazy stream of events (see unsafeInterleaveIO) or borrow ideas from reactive frameworks. Also see Data.Binary.Get module or parsec package for implementing parser.


Find the 4 best parameters of a function to have the best cumulative sum I have a dataframe data with several columns and for each columns there are 10 years of daily data on average, something around 4000 rows then. The dataframe looks like this:
Date           A       B      C    ...
YYYY-MM-DD   123.4   56.7   89.0
YYYY-MM-DD   12.34   -5.6    NaN
...

Date is datetime64, every value is a float (positive and negative, they are real numbers), there could be NaN, there are no upside or downside boundaries.
I have this function :
def fct(data, name_a, name_b, param_a, param_b, param_c, param_d):

    df = data.loc[(data[name_a].notnull()) & (data[name_b].notnull()),[name_a, name_b]]

    df_corr = pd.DataFrame()
    df_corr[name_a] = df[name_a]
    df_corr[name_b] = df[name_b].shift(param_a)
    print(df_corr.corr().iloc[0,1])

    df['pct'] = df[name_a].pct_change()
    df['name_b_delta'] = df[name_b].shift(2+param_a)-df[name_b].shift(2+param_a+param_b)
    df['is_ok'] = df['name_b_delta'].apply(lambda x : np.sign(x+(-param_c)))

    if param_d:
        df['results'] = df['is_ok'] * (-1) * df['pct']
    else:
        df['results'] = df['is_ok'] * df['pct']

    df['cumul_results'] = df['results'].cumsum()

    return df

and currently, to optimize the cumul_results for differents pairs of name_a and name_b, I search for the best params using those nested loops :
name_a_list = ['a_dozen_of_elements_here']
name_b_list= ['25_elements_here']

df_line= []
df_index = []

for name_a in name_a_list :
    for name_b in name_b_list :
        for param_a in range(0,15):
            for param_b in range(1,15):
                for param_c in range (-2, 3, 1):
                    for param_d in [True, False]:
                        df = fct(data, name_a, name_b, param_a, param_b, param_c/1000, param_d):
                        df_index.append(f"{name_a}-{name_b}")
                        df_line.append([df['cumul_results'].iloc[-1], param_a, param_b, param_c, param_d])

results = pd.DataFrame(index=df_index, data=df_line, columns=['final result', 'param_a', 'param_b', 'param_c', 'param_d'])

It's more than 840 000 calls of the function fct and takes around 4 hours. I was wondering then if I could increase the performance of this code to reduce time. Thanks !

Answer, part one
It looks like about half the loops can be avoided by doing the True and False cases at the same time.  These differ only by the multiplication by -1 at the end of fct.  That suggests that results of of a True invocation can be used to shortcut the computations of the immediately following invocation of fct.
Is there a reason to not cache the results of the several array accesses?  That is, for example, df[name_a], df[name_b], and others?
Can the apply and lambda be avoided?  This seems overly complex (meaning, slow) way to perform the computation.
I presume the print invocation is not there usually.  IO in the inner loop would slow things down considerably.
Answer, part 2
Sample code.  This runs for me, but I haven't verified that it produces exactly the same results.
I made two kinds of updates: Values which were shared across loops were lifted to the most common loop.  The True and False cases were collapsed into a single function call which returns the True and False results using multiple values.
Ignore my comment re: apply.
My gut tells me that at least the innermost shift invocation ought to be unnecessary, but I haven't figured how to write a correct apply that would remove it.  (I need the apply iteration index to be passed in as an extra parameter.)
I added a number of print statements for debugging which you will want to remove.
import pandas as pd
import numpy as np

def fct_tf(data, name_a, name_b, param_a, param_b, param_c, df_common, df_shift):
    "Compute results for two sets of parameters"

    df_false = df_common.copy()

    # Lift one of the shifts up a loop.
    # Still working on removing the shifts altogether.
    # df_false['name_b_delta'] = df_false[name_b].shift(2+param_a)-df_false[name_b].shift(2+param_a+param_b)
    df_false['name_b_delta'] = df_shift - df_shift.shift(param_b)
    df_false['is_ok'] = df_false['name_b_delta'].apply(lambda x : np.sign(x+(-param_c)))

    df_false['results'] = df_false['is_ok'] * df_false['pct']
    df_false['cumul_results'] = df_false['results'].cumsum()

    df_true = df_false.copy()
    df_true['results'] = -1 * df_false['results']
    df_true['cumul_results'] = -1 * df_false['cumul_results']

    return df_true, df_false

def fct(data, name_a, name_b, param_a, param_b, param_c, param_d):
    "Compute results for one set of parameters"

    df = data.loc[(data[name_a].notnull()) & (data[name_b].notnull()),[name_a, name_b]]

    df['pct'] = df[name_a].pct_change()
    df['name_b_delta'] = df[name_b].shift(2+param_a)-df[name_b].shift(2+param_a+param_b)
    df['is_ok'] = df['name_b_delta'].apply(lambda x : np.sign(x+(-param_c)))

    if param_d:
        df['results'] = df['is_ok'] * (-1) * df['pct']
    else:
        df['results'] = df['is_ok'] * df['pct']

    df['cumul_results'] = df['results'].cumsum()

    return df

def compute(data, names_a, names_b):
    "Compute results ..."

    df_line= []
    df_index = []

    for name_a in names_a :
        print('name_a: ' + name_a)
        for name_b in names_b :
            print('name_b: ' + name_b)

            # 'df_common' will have just one column if the names are the same.
            # Then, an exception occurs when invoking 'pct_change()'.
            if ( name_a == name_b ):
                continue

            # This part of the results data is common across the parameters.
            df_common = data.loc[(data[name_a].notnull()) & (data[name_b].notnull()),[name_a, name_b]]
            print('Common data:')
            print(df_common)

            # The percent change is also common.  Note that the values are not free from
            # a dependence on 'name_b', since the null check can change which rows are selected.
            # That means this value cannot be lifted to the 'name_a' loop.
            df_common['pct'] = df_common[name_a].pct_change()
            print("Common data[pct]:")
            print(df_common['pct'])

            for param_a in range(0,15):
                # Reduce the number of shifts
                # In theory, this shift should be avoidable, but I'm still figuring
                # how to remove it.
                df_shift = df_common[name_b].shift(2+param_a)

                for param_b in range(1,15):
                    for param_c in range (-2, 3, 1):
                        # The computations for 'False' and 'True' are the same, except
                        # for a change in sign.
                        # for param_d in [True, False]:
                            # df = fct(data, name_a, name_b, param_a, param_b, param_c/1000, param_d)
                            # df_index.append(f"{name_a}-{name_b}")
                            # df_line.append([df['cumul_results'].iloc[-1], param_a, param_b, param_c, param_d])

                        df_true, df_false = fct_tf(data, name_a, name_b, param_a, param_b, param_c/1000, df_common, df_shift)

                        df_index.append(f"{name_a}-{name_b}")
                        df_line.append([df_true['cumul_results'].iloc[-1], param_a, param_b, param_c, True])

                        df_index.append(f"{name_a}-{name_b}")
                        df_line.append([df_false['cumul_results'].iloc[-1], param_a, param_b, param_c, False])

    results = pd.DataFrame(index=df_index, data=df_line, columns=['final result', 'param_a', 'param_b', 'param_c', 'param_d'])

    return results


First time trying async I've been trying to learn async coding recently. I've managed to get it to work and the UI thread doesn't block like it does if I don't make the code async. I realise this is a very contrived example, but is this how async should be done?

public partial class MainWindow : Window
 {
  public MainWindow()
  {
     InitializeComponent();
  }

  private async void Button_Click(object sender, RoutedEventArgs e)
  {
     string outputString = (await Authentication(usernameTextBox.Text)) ? "Name exists" : "Name does not exist";

     MessageBox.Show(outputString);
  }

  private Task<bool> Authentication(string username)
  {
     return Task.Run(() => CheckIfNameExistsInDatabaseOnServer(username));
  }

  private bool CheckIfNameExistsInDatabaseOnServer(string username)
  {
     //Pretend to pass details and wait for a response
     Thread.Sleep(5000);

     return (username == "Will");
  }
}

There isn't much to review here, but I would probably have introduced a variable for the awaited call's result, and then used a ternary to determine the outputString; the difference would have been that the awaited call would have stood out more, but there's nothing really wrong with the way you did it.

That said outputString would probably be better off as outputMessage, or simply output - having the type name in an identifier is never a sign of great naming.

I realize it's a tiny little example, but you have everything done in the UI's code-behind here - that's only good for prototype/throw-away code (which this probably is). In a real app you would have coded the logic elsewhere.


A more functional try...catch construct in Java I saw a question on Stack Overflow asking for a review of a custom try...catch construct that made use of Optionals, and got the idea to try writing my own version. Mine doesn't use Optionals though. Each supplied catch handler is expected to return a value.

I realized about half way through writing this that this is probably an awful idea that should never be used d in the real world for various reasons. I haven't written Java in a few years though, and I'm curious what can be improved.

Example of use:

Integer i =
        new Try<>(()-> Integer.parseInt("f"))
                  .catching(NumberFormatException.class, (e)-> 2)
                  .catching(IllegalStateException.class, (e) -> 3)
                  .andFinally(()-> System.out.println("Finally!"));

// Finally!

Integer j =
        new Try<>(()-> Integer.parseInt("1"))
                  .catching(NumberFormatException.class, (e) -> 2)
                  .execute();

System.out.println(i); // 2
System.out.println(j); // 1

Basically how it works is the Try object can have catching methods chained on it that register handlers. When an exception is thrown, it looks for a corresponding handler, and calls it, returning the returned value. If no handler is found, it rethrows the exception.

Major problems:

The need for .class is unfortunate. I couldn't think of how else to have the caller indicate the class of exception to be caught though.
Non-exception classes can be registered, although it will have no effect other than the minimal memory and CPU usage.
The need for execute when andFinally isn't used. I can't see how the class would know that all catches have been added though.
The need for new is unfortunate too. I figured I could take a page from Scala and create a static method that acts as a constructor, but then I'd need to do something like Try.tryMethod to reference it, which isn't a whole lot better.
The check in andFinally seems like it's unnecessary. Unless someone does something bizarre like:

Try t = new Try<>(() -> null);
t.andFinally(() -> {});
t.andFinally(() -> {});

It shouldn't be possible to add multiple finally blocks. I'm not sure to what extent I should stop the user from doing stupid things. I could make the object immutable, but again, I'm not sure if that's necessary.

Since this is basically just exercise code, I'd love to anything at all that could be improved here. This is the first Java I've really written in a few years, so I'm sure there's lots that can be improved.

import java.util.HashMap;
import java.util.Map;
import java.util.function.Function;
import java.util.function.Supplier;

public class Try<T> {
    private Supplier<T> tryExpr;
    private Map<Class, Function<Exception, T>> handlers = new HashMap<>();
    private Runnable finallyExpr;

    public Try(Supplier<T> tryExpr) {
        this.tryExpr = tryExpr;
    }

    public Try<T> catching(Class ex, Function<Exception, T> handler) {
        if(handlers.containsKey(ex)) {
            throw new IllegalStateException("exception " + ex.getSimpleName() + " has already been caught");

        } else {
            handlers.put(ex, handler);

            return this;
        }
    }

    public T execute() {
        try {
            return tryExpr.get();

        } catch (Exception e) {
            if(handlers.containsKey(e.getClass())) {
                Function<Exception, T> handler = handlers.get(e.getClass());

                return handler.apply(e);

            } else {
                throw e;
            }

        } finally {
            if(finallyExpr != null) {
                finallyExpr.run();
            }
        }
    }

    public T andFinally (Runnable newFinallyExpr) {
       if (finallyExpr == null) {
           finallyExpr = newFinallyExpr;

       } else {
           throw new IllegalStateException("Cannot have multiple finally expressions");
       }

       return execute();
    }

}

Thank you for sharing your code and this idea with us! :)

I have just some personal preferences and ideas to some of you "major problems" witch should be additional to the answers of @Mateusz Stefek and @TorbenPutkonen

Not Functional

  public Try<T> catching(Class ex, Function<Exception, T> handler) {
  if(/* ... */) {
    throw new IllegalStateException("exception " + ex.getSimpleName() + " has already been caught");
  } else {
    /* ... */
  }
}

In functional programming attempt not to throw any error instead you would return an Either.

  new Try<>(()-> Integer.parseInt("1"))
                      .catching(NumberFormatException.class, (e) -> 2)
                      .execute();

On each method call on Try you modify it, but fp is all about immutability.

Some Refactoring

Naming

  public Try<T> catching(Class ex, Function<Exception, T> handler)

I would write out ex to exception. Than we can change the name of

  private final Map<Class, Function<Exception, T>> handlers

to exceptionByHandler, which makes it easier to read, because it suggests a Map more intuitively, than handler which could be a List.

Null check

We have currently two null checks

  // in execute
if(finallyExpr != null)

// in andFinally
if (finallyExpr == null)

The utility class Objects offers the static methods isNull and nonNull and with an static import it could look like

// in execute
if(nonNull(finallyExpr))

// in andFinally
if (isNull(finallyExpr))

But if we use an Optional in andFinally it could look like

public T andFinally(Runnable newFinallyExpr) {
    finallyExpr = Optional.ofNullable(finallyExpr)
                          .orElseThrow(() -> new IllegalStateException("Cannot have multiple finally expressions"));

    return execute();
}

Non Intuitiv API

  The check in andFinally seems like it's unnecessary. [...]

You already mention this as a flaw.. but from an other perspective. For me as a client it is not intuitive to have the method andFinally and execute to fire up the Try.

More natural would be to let the client call the execute itself and override all finally with the next occurring finally.

new Try<>(()-> Integer.parseInt("f"))
                  .catching(NumberFormatException.class, (e)-> 2)
                  .catching(IllegalStateException.class, (e) -> 3)
                  .andFinally(()-> System.out.println("Finally!"))
                  .andFinally(()-> System.out.println("Finally #2!")) // overrides all previous finally
                  .execute();

Improvement

Make Try immutable

As in functional programming objects are immutable we can do it in Java too. We can let the client modify a Try with methods we provide him and return each time a new Try.

But to build up a Try we can create a TryBuilder witch is mutable. This is a common way which you can find in the Java-World to or example with String and StringBuilder.

State Pattern

  The need for execute when andFinally isn't used. I can't see how the class would know that all catches have been added though.

This is a scenario for the State Pattern. We have our base-state, which allows the client to add multiple catch-cases. After the last catch-case we need to let the client allow only one finally-case. After this our object is in the execution-state.

Builder Pattern

We can mix the State-Pattern with the Builder-Pattern and on each state we can use a builder, which only let use the methods the client currently needs to make the api more intuitive.

Replace the Constructor

  The need for new is unfortunate too. I figured I could take a page from Scala and create a static method that acts as a constructor, but then I'd need to do something like Try.tryMethod to reference it, which isn't a whole lot better.

With a static method as constructor we are allowed to return a different reference type.

new Try(/* ... */) // can only return an instance of Try

Try.of(/* ... */)  // can return everything you can imagine

For the use case I have in mind we need to make the constructor private and return a builder that we need to implement.

private Try(/* ... */) { /* ... */ }

public  static <T> CatchBuilder of(Supplier<T> tryExpr) {
    return new CatchBuilder<T>(new TryBuilder<T>().withTryExpr(tryExpr));
}

Example Implementation

Try

The Try is a public class with builds the API to the client. It provides the client outside of the package only a of(Supplier<T> tryExpr) to build up a Try. The constructor is package private therewith TryBuilder can access it. With of(Supplier<T> tryExpr) the clients gets a CatchBuilder (this is the modified State-Pattern mentioned above)

public class Try<T> {

  private Supplier<T> tryExpr;

  private Map<Class, Function<Exception, T>> handlers;

  private Runnable finallyExpr;

  Try(Supplier<T> tryExpr,
      Map<Class, Function<Exception, T>> handlers,
      Runnable finallyExpr) {
    this.tryExpr = tryExpr;
    this.handlers = handlers;
    this.finallyExpr = finallyExpr;
  }

  public static <T> CatchBuilder of(Supplier<T> tryExpr) {
    return new CatchBuilder<T>(new TryBuilder<T>().withTryExpr(tryExpr));
  }

  public T execute() {
    try {
      return tryExpr.get();
    } catch (Exception e) {
      if (handlers.containsKey(e.getClass())) {
        Function<Exception, T> handler = handlers.get(e.getClass());
        return handler.apply(e);
      } else {
        throw e;
      }
    } finally {
      if (finallyExpr != null) {
        finallyExpr.run();
      }
    }
  }
}

TryBuilder

This is the mutable way to build up a Try.

class TryBuilder<T> {

  private Supplier<T> tryExpr;

  private Map<Class, Function<Exception, T>> handlers;

  private Runnable finallyExpr;

  public TryBuilder<T> withTryExpr(Supplier<T> tryExpr) {
    this.tryExpr = tryExpr;
    return this;
  }

  public TryBuilder<T> withHandlers(Map<Class, Function<Exception, T>> handlers) {
    this.handlers = handlers;
    return this;
  }

  public TryBuilder<T> withFinallyExpr(Runnable finallyExpr) {
    this.finallyExpr = finallyExpr;
    return this;
  }

  public Try<T> build() {
    return new Try<>(tryExpr, handlers, finallyExpr);
  }

}

CatchBuilder and FinallyBuilder

class CatchBuilder<T> {

  private final Map<Class, Function<Exception, T>> handlers = new HashMap<>();

  private final TryBuilder<T> tryBuilder;

  CatchBuilder(TryBuilder<T> tryBuilder) {
    this.tryBuilder = tryBuilder;
  }

  public CatchBuilder<T> withCatch(Class ex, Function<Exception, T> handler) {
    if (handlers.containsKey(ex)) {
      throw new IllegalStateException("exception " + ex.getSimpleName() + " has already been caught");
    }

    handlers.put(ex, handler);
    return this;
  }

  public FinallyBuilder<T> and(Class ex, Function<Exception, T> handler) {
    withCatch(ex, handler);
    return new FinallyBuilder<>(tryBuilder.withHandlers(handlers));
  }

  public FinallyBuilder<T> onlyCatch(Class ex, Function<Exception, T> handler) {
    return and(ex, handler);
  }

}

class FinallyBuilder<T> {
  private final TryBuilder<T> tryBuilder;

  public FinallyBuilder(TryBuilder<T> tryBuilder) {
    this.tryBuilder = tryBuilder;
  }

  public Try<T> finallyDo(Runnable newFinallyExpr) {
    return tryBuilder.withFinallyExpr(newFinallyExpr)
                     .build();
  }

  public Try<T> withoutFinally() {
    return tryBuilder.build();
  }

}

Exmaple

public static void main(String[] args) {

  Try<Integer> firstTry = Try.of(() -> Integer.parseInt("f"))
                             .withCatch(NumberFormatException.class, (e) -> 2)
                             .and(IllegalStateException.class, (e) -> 3)
                             .finallyDo(() -> System.out.println("Finally!"));

  Try<Integer> secondTry = Try.of(() -> Integer.parseInt("f"))
                              .onlyCatch(NumberFormatException.class, (e) -> 2)
                              .withoutFinally();
}


Convert between date/time and time-stamp without using standard library routines I am trying to implement two simple convertors: date/time to time-stamp and vice-versa, without any dependencies on time library routines (such as localtime, mktime, etc, mainly due to the fact that some of them are not thread-safe).

I have the following date/time structure:

typedef struct
{
    unsigned char second; // 0-59
    unsigned char minute; // 0-59
    unsigned char hour;   // 0-23
    unsigned char day;    // 1-31
    unsigned char month;  // 1-12
    unsigned char year;   // 0-99 (representing 2000-2099)
}
date_time_t;

I would like to have a second opinion on the following conversion routines (given a legal input):

static unsigned short days[4][12] =
{
    {   0,  31,  60,  91, 121, 152, 182, 213, 244, 274, 305, 335},
    { 366, 397, 425, 456, 486, 517, 547, 578, 609, 639, 670, 700},
    { 731, 762, 790, 821, 851, 882, 912, 943, 974,1004,1035,1065},
    {1096,1127,1155,1186,1216,1247,1277,1308,1339,1369,1400,1430},
};

unsigned int date_time_to_epoch(date_time_t* date_time)
{
    unsigned int second = date_time->second;  // 0-59
    unsigned int minute = date_time->minute;  // 0-59
    unsigned int hour   = date_time->hour;    // 0-23
    unsigned int day    = date_time->day-1;   // 0-30
    unsigned int month  = date_time->month-1; // 0-11
    unsigned int year   = date_time->year;    // 0-99
    return (((year/4*(365*4+1)+days[year%4][month]+day)*24+hour)*60+minute)*60+second;
}

void epoch_to_date_time(date_time_t* date_time,unsigned int epoch)
{
    date_time->second = epoch%60; epoch /= 60;
    date_time->minute = epoch%60; epoch /= 60;
    date_time->hour   = epoch%24; epoch /= 24;

    unsigned int years = epoch/(365*4+1)*4; epoch %= 365*4+1;

    unsigned int year;
    for (year=3; year>0; year--)
    {
        if (epoch >= days[year][0])
            break;
    }

    unsigned int month;
    for (month=11; month>0; month--)
    {
        if (epoch >= days[year][month])
            break;
    }

    date_time->year  = years+year;
    date_time->month = month+1;
    date_time->day   = epoch-days[year][month]+1;
}

I have tested this on an extensive amount of legal input (between 01/01/2000 and 31/12/2099). Any constructive comments would be appreciated (performance improvement suggestions, readability, etc).

Good that OP is using 4 simplifications: year 2000-2099, no DST, no leap second, no timezone.  So OP knows of code limitations concerning these.  Various elements of this function break without those givens.
Make static unsigned short days a const.
Use a long for your epoch as in:

void epoch_to_date_time(date_time_t* date_time,unsigned long epoch)

as unsigned is only guaranteed to range form 0 to at least 65535 which is insufficient here.
For various functions, consider adding const.  There are pros and cons to this, but may be beneficial in your case:

// unsigned int date_time_to_epoch(date_time_t* date_time)
unsigned int date_time_to_epoch(const date_time_t* date_time)

There exist equation based (non-loop) solutions to the year-day that do not need a table like your  unsigned short days[4][12].  Code then looks more complicated, but is faster.  Please advise if interested.
Most of the magic numbers like 60, 24 are so well-known that constant or macro substitution seems superfluous.  But (365*4+1) may benefit with something like "DaysPer4Years".


Subquery v/s inner join in sql server I have  following queries

First one using inner join

SELECT item_ID,item_Code,item_Name
FROM [Pharmacy].[tblitemHdr] I INNER JOIN  EMR.tblFavourites F ON I.item_ID=F.itemID
WHERE F.doctorID = @doctorId AND F.favType = 'I'

second one using sub query like

SELECT item_ID,item_Code,item_Name from [Pharmacy].[tblitemHdr]
WHERE item_ID IN
(SELECT itemID FROM EMR.tblFavourites
WHERE doctorID = @doctorId AND favType = 'I'
)

In this item table [Pharmacy].[tblitemHdr] Contains 15 columns and 2000 records. And [Pharmacy].[tblitemHdr] contains 5 columns and around 100 records. in this scenario which query gives me better performance?

The SQL Server query analyzer is smart enough to understand that these queries are identical (given that item_ID is unique in EMR.tblFavourites for a certain doctorID/favType pair).

They should yield exactly the same execution plan, and thus they are equal in terms of performance. I would prefer second variant though...


Transform byte array into upside down array of floating point pixels I need to do the following in Python 3.x:

Interpret an array of bytes as an array of single-precision floats.
Then group each four consecutive floats into subarrays, i.e. transform [a,b,c,d,e,f,g,h] into [[a,b,c,d], [e,f,g,h]]. The subarrays are called pixels, and the array of pixels forms an image.
Flip the image vertically.

Here is what I have now:

floats = array.array('f')
floats.fromstring(tile_data)
pix = []
for y in range(tile_h - 1, -1, -1):
    stride = tile_w * 4
    start_index = y * stride
    end_index = start_index + stride
    pix.extend(floats[i:i + 4] for i in range(start_index, end_index, 4))

tile_data is the input array of raw bytes, tile_w and tile_h are respectively the width and height of the image, pix is the final upside down image.

While this code works correctly, it takes around 50 ms to complete on my machine for a 256x256 image.

Is there anything obviously slow in this code? Would numpy be a potentially good avenue for optimization?

Edit: here is a standalone program to run the code and measure performance:

import array
import random
import struct
import time

# Size of the problem.
tile_w = 256
tile_h = 256

# Generate input data.
tile_data = []
for f in (random.uniform(0.0, 1.0) for _ in range(tile_w * tile_h * 4)):
    tile_data.extend(struct.pack("f", f))
tile_data = bytes(tile_data)

start_time = time.time()

# Code of interest.
floats = array.array('f')
floats.fromstring(tile_data)
pix = []
for y in range(tile_h - 1, -1, -1):
    stride = tile_w * 4
    start_index = y * stride
    end_index = start_index + stride
    pix.extend(floats[i:i + 4] for i in range(start_index, end_index, 4))

print("runtime: {0} ms".format((time.time() - start_time) * 1000))

Would numpy be a potentially good avenue for optimization?

Yes. In general, pushing python loops into C extensions often makes sense.

You might prefer to start the timer after pix = [], since you're not focused on improving fromstring's performance.

There's nothing obviously slow, beyond bulk data movement and regrouping, and it would take changing the problem if you wanted to introduce a level of indirection to avoid that work. One could hoist the stride constant out of the loop, but that's pretty far down the list of worries.


Random 6 to 10 digits number I am generating a random numerical 6-10 digit password. I need some important reviews of yours.  Is it OK or are there some needed modifications?

val AB = "0123456789669900"
val rnd = new Random()

def randomNumericalString(): String = {
    val len = rnd.nextInt(5) + 5
    val sb = new StringBuilder(len)
    for (i <- 0 to len)
      sb.append(AB.charAt(rnd.nextInt(AB.length())))
    return sb.toString
  }

Your algorithm makes the probabilities of some numbers much higher than other numbers. Is this intentional? For example, 0 will appear 3 times as often as 1. It would be better to simply have the input string "0123456789" and have the uniform distribution.

The range you supply in your for-loop is confusing, the requirements are for 6 digits, but, you use the magic-number 5. in the random. Consider using:

val shortest = 6
val longest = 10
val len = shortest + rnd.nextInt(longest - shortest + 1)
for (i <- 1 to len)

That is enough of an answer, on it's own, but I would suggest you consider the following. It is a little flawed in the sense that it never generates a value with a 0 as the first character.... I was part way through the answer when I realized this, but thought you should consider the approach anyway:

def randomNumericalString(): String = {
    return ((100000.0 + rnd.nextDouble() * (10000000000.0 - 100000.0)).toLong).toString
}

The above will generate a random number, with uniform distribution, between 100000 and 9999999999 (and here, as an ideone )


Convert a start and end date chosen by a user to epoch This method allows the user to choose a start and end date in human readable form, then convert it to epoch. The method will return a array of longs composed by the epoch start date and the epoch end date.

I handle some exception so if the user types anything else than the date it will end the iteration and start again. If the end date is before the start date it will start again...

private static long[] rectime_choice_period() {
    long epochstart = 0;
    long epochend = 0;
    boolean confirm = true;

    String datetimeend = null;
    String datetimestart = null;

    BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
    while (confirm){

        //enter start date
        System.out.print("\nChoose start date (MM/dd/yyyy hh:mm:ss): ");
        try{
            datetimestart = br.readLine();
        }catch (IOException e){
            System.err.println(e.getClass().getName()+": "+e.getMessage() + "\n\n\n");
            continue;
        }

        //enter end date
        System.out.print("\nChoose end date (MM/dd/yyyy hh:mm:ss): ");
        try{
            datetimeend = br.readLine();
        }catch (IOException e){
            System.err.println(e.getClass().getName()+": "+e.getMessage() + "\n\n\n");
            continue;
        }

        //convert to epoch
        try{
            epochstart = new java.text.SimpleDateFormat("MM/dd/yyyy HH:mm:ss").parse(datetimestart).getTime() / 1000;
            epochend = new java.text.SimpleDateFormat("MM/dd/yyyy HH:mm:ss").parse(datetimeend).getTime() / 1000;
        }catch (ParseException e){
            System.err.println("Error entered date cannot be converted!");
            continue;
        }catch (Exception e){
            System.err.println(e.getClass().getName()+": "+e.getMessage() + "\n\n\n");
            continue;
        }

        if ((epochend - epochstart) > 0){
            System.out.println("Start time :" + epochstart + "\tEnd time : " + epochend);
            confirm = false;
        }else {System.err.println("Wrong date! Start date should be before end date!");}

    }

    long[] period = {epochstart, epochend};
    return period;
}

But I am looking to optimize this code, maybe avoid the loop or the continue statement, I checked on google way to improve java code but nothing was really interesting. So any idea? Something I made wrong?

The while loop isn't really a problem. You need some way to loop back and re-ask the user input if something went wrong (e.g. they entered wrong data).

However, there are a couple of improvements that you can make:

You are opening a stream with

BufferedReader br = new BufferedReader(new InputStreamReader(System.in));

but you are never closing it. This can lead to a resource leak. Starting with Java 7, you can deal easily with this by using the try-with-resources statement. Also, instead of using a BufferedReader, it would be easier to use a Scanner and its nextLine() method.

try (Scanner scanner = new Scanner(System.in)) {
    // read user input with scanner.readLine();
}

This has other advantages: you don't need to catch any IOException so the code is cleaner.

try (Scanner scanner = new Scanner(System.in)) {
    while (confirm){
        System.out.print("\nChoose start date (MM/dd/yyyy hh:mm:ss): ");
        String datetimestart = scanner.nextLine();

        System.out.print("\nChoose end date (MM/dd/yyyy hh:mm:ss): ");
        String datetimeend = scanner.nextLine();
        // ...
    }
}

You should respect Java naming conventions for your variables and methods. Instead of datetimestart, prefer dateTimeStart (camel-case) and epochEnd. In the same way, instead of rectime_choice_period, prefer recTimeChoicePeriod (arguably, you could have another more descriptive name).
In the same way, you should respect the Java braces style

}else {System.err.println("Wrong date! Start date should be before end date!");}

should be rewritten

} else {
    System.err.println("Wrong date! Start date should be before end date!");
}

Don't hardcode the date format "MM/dd/yyyy HH:mm:ss". You should refactor that into a constant instead:

private static final String DATE_FORMAT = "MM/dd/yyyy HH:mm:ss";

in this way, if the pattern should ever change, you don't need to update the 4 occurences where it appears.
You are constructing twice a SimpleTextFormat when you could only construct one:

DateFormat formatter = new SimpleDateFormat("MM/dd/yyyy HH:mm:ss");
epochStart = formatter.parse(dateTimeStart).getTime() / 1000;
epochEnd = formatter.parse(dateTimeEnd).getTime() / 1000;

As a side-note also, consider using imports instead of writing explicitely the fully qualified name of the class: import java.text.SimpleDateFormat;.
Avoid declarations of all the variables at the start of the method. Declare them only when they are needed. So, replace

String datetimeend = null;
// ...
datetimestart = scanner.nextLine();

with

String dateTimeEnd = scanner.nextLine();

You don't need the parentheses inside if ((epochend - epochstart) > 0). A simple if (epochend - epochstart > 0) suffices and avoids extra parentheses.
Avoid useless comments: //enter start date adds no value as a comment. Good comments don't repeat the code but clarify its intent.
Also, avoid useless temporary variables:

long[] period = {epochstart, epochend};
return period;

can just be written return new long[] { epochStart, epochEnd };.
In the code parsing the String into long, why are you catching the general Exception since you already have a catch clause for the ParseException? It is not necessary and should be removed.


Is there a more succinct way to write this Ruby function? Just for fun, I want to try to write this Ruby function more succinctly. I imagine it can be done, I'm just not knowledgeable enough with Ruby yet to know how. Any suggestions?

  def tags
    return ['Cancelled'] if cancelled?
    tags = []
    tags << 'Filled' if filled?
    tags << 'In Progress' if in_progress?
    tags
  end

Functional approach:

def tags
  if cancelled?
    ["Cancelled"]
  else
    [("Filled" if filled?), ("In Progress" if in_progress?)].compact
  end
end

Ideas behind the snippet:

Don't perform imperative side-effects on arrays (unless performance is a problem, not the case here).
Use the pattern: array of "nilable" expressions + compact to build arrays with conditional elements.
Minimize the use of inline conditional statements, they make a function harder to understand. They are ok as guards (early exit of functions), but not here. Note that I do use inline ifs, but they are expressions, not statements; more orthodox -but also more verbose- would be (filled? ? "Filled" : nil).


Dynamically sized queue This is my first deep dive into C, and I want to make sure that I'm not doing anything stupid. The code below creates an "Array List" of sorts, which automatically resizes itself.

This code is meant to run on a PIC24, where memory is at a premium.

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

#ifndef CHAR_QUEUE_H
#define CHAR_QUEUE_H
#define QUEUE_INIT_SIZE 128

typedef struct CharQueue_t{
    uint16_t size;
    uint16_t capacity;
    unsigned char *data;
    void (*append) (struct CharQueue_t *, unsigned char);
    void (*destroy) (struct CharQueue_t *);
    unsigned char (*get) (struct CharQueue_t *, uint16_t);
    unsigned char (*next) (struct CharQueue_t *);
} CharQueue_t;

CharQueue_t *CharQueueInit();

void CharQueueAppend(struct CharQueue_t *queue, unsigned char value);

void CharQueueDestroy(struct CharQueue_t *queue);

unsigned char CharQueueGet(struct CharQueue_t *queue, uint16_t idx);

unsigned char CharQueueNext(struct CharQueue_t *queue);

#endif  /* CHAR_QUEUE_H */

CharQueue_t *CharQueueInit() {
    CharQueue_t *queue = malloc(sizeof(CharQueue_t));
    // Initialize size and capacity
    queue->size = 0;
    queue->capacity = QUEUE_INIT_SIZE;
    queue->data = malloc(sizeof(unsigned char) * queue->capacity);
    queue->append = CharQueueAppend;
    queue->destroy = CharQueueDestroy;
    queue->get = CharQueueGet;
    queue->next = CharQueueNext;
    return queue;
}

void CharQueueAppend(struct CharQueue_t *queue, unsigned char value) {
    // Resize the queue if it's at capacity
    if (queue->size >= queue->capacity) {
        queue->capacity *= 2;
        queue->data = realloc(queue->data, sizeof(unsigned char) * queue->capacity);
    }
    queue->data[queue->size] = value;
    queue->size++;
}

void CharQueueDestroy(struct CharQueue_t *queue) {
    free(queue->data);
    free(queue);
}

unsigned char CharQueueGet(struct CharQueue_t *queue, uint16_t idx) {
    if (idx >= queue->size || idx < 0) {
        return 0;
    }
    return queue->data[idx];
}

unsigned char CharQueueNext(struct CharQueue_t *queue) {
    char data = queue->data[0];
    queue->size = queue->size - 1;
    // Move the pointer up the stack -- Does this leak memory?
    *queue->data++;
    return data;
}

int main(void) {
    CharQueue_t *queue = CharQueueInit();
    int i;
    for (i = 0; i < 256; i++) {
        queue->append(queue, i);
    }
    printf("Queue Size is %d\r\n", queue->size);
    for (i = 0; i < 256; i++) {
        printf("#%d in queue: 0x%d\r\n", i, queue->next(queue));
    }
    return 0;
}

Just a few things:

uint16_t for a size type is really small. To be precise, the maximum value of uint16_t is 2^16 - 1, or 65535. That's 64 kb of data only, which is not enough for most applications, if you consider that modern PCs have gigabytes of memory available. For sizes in general, size_t suggests itself, and offers plenty of bits for addresses (on most modern pc architectures, at least).
Why all of these functions pointers in the struct? Unless your main design goal is maximum flexibility and genericity, all you're likely achieving is bloating your struct.
Comparing a variable of type uint16_t against 0 as in if (idx >= queue->size || idx < 0) is pointless. The u in uint16_t stands for "unsigned", and variables of the type can only take values in the range [0, 2^16 - 1].
// Move the pointer up the stack -- Does this leak memory?
*queue->data++;

Not only does this leak data, it makes freeing the memory actually impossible! free can only be called on pointers directly obtained from  malloc and friends. As soon as you increment the pointer, free will automatically cause undefined behavior. Under all circumstances, you have to keep a pointer to the start around, or do some index shenanigans.
sizeof(unsigned char) is guaranteed to be 1 by definition. You don't need to multiply by that when you're allocating memory.
It's usually a good idea to check for malloc failures. If malloc fails to allocate memory, it returns NULL, which can be easily checked against and which allows you to exit gracefully, rather than (hopefully) crashing with a segfault.
For that very same reason, i.e. that malloc can fail, directly assigning from realloc is dangerous, since realloc can also fail and return NULL, in which case you lose the pointer to the original memory if you're not careful (leading to a certain leak down the line).
Similarly, check for NULL on all the pointers you receive from a user. Calling any of your functions with NULL as the first argument is going to lead to undefined behavior, which you should guard against.


Enhance and Shrink Preg_match lines I made a preg_match to get my results and it works fine but I think it can be done with simple lines instead of repeating the code

$input = array('http://example.com/blue-bloods-s10e13-more-text/','http://example.com/charmed-2018-s02e11-more-text/','http://example.com/dynasty-2017-s03e11-more-text/','http://example.com/magnum-p-i-2018-s02e14-more-text/','http://example.com/fresh-off-the-boat-s06e13-more-text/','http://example.com/hawaii-five-0-2010-s10e14-more-text/','http://example.com/american-housewife-s04e13-more-text/','http://example.com/love-us-s01e13-more-text/');

    foreach ($input as $value ){
preg_match('/http:\/\/example.com\/([^`]*?)-s([0-9]{2})e([0-9]{2})/', $value, $matches);

$name = $matches[1];
$cname = preg_replace('/-us/', '', $name);
$cname1 = preg_replace('/-/', ' ', $cname);
$cname2 = preg_replace('/([0-9]{4})$/', '', $cname1);
$cname3 = preg_replace('/\s$/', '', $cname2);

echo $cname3."<br>";
}

The output is exactly like what I want

blue bloods
charmed
dynasty
magnum p i
fresh off the boat
Hawaii five 0
american housewife
love

Toward optimization and better functionality

consider using short array syntax [] instead of old-fashioned array()
do not designate redundant captured groups such as ([0-9]{2})...([0-9]{2}) if you only need the 1st captured group ([^`]*?)
regex [0-9] has a concise equivalent for designating digits - \d

All those preg_replace calls in the initial approach can be combined into a single call using regex alternation group /-us|(\d{4}|\s)$/ for combining the needed patterns. The \s$ patten could also be excluded from combined pattern in favor of rtrim function call.As /-/ is the only pattern that is too simple and requires different replacement string ' ' (space) - it deserves to be a simple string replacement with str_replace function.

The final optimized approach:

$input = ['http://example.com/blue-bloods-s10e13-more-text/','http://example.com/charmed-2018-s02e11-more-text/',
          'http://example.com/dynasty-2017-s03e11-more-text/','http://example.com/magnum-p-i-2018-s02e14-more-text/',
          'http://example.com/fresh-off-the-boat-s06e13-more-text/','http://example.com/hawaii-five-0-2010-s10e14-more-text/',
          'http://example.com/american-housewife-s04e13-more-text/','http://example.com/love-us-s01e13-more-text/'];

foreach ($input as $value){
    preg_match('/http:\/\/example.com\/([^`]*?)-s\d{2}e\d{2}/', $value, $matches);
    $name = str_replace('-', ' ', preg_replace('/-us|(\d{4}|\s)$/', '', $matches[1]));
    echo $name . "<br>";
}

The output:

blue bloods
charmed
dynasty
magnum p i
fresh off the boat
hawaii five 0
american housewife
love


Bash script to share snippets of code using rsync I just got through writing a bash script so my friend and I can share snippets of code to a central file via rsync and ssh. What it does is this;

Sets up ssh public key auth if not already working.
Makes a local "snippet file" using rsync to copy an exiting file from remote server.
Allows the user to edit the snippet file.
If user chooses, sync snippet file to remote host.

I put the project on github at https://github.com/shakabra/snippetShare

It's a pretty straight-forward script, but I wonder what could be done better.

I am really new at programming and was hoping you guys might spot some obvious mistakes that I just over-looked or don't understand.

#!/bin/bash
SERVER="dakinewebs.com"
UN=""
PORT="22"
TIME=$(date +"%c")
localSnippetFile=$HOME/snippetFile.html
remoteSnippetFile=/home/shakabra/dakinewebs.com/snippetShare/snippetFile.html

checkSsh () {
    ssh -o BatchMode=yes "$UN"@"$SERVER" 'exit'
    if [[ $? = 255 ]]; then
        echo "Local key doesn't exits"
        echo "creating local key"
        ssh-keygen -t rsa
        ssh-add
        uploadKey
    fi
}
sshConfig () {
    if [[ -z $SERVER ]]; then
        echo -e "SERVER NAME:\n"
        read SERVER
    fi

    if [[ -z $UN ]]; then
        echo -e "USERNAME:\n"
        read UN
    fi

    if [[ -z $PORT ]]; then
        echo -e "PORT:\n"
        read PORT
    fi
}
makeLocalSnippet () {
    echo "syncing with remote snippet"
    rsync -avz --progress -e ssh "$UN"@"$SERVER":"$remoteSnippetFile" "$localSnippetFile"
}

editSnippet () {
    echo -e "What's the title of the snippet?\n"
    read snippetTitle
    echo -e "<time>"$TIME"</time>\n" >> "$localSnippetFile"
    echo -e ""$snippetTitle"" >> "$localSnippetFile"
    echo "

" >> "$localSnippetFile"
    nano "$localSnippetFile"
    syncSnippet
        while [[ $? = 1 ]]; do
            syncSnippet
        done
}
syncSnippet () {
    read -p "Sync snippet file now [y/n]: " syncNow
    case $syncNow in
        y|Y|yes|YES|Yes)
        rsync -av --delete -e ssh "$localSnippetFile"  "$UN"@"$SERVER":"$remoteSnippetFile"
    ;;
        n|N|no|No|NO)
        echo "Guess we'll sync next time. Hope it wasn't too important!"
    ;;
        *)
        echo "not an answer"
        return 1
    ;;
    esac
}
uploadKey ()
{
    ssh-copy-id "$UN"@"$SERVER"
    ssh "$UN"@"$SERVER" 'exit'
    echo "Key uploaded"
}
sshConfig
checkSsh
makeLocalSnippet
editSnippet

Your read calls perform \ expansion: if the line ends with \, the shell continues reading the next line and returns the text without the backslash-newline sequence; any other backslash quotes the next character (i.e. the backslash is ignored, except that \\ yields \). This is probably not desirable; use read -r to avoid this behavior. Also, for future reference, note that whitespace at the beginning and end of the input is stripped (you need to set IFS to the empty string to avoid this, but here stripping whitespace is desirable); see Understanding IFS and the linked posts.

Furthermore, since this script is designed for interactive use, you should use read -e to enable editing of the input line (with readline). It would also be nicer to have the prompt together with the input line: use read -p 'Server name: ' and so on instead of calling echo (and don't SHOUT). For example:

if [[ -z $SERVER ]]; then
    read -rep 'Server name: ' SERVER
fi

rsync -avz --progress -e ssh "$UN"@"$SERVER":"$remoteSnippetFile" "$localSnippetFile"

This looks a little more complicated than it needs to be, you can write "$UN@$SERVER:$remoteSnippetFile".

The following part has a bug and is not as readable as it could be:

echo -e "<time>"$TIME"</time>\n" >> "$localSnippetFile"
echo -e ""$snippetTitle"" >> "$localSnippetFile"
echo "

" >> "$localSnippetFile"

The bug is that $TIME and $snippetTitle undergo word splitting and filename generation (i.e. globbing). For example, if $snippetTitle is The A* algorithm and the current directory contains the files Astar.tex, Astar.pdf and test.c, then you're writing The Astar.pdf Astar.tex algorithm to the file. Always use double quotes around variable and command substitutions, unless you know why you must leave them off and why it's safe to do so. (Exception: you may leave off the double quotes in contexts where they are explicitly not needed, such as inside [[ … ]].)

The readability improvement is to use a here document.

cat <<EOF >>"$localSnippetFile"
<time>$TIME</time>
$snippetTitle

EOF

nano "$localSnippetFile"

glenn jackman has already mentioned this: it's rude to impose your editor on the user. There is a standard for letting the user choose an editor: use the VISUAL environment variable, falling back to EDITOR. Use a sensible fallback if neither variable is set.

if [[ -z ${VISUAL=$EDITOR} ]]; then
  for VISUAL in sensible-editor nano vi; do
    if type "$VISUAL" >/dev/null 2>/dev/null; then break; fi
  done
fi

Drop this …

       while [[ $? = 1 ]]; do
           syncSnippet
       done

… and instead change syncSnippet to first call read until the user enters an accepted input, and then perform the actions based on this input.

while
    read -p "Sync snippet file now [y/n]: " -rN1 yn
    [[ $yn != [YNyn] ]]
do :; done
if [[ $yn = [Yy] ]]; then
  rsync …
fi


Is there a better way to do get/set periodically in tornado? I use a global variable to fit my need:

scores = {}

def update():
    global scores
    # fetch data from database
    scores = xxx

class MyRequestHandler(tornado.web.RequestHandler):
    @tornado.web.asynchronous
    def get(self):
        def choice_with_score(items):
            # return a key related to scores
            return key

        def get_key():
            global scores
            return choice_with_score(scores)

        self.write('%s' % get_key())
        self.finish()

if __name__ == '__main__':
    # initially update
    update()

    app = tornado.web.Application([
        (r'.*', MyRequestHandler),
    ])

    http_server = tornado.httpserver.HTTPServer(app)
    http_server.listen(port)
    ioloop = tornado.ioloop.IOLoop.instance()

    # background update every x seconds
    task = tornado.ioloop.PeriodicCallback(
            update,
            15 * 1000)
    task.start()

    ioloop.start()

Here, the function update() gets new data instead of updating in every incoming request, which is why I use a global variable.

Are there other ways to do the same work?

You could use a class to keep your scores inside a defined scope and not use the globals. This way it's fairly easy to test and you don't have to deal globals. All you need to ensure is that you always pass the same instance. You could probably do it using some sort Singleton patterns.

import tornado
import tornado.web

# I hate the name manager but couldn't come up with something good fast enough.
class ScoreManager(object):
    def __init__(self):
        self._scores = {}

    def fetch_from_database(self):
        # fetch from database
        self._scores = {'key': 'score'}

    def get_all(self):
        return self._scores

class MyRequestHandler(tornado.web.RequestHandler):
    # this is the way to have sort of a constructor and pass parameters to a RequestHandler
    def initialize(self, score_manager):
        self._score_manager = score_manager

    @tornado.web.asynchronous
    def get(self):
        def choice_with_score(items):
            # return a key related to scores
            return key

        def get_key():
            global scores
            return choice_with_score(self._score_manager)

        self.write('%s' % get_key())
        self.finish()

if __name__ == '__main__':
    # initially update
    score_manager = ScoreManager()
    score_manager.fetch_from_database()

    app = tornado.web.Application([
        (r'.*', MyRequestHandler, dict(score_manager=score_manager)), # these params are passed to the initialize method.
    ])

    http_server = tornado.httpserver.HTTPServer(app)
    http_server.listen(port)
    ioloop = tornado.ioloop.IOLoop.instance()

    # background update every x seconds
    task = tornado.ioloop.PeriodicCallback(
            update,
            15 * 1000)
    task.start()

    ioloop.start()


DRY version of this one line if/else for property's existence I have an object for which I wish to modify a property if a condition is met, else leave it virginal.

I find my current code to be very repetitive and would instead like to use a more succinct form, but am unsure of how to lambda-fy it or at least tell it to pass if condition not met:

my_object.unique_id = 'TBC' if not my_object.unique_id else my_object.unique_id

I guess this is nicer, but still repetitive:

if not my_object.unique_id: my_object.unique_id = 'TBC'

I would like something more like

my_object.unique_id = 'TBC' if None

Is there a way to only reference the property once?

You can use

my_object.unique_id = my_object.unique_id or 'TBC'

Otherwise, stop worrying about having to repeat a variable name, which is not what people typically mean when they talk about DRY. Variable names are often repeated when you do in-place manipulation of a variable, because you are saying "do something to x and then store the result as x".

If you have lots of similar lines of code then you could submit some more of your code for review; there may be other ways to simplify.


Calculating a quality score of something based on attributes I'm willing to have a way to calculate a quality score of an object; in this case an addressbook contact.

I believe there are way better approaches to do this and I'd like to hear your references.

How can I calculate a quality scoring of an object based on its properties?

/**
 *
 * get Quality
 *
 * @return float Contact quality percentual score calculated from profile completion
 */
public function getQualityAttribute()
{
    $quality  = 0;
    $quality += isset($this->firstname) ? 1 : 0;
    $quality += isset($this->lastname) ? 1 : 0;
    $quality += isset($this->nin) ? 5 : 0;
    $quality += isset($this->birthdate) ? 2 : 0;
    $quality += isset($this->mobile) ? 4 : 0;
    $quality += isset($this->email) ? 4 : 0;
    $quality += isset($this->postal_address) ? 3 : 0;
    $total    = 20;
    return $quality/$total*100;
}

If you want to know the purpose of this method or further in the app, you can find the code here.

This approach sounds logically good, as it allows to determine how much the profile has been completed. There seems to exist a correlation between people completing their profile fully and giving interesting data in these. It also provides an incentive for completion.

A first iteration of improvement could be to decouple the data and the calculation operation.

First, define an array with a bunch of scores.

Then, loop on this array and compute it:

/**
 * Computes the quality of a contact, according profile completion
 *
 * @return float The score calculated from profile completion (between 0 and 1)
 */
public function getQualityAttribute()
{
    $propertiesScore = [
        'firstname' => 1,
        'lastname' => 1,
        'nin' => 5,
        'birthdate' => 2,
        'mobile' => 4,
        'email' => 4,
        'postal_address' => 3,
    ];
    $totalScore = array_sum($propertiesScore);

    $qualityScore = 0;
    foreach ($propertiesScore as $property => $score)
    {
        if (isset($this->$property))
        {
            $qualityScore += $score;
        }
    }

    return $qualityScore / $totalScore * 100;
}

This is a first naive approach to illustrate the method, but introduces something not desirable, $this->$property: this works, but it's not easy to parse by a static analyzer, as the type is not easy to determine. Normally, such code should be replaced by an array.

A more robust approach is so something like this:

    $qualityScore = 0;
    $instanceProperties = get_class_vars($this);
    foreach ($propertiesScore as $property => $score)
    {
        if (isset($instanceProperties[$property]))
        {
            $qualityScore += $score;
        }
    }

Then, we could see the properties score array is a preference. Laravel offers a config repository to store that.

Move this array to config/app.php or better, create a config/quality.php so you'll be able to put future settings too:

<?php

return [

    /*
    |--------------------------------------------------------------------------
    | Quality score per user profiles
    |--------------------------------------------------------------------------
    |
    | This option allows to balance the equation which determines the quality
    | score of a profile, according its level of completion.
    |
    | You can assign a weight (the greater the more important) to each property.
    |
    */

    'scores' => [
        'profile' => [
            'firstname' => 1,
            'lastname' => 1,
            'nin' => 5,
            'birthdate' => 2,
            'mobile' => 4,
            'email' => 4,
            'postal_address' => 3,
        ],
    ],
];

Now you can access it from the getQualityAttribute method using $propertiesScore = Config::get('quality.scores.profile');. Don't forget to add use Config; at the top of your file to get this facade.

Finally, if 0 is never a valid answer, you could use empty instead of isset, so you avoid a bug if the property is "" instead of null (that would give score).


Handling a double click issue I've been doing a lot of form submission using Ajax now and being used to it than having a normal submission. What's been troubling me is that I always ran with a double submission issue when the user clicks twice the submit button to make sure hey hit it.

One way I handle it is by using a flag:

ajaxIsLoading = false; // initiate flag
function listItem() {
  // check if ajax is not loading anymore
  if(!ajaxIsLoading) {
    ajaxIsLoading = true;
    $.post( "/post")
      .done(function( data ) {
       // data successfully submitted
       // set flag to false since post is done
       ajaxLoading = false;
    });
  }
}

One problem I know using this is when your internet connection stops in the middle of it and didn't make through setting the flag back to false.

What I want to ask:

Is this a good way to handle this kind of circumstance?
Is it worth to check your network in case it went down so I can set the flag back to false knowing there's a very little chance it will happen?

Not sure of what is the context for your issue, so I will examine the 2 distinct cases I can think to.
Anyway I'm surprised your code doesn't show any data passed along with the POST: this should probably added in both cases below.

1) You said that "the user clicks twice the submit button": may be it means that you have only one button and "a lot of form submission" are successively done through this unique button.
If this is the case, the usually expected advantage of asynchronously processing is of no value here, and the pretty simple solution is to proceed synchronously, like this:

function listItem() {
    $.post( {url: "/post", async: false} )
      .done(function( data ) {
       // data successfully submitted
    });
}

(this way, no further click will have effect, till process done; also note that we had to change the way we are using $.post(), passing it a plain object as argument)

2) At the opposite, since you have written a function rather than a simple piece of code, you may have a lot of buttons, each one submitting a different form through the same listItem() call.
In this case, IMO the simplest way is to pass the button id to your function, that will disable it before launching POST, then enable it when achieved, like this:

function listItem(btnId) {
    button = $('#' . btnId);
    button.attr( {disabled:'disabled'} );
    $.post( "/post" )
    .done(function( data ) {
        // data successfully submitted
    })
    .always(function() {
        button.removeAttr('disabled');
    });
}

(using .always() ensures the button will be enabled even if POST failed or connection breaks)


Large message population design We are designing a small framework to create and send messages to external parties as a result of internal events.  I am reasonably happy with the design, but I am wandering if it can be further improved.

The external messages are large and complex and involve getting data from different areas in the system and are sent asynchronously when the internal event happens.  A single internal event can cause multiple external messages to be sent.

The internal event (incomingMessage) contains some of the information required to populate the message, and that is provided to the message sending framework, but additional information is required to be retrieved from the database through a potentially expensive query.

The MessageBuilder class builds the messages resulting from the internal event with the help of a list of IMessageContentProvider, which can provide content for different parts of the message. These are passed in through the constructor.

public interface IMessageBuilder<in TIncoming>
{
    IEnumerable<object> Build(TIncoming incomingMessage);
}
 public class MessageBuilder<TIncoming, TOutgoing> : IMessageBuilder<TIncoming> where TOutgoing : class, new()
{
    private readonly IMessageContentProvider<TIncoming, TOutgoing>[] contentProviders;

    public MessageBuilder(IMessageContentProvider<TIncoming, TOutgoing>[] contentProviders)
    {
        this.contentProviders = contentProviders;
    }

    public IEnumerable<object> Build(Project project, IServiceBusResolver serviceBusResolver, TIncoming incomingMessage)
    {
        TOutgoing outgoingMessage = BuildOutgoingMessage(project, serviceBusResolver, incomingMessage);

        return outgoingMessage != null ? new[] { outgoingMessage } : null;
    }

    protected virtual TOutgoing BuildOutgoingMessage(Project project, IServiceBusResolver serviceBusResolver, TIncoming incomingMessage)
    {
        TOutgoing outgoingMessage = new TOutgoing();

        foreach (IMessageContentProvider<TIncoming, TOutgoing> contentProvider in contentProviders)
        {
            contentProvider.Populate(incomingMessage, ref outgoingMessage);
        }

        return outgoingMessage;
    }

The external message definition also requires some translation from internal types to external message types, these are enumerations and data structures used in the external large message. This is done in a static converter class (this seems a bit clunky), which has a series of methods to convert specific parts of the message. All required data is passed in as parameters.

public static class XXMessageConverter
{
    public static TypeA GetTypeA(InternalTypeA internalTypeA, OtherData1 data, OtherData2 data2)
    {
    }
    // ... More getter methods
}

One thing that worries me is that the content providers are independent of each other and there is nothing stopping one provider overwriting data populated by another. Also the order of the providers passed in to the MessageBuilder is important. I have been thinking whether using the fluent builder pattern will be more appropriate and whether message type conversion should happen in the builder or in a separate class.

I added a new type ITypeConverter and started adding a series of converters for translating messages. It's simple at this stage, I may have to add some context to it as some messages require external information to convert.

public interface ITypeConverter<in TSource, out TDestination>
{
    TDestination Convert(TSource source);
}

public IEnumerable<object> Build(Project project, IServiceBusResolver serviceBusResolver, TIncoming incomingMessage)
{
    TOutgoing outgoingMessage = BuildOutgoingMessage(project, serviceBusResolver, incomingMessage);

    return outgoingMessage != null ? new[] { outgoingMessage } : null;
}

Why should this return an IEnumerable<T> ? This just doesn't make sense at all. BuildOutgoingMessage() is only building one object.

The BuildOutgoingMessage() method doesn't use the serviceBusResolver parameter. If this is because it is only for overriding in a derived class, you should consider to add some documentation for this case.

The MessageBuilder class states that it implements IMessageBuilder<TIncoming> but it doesn't implement its method.