Converting words/numbers to Discord "letter/number emojis" I am a beginner to Python (and programming overall) and I decided that it would be funny if I wrote a program that could convert Letters/Numbers to these
"Letter Emojis" in Discord.
It works but I am quite unsure if this is the best way to do it.
There are also some bugs caused by adding spaces before actually typing Letters/Numbers.
I commented it as precisely as I could to make the Code as understandable as possible

from tkinter import *

root = Tk()

# defines the funtion click which gets called when the submitButton1 is pressed
def click():
    # makes sure the list is empty in case the user presses the button again to convert something new
    result.clear()
    # saves the text which is entered in the textentry box, this later gets converted to "Discord Letters"
    entered_text=textentry.get()
    # deletes the output box in case the user presses the button again to convert something new
    output.delete(1.0, END)
    # the function to convert a String to "Discord Letters"
    discord(entered_text)
    # loop which gets the length of the string typed in the textentry box and then outputs it to the output textbox
    for i in range(len(entered_text)):
        output.insert(END, result[i])

# A List in which the converted Letters are stored it's later used to output the converted Letters in a Textbox
result = []

# the function to convert a String to "Discord Letters"
def discord(word):
    # List of numbers from 0-9 , is used to check the string for numbers and then convert them
    # to "Discord Digits" since they have a different syntax in discord than Letters
    chars = set('0123456789')
    s = {'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v',
         'w', 'x', 'y', 'z'}
    # In case the User enters spaces they get removed since they just cause trouble
    word = word.replace(" ", "")
    word = word.lower()
    w = word
    for i in range(len(w)):
        # checks if the string has a number and outputs accordingly
        if any((c in chars) for c in w[i]):
            list_one = ["zero", "one", "two", "three", "four", "five", "six", "seven", "eight", "nine", ]
            result.append(':' + list_one[int(w[i])] + ":" + " ")
        # checks if the string has letter and outputs accordingly (might be unnecessary)
        elif any((c in s) for c in w[i]):
            result.append(':regional_indicator_' + w[i] + ":" + " ")
        else:
            # In case the user inputs something wrong
            print("False Input")

root.title("Discord Letter Converter")
# TextInput
textentry = Entry(root, width=20, bg="white")
textentry.pack()
# submitButton
submitButton1 = Button(root, text="Submit", width=6, command=click)#
submitButton1.pack()
# TextOutput
output = Text(root, width=75, height=6, wrap=WORD, background="white")
output.pack()
root.mainloop()
```

Disclaimer: I know nothing about these "Discord letters"

Code organisation

You could reorganise your code to make units easier to use independently.

The easiest thing to do it to re-write the discord function so that it returns a value instead of updating a global variable.

Another thing to do is to put all the Tkinter logic into a function. Usually, that function actually doing things would be put behind an if __name__ == "__main__" guard.

We'd have

# the function to convert a String to "Discord Letters"
def convert_to_discord_letters(word):
    # A List in which the converted Letters are stored it's later used to output the converted Letters in a Textbox
    result = []
    # List of numbers from 0-9 , is used to check the string for numbers and then convert them
    # to "Discord Digits" since they have a different syntax in discord than Letters
    chars = set('0123456789')
    s = {'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v',
         'w', 'x', 'y', 'z'}
    # In case the User enters spaces they get removed since they just cause trouble
    word = word.replace(" ", "")
    word = word.lower()
    w = word
    for i in range(len(w)):
        # checks if the string has a number and outputs accordingly
        if any((c in chars) for c in w[i]):
            list_one = ["zero", "one", "two", "three", "four", "five", "six", "seven", "eight", "nine", ]
            result.append(':' + list_one[int(w[i])] + ":" + " ")
        # checks if the string has letter and outputs accordingly (might be unnecessary)
        elif any((c in s) for c in w[i]):
            result.append(':regional_indicator_' + w[i] + ":" + " ")
        else:
            # In case the user inputs something wrong
            print("False Input")
    # print(result)
    return result

# defines the funtion click which gets called when the submitButton1 is pressed
def click():
    # saves the text which is entered in the textentry box, this later gets converted to "Discord Letters"
    entered_text=textentry.get()
    # deletes the output box in case the user presses the button again to convert something new
    output.delete(1.0, END)
    # the function to convert a String to "Discord Letters"
    result = convert_to_discord_letters(entered_text)
    # loop which gets the length of the string typed in the textentry box and then outputs it to the output textbox
    for i in range(len(entered_text)):
        output.insert(END, result[i])

def tkinter_discord_letter_converter():
    root = Tk()
    root.title("Discord Letter Converter")
    # TextInput
    textentry = Entry(root, width=20, bg="white")
    textentry.pack()
    # submitButton
    submitButton1 = Button(root, text="Submit", width=6, command=click)#
    submitButton1.pack()
    # TextOutput
    output = Text(root, width=75, height=6, wrap=WORD, background="white")
    output.pack()
    root.mainloop()

if __name__ == '__main__':
    tkinter_discord_letter_converter()

Note: I do not have tested this because I do not have Tkinter but I've tested parts of your code because I have...

Unit Tests

Now that the code is reorganised, we can easily write small tests for the discord function that I've renamed test_convert_to_discord_letters

def test_convert_to_discord_letters():
    """Test function convert_to_discord_letters."""
    # TODO: This could/should use a proper unit-test framework
    # Empty case
    assert convert_to_discord_letters("") == []
    # Whitespace
    assert convert_to_discord_letters(" ") == []
    # Special characters
    assert convert_to_discord_letters("#~&$£()[]") == []
    # Lowercase letters
    assert convert_to_discord_letters("abcz") == [':regional_indicator_a: ', ':regional_indicator_b: ', ':regional_indicator_c: ', ':regional_indicator_z: ']
    # Uppercase letters
    assert convert_to_discord_letters("ABCZ") == [':regional_indicator_a: ', ':regional_indicator_b: ', ':regional_indicator_c: ', ':regional_indicator_z: ']
    # Digits
    assert convert_to_discord_letters("42") == [':four: ', ':two: ']
    # Mix
    assert convert_to_discord_letters("Bar -_- 9") == [':regional_indicator_b: ', ':regional_indicator_a: ', ':regional_indicator_r: ', ':nine: ']

Improving test_convert_to_discord_letters

Now that we have unit-tests for the function, we can more safely try to improve it without breaking its behavior.

Loop like a native

I highly recommend Ned Batchelder's talk "Loop like a native" about iterators. One of the most simple take away is that whenever you're doing range(len(iterabme)), you can probably do things in a better way: more concise, clearer and more efficient.

In your case, this gives:

    for char in w:
        # checks if the string has a number and outputs accordingly
        if any((c in chars) for c in char):
            list_one = ["zero", "one", "two", "three", "four", "five", "six", "seven", "eight", "nine", ]
            result.append(':' + list_one[int(char)] + ":" + " ")
        # checks if the string has letter and outputs accordingly (might be unnecessary)
        elif any((c in s) for c in char):
            result.append(':regional_indicator_' + char + ":" + " ")
        else:
            # In case the user inputs something wrong
            pass # print("False Input")

Then, a few things seem more obvious:

we don't really need the w variable. Also, various re-assignment of word are not required
the variations of any((c in XXX) for c in char) look weird once we realise that char is a single character. We could write this: if char in XXX
the variable chars would be better named digits while s would be better named letters

At this stage, we have

# the function to convert a String to "Discord Letters"
def convert_to_discord_letters(word):
    # A List in which the converted Letters are stored it's later used to output the converted Letters in a Textbox
    # List of numbers from 0-9 , is used to check the string for numbers and then convert them
    # to "Discord Digits" since they have a different syntax in discord than Letters
    digits = set('0123456789')
    letters = {'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z'}
    result = []
    for char in word.replace(" ", "").lower():
        # checks if the string has a number and outputs accordingly
        if char in digits:
            list_one = ["zero", "one", "two", "three", "four", "five", "six", "seven", "eight", "nine", ]
            result.append(':' + list_one[int(char)] + ":" + " ")
        # checks if the string has letter and outputs accordingly (might be unnecessary)
        elif char in letters:
            result.append(':regional_indicator_' + char + ":" + " ")
        else:
            # In case the user inputs something wrong
            pass # print("False Input")
    # print(result)
    return result

Definitions of data used

For letters, you could use ascii_lowercase from string module:

import string
...
    letters = set(string.ascii_lowercase)

For digits, you define a set and a list, convert the value to integer to get the string at the relevant index. It seems like the best data structure to use would be a single dictionnary mapping digits to strings.

    digits = {"0": "zero", "1": "one", "2": "two", "3": "three", "4": "four", "5": "five", "6": "six", "7": "seven", "8": "eight", "9": "nine"}

Useless concatenations of literal string

":" + " " can be written ": ".

At this stage, we have:

# the function to convert a String to "Discord Letters"
def convert_to_discord_letters(word):
    # A List in which the converted Letters are stored it's later used to output the converted Letters in a Textbox
    # List of numbers from 0-9 , is used to check the string for numbers and then convert them
    # to "Discord Digits" since they have a different syntax in discord than Letters
    digits = {"0": "zero", "1": "one", "2": "two", "3": "three", "4": "four", "5": "five", "6": "six", "7": "seven", "8": "eight", "9": "nine"}
    letters = set(string.ascii_lowercase)
    result = []
    for char in word.replace(" ", "").lower():
        # checks if the string has a number and outputs accordingly
        if char in digits:
            result.append(':' + digits[char] + ": ")
        # checks if the string has letter and outputs accordingly (might be unnecessary)
        elif char in letters:
            result.append(':regional_indicator_' + char + ": ")
        else:
            # In case the user inputs something wrong
            pass # print("False Input")
    print(result)
    return result

And we realise that most comments are not required anymore because the code is much clearer. On the other hand, we could add a proper docstring.

def convert_to_discord_letters(word):
    """Convert a string into a list of strings corresponding to "Discord Letters"."""
    digits = {"0": "zero", "1": "one", "2": "two", "3": "three", "4": "four", "5": "five", "6": "six", "7": "seven", "8": "eight", "9": "nine"}
    letters = set(string.ascii_lowercase)
    result = []
    for char in word.replace(" ", "").lower():
        if char in digits:
            result.append(':' + digits[char] + ": ")
        elif char in letters:
            result.append(':regional_indicator_' + char + ": ")
        else:
            pass # TODO: I'll let you decide how to handle other cases
    return result

Removing the useless logic

It is now clearer that the .replace(" ", "") logic removes whitespace which would have no effect anyway (as we'd end up in the non-handled case).
You can remove this.

More

I suspect the click function could benefit from the same ideas but it can't test it at the moment.

Also, the code seems to assume that entered_text and result will have the same length which may not be the case for non-handled characters.


Optimizing ByteString escaping I wrote a string escaping function in C, and I'm trying to rewrite it to Haskell.  The C version, along with comments explaining why it does what it does, can be found on GitHub.

Here's a naïve implementation based on Data.ByteString.concatMap:

{-# LANGUAGE ViewPatterns #-}
import Data.Bits
import Data.ByteString          (ByteString)
import Data.ByteString.Char8    ()
import Data.ByteString.Internal (c2w, w2c)

import qualified Data.ByteString        as B
import qualified Data.ByteString.Lazy   as L

escapeCopyBytea :: ByteString -> ByteString
escapeCopyBytea = B.concatMap f
    where
        f (w2c -> '\\') = B.replicate 4 (c2w '\\')
        f c | c >= 32 && c <= 126 = B.singleton c
        f c = B.pack
                [ c2w '\\'
                , c2w '\\'
                , c2w '0' + ((c `shiftR` 6) .&. 0x7)
                , c2w '0' + ((c `shiftR` 3) .&. 0x7)
                , c2w '0' + (c .&. 0x7)
                ]

mapChunks :: (ByteString -> ByteString) -> L.ByteString -> L.ByteString
mapChunks f = L.fromChunks . map f . L.toChunks

main :: IO ()
main = L.getContents >>= L.putStr . mapChunks escapeCopyBytea

I'd expect this to be a few times slower than the C version.  Nope, it is 125 times slower than the C version.

Then, I tried using blaze-builder:

import Blaze.ByteString.Builder
import Data.Bits
import Data.Monoid              (mappend, mconcat, mempty)
import Data.ByteString          (ByteString)
import Data.Word                (Word8)

import qualified Data.ByteString        as B
import qualified Data.ByteString.Lazy   as L

writeBackslash :: Write
writeBackslash = writeWord8 92

escape1 :: Word8 -> Builder
escape1 92 = fromWrite $ writeBackslash
               `mappend` writeBackslash
               `mappend` writeBackslash
               `mappend` writeBackslash
escape1 c | c >= 32 && c <= 126 = fromWrite $ writeWord8 c
          | otherwise = fromWrite $ writeBackslash
                          `mappend` writeBackslash
                          `mappend` writeWord8 (48 + ((c `shiftR` 6) .&. 0x7))
                          `mappend` writeWord8 (48 + ((c `shiftR` 3) .&. 0x7))
                          `mappend` writeWord8 (48 + (c .&. 0x7))

escapeCopyBytea2 :: ByteString -> Builder
escapeCopyBytea2 = B.foldl' f mempty
    where
        f b c = b `mappend` escape1 c

main :: IO ()
main = L.getContents >>= L.putStr . toLazyByteString . mconcat
                       . map escapeCopyBytea2 . L.toChunks

This made a difference.  It's even slower, 300 times slower than the C version.  I thought blaze-builder was supposed to be really fast!

Simply summing bytes, by folding over the input in a similar fashion as I do above, is reasonably fast (takes 5 times longer than the C version of the escaping code):

import qualified Data.ByteString      as B
import qualified Data.ByteString.Lazy as L

import Data.List (foldl')

main :: IO ()
main = L.getContents >>= print . foldl' (+) 0 . map (B.foldl' (+) 0) . L.toChunks

What can I do to make this escaping function faster?  Is it possible to get anywhere near the performance of C here, or is C (or perhaps working with Ptr or Addr# directly and doing it the C way) the only viable option to make this efficient?

Edit: I wrote a much faster implementation and put it on GitHub.  However, it uses a lot of ugly buffer manipulation.  I'd still like to know if there's a simpler way to escape bytes in Haskell that isn't slow.

Whew, this is quite a tricky one. The main problem here is that you really, really need to keep Haskell from allocating anything in the relatively spacious inner loop. Otherwise, you are looking at a dozen bytes going through the garbage collector for every byte you escape.

As you note, blaze-builder has a reputation for being fast, mainly for being pretty good at this. Let's start off with an explanation why that is. Here's roughly how a Builder is defined (see Blaze.ByteString.Builder.Internal.Types):

newtype Builder = Builder (forall r. BuildStep r -> BuildStep r)
newtype BuildStep a = BufRange -> IO (BuildSignal a)
data BufRange a = BufRange !(Ptr Word8) !(Ptr Word8)

The compiler will replace all newtype wrappers with simple "casts", and un-box BufRange where possible. So this means that, say, your escape1 function would actually have an after-optimization type closer to:

escape1 :: Word8 -> (BufRange -> IO (BuildSignal a)) -> BufRange -> IO (BuildSignal a)

So this actually just takes the pointers from BufRange to write data using the IO monad, then finally calls the continuation. Ideally, we even know the continuation in question and can jump directly to the code. At no point does this require heap-allocation, everything gets nicely optimized out.

So, what's the problem in your case? The trouble is the "glue code". Let us look at the source of ByteString's foldl, with some additional type annotations:

lgo :: Builder -> Addr# -> Addr# -> IO Builder
lgo !z !p !q | p == q    = return z
             | otherwise = do c <- peek p
                              lgo (f z c) (p `plusPtr` 1) q

For every byte, this worker loop forces f in order to get the new "accumulated value". Unfortunately, that's a pretty bad idea here, as our accumulator is a Builder - a closure (as we saw above). Therefore this will actually allocate a huge list of closures, but not start producing anything of value until the very end.

So, how to improve this? Remember again that our f has type ... -> BuildStep r -> BuildStep r somewhere deep down. We would actually like to pass lgo as the BuildStep continuation, so after inlining it becomes a simple jump. We also don't really need the accumulation parameter, as the Builder is taking care of accumulating its result by itself.

Going through with this, we actually get a lazy right fold:

lgo :: Addr# -> Addr# -> BuildStep r -> BuildStep r
lgo !p !q | p == q    = v
          | otherwise = f (inlinePerformIO (peek p)) (lgo (p `plusPtr` 1) q)

(Note I also removed the IO by using inlinePerformIO, which might be unsafe in other usage scenarios, as any addresses escaping due to lazy evaluation might point to a garbage-collected buffer later!)

The type signature looks a lot better now - and from my testing this actually performs roughly twice as fast as the original version. Where does the rest of the performance get lost? Well, a look at the Core shows code like

lgo = \p q -> case p == q of
                True -> \r -> let ... in \stp -> ...
                False ->

This means that GHC is generating a lot of partially-applied functions - primarily because the optimizer doesn't want to lose sharing in case we want to call lgo with the same p and q, but different BuildSteps (which we never will, but GHC can't prove that).

So we need to force GHC to use higher arity for this function's implementation. Unfortunately, I am somewhat out of ideas how to accomplish that in an elegant way. So here's the manual solution -- writing our lambdas out at the top of the expression:

foldBuilder :: (Word8 -> Builder) -> B.ByteString -> Builder
foldBuilder f (PS x s l) =
  fromBuildStepCont $ \cont range ->
    withForeignPtr x $ \ptr -> do
      let lgo !p !q !range'
            | p == q    = cont range'
            | otherwise = do
                c <- peek p
                let p' = p `plusPtr` 1
                    step = unBuilder (f c) (BuildStep $ lgo p' q)
                runBuildStep step range'
      lgo (ptr `plusPtr` s) (ptr `plusPtr` (s+l)) range

I get decent performance out of this version here, and I suppose it's slightly better than implementing it completely by hand. It also happens to not require inlinePerformIO at all any more, which is a nice plus.

Code can be found at GitHub. I hope this helps in some way :)


Converting models into a list of ViewModels I have the following code:

public abstract class BaseViewModel<PrimaryKey, ModelType>
{
    //Some base fields

    public static List<T> Convert<T>(IEnumerable<ModelType> collection)
        where T : BaseViewModel<PrimaryKey, ModelType>, new()
    {
        var list = new List<T>();

        if (collection != null)
        {
            foreach (var item in collection)
            {
                var view = new T();
                view.Value = item;

                list.Add(view);
            }
        }

        return list;
    }
}

Of course, it works because it requires to I set a generic type.

var list = GroupViewModel.Convert<GroupViewModel>(model);

There is GroupViewModel extends from BaseViewModel. But how can I reduce this generic casting to it will get GroupViewModel type automaticaly when I call it so: var list = GroupViewModel.Convert(model);.
Is it possible? Just I learn C# only.

I don't know what to say more then:

make it an extension and return an IEnumerable<T> instead by using linq

do you mean something like this?

public static IEnumerable<T> Convert<T>(this IEnumerable<ModelType> modelTypes)
    where T : BaseViewModel<PrimaryKey, ModelType>, new()
{
    return modelTypes.Select(x => new T { Value = x });
}

use it:

var groupViewModels = modelTypes.Convert<GroupViewModel>().ToList();


JavaScript naming and grouping I'm fairly new to JavaScript. I would like some feedback on the naming and grouping of the following code. Is there any change that will make it more readable?

Template.documentPage.events({
  'click .hint-text': function(e) {
    e.preventDefault()

    var currentHintId = $(e.target).attr('id')
    Session.set('currentHintId', currentHintId)

    $('.hint-popup').show()
  },

  'click .hint-submit': function(e) {
    e.preventDefault()

    var currentHintId = Session.get('currentHintId')
    var popupInput = $('.hint-popup input').val()

    $('#' + currentHintId).text(popupInput)
    Documents.update(this._id, {$set: {content: $('.content').html()}}, function() {
      console.log('Saved.')
    })

    $('.hint-popup').hide()
    $('.hint-popup input').val('Enter text')
  }
})

To start off, I can't see a single semicolon being used here. You should always close your statements with a semicolon.

Also I can't quite understand what you've done here:

'click .hint-text': function(e) {
    e.preventDefault()

    var currentHintId = $(e.target).attr('id')
    Session.set('currentHintId', currentHintId)

    $('.hint-popup').show()
},

Is this a property in an object? If so why have you named it 'click .hint-text'? Give it a proper name such as togglePopup or something along those lines to make it more obvious what it does. Now it's just confusing.

And for the other function you could name it submit(whatever you're submitting).

If you included some additional markup it would help your question a bit.


Javascript singleton to notify different objects of changes in viewport size I'm working on a singleton object to manage size changes in the viewport, I wanted it to be a singleton because there may be different unrelated objects that need to be notified when a change in viewport dimensions has occurred.
So I came up with this solution based on the singleton design pattern found in here.

var CAResizeManager = ( function () {

    var instance;

    function init() {

        var o_actions   = [];
        var o_cache_pos = {};
        var o_timeout;

        /*
         * Custom 'addEvent' function;
         */
        addEvent( window, 'resize', function () {

            /*
             * Clear timeout if resize is called severla times in short timespan.
             */
            clearTimeout( o_timeout );

            o_timeout = setTimeout( function () {

                var o_viewport_size = getViewportSize();
                /*
                 * Loop through the actions array to pass new viewport size;
                 */
                for ( var i = o_actions.length - 1; i >= 0; i-- ) {
                    var o_func = o_actions[ i ];

                    if ( o_func ) {
                        o_func( o_viewport_size );
                    }
                }

            } ,300 );

        } );

        return {
            addAction: function ( arg_func ) {

                /*
                 * Generates a unique id to identify action.
                 */
                var i_id = Math.random().toString().replace( '0.', '' );

                o_actions.push( arg_func );

                /*
                 * Store position in an object to avoid looping through the
                 * actions array.
                 */
                o_cache_pos[ i_id ] = o_actions.length === 0 ? 0 : o_actions.length - 1;

                /*
                 * Return id to external object.
                 */
                return i_id;
            },
            removeAction: function ( arg_id ) {

                var i_index = o_cache_pos[ arg_id ];

                /*
                 * Check if id exists;
                 */
                if ( i_index ) {
                    return;
                }

                /*
                 * Remove action from array
                 */
                o_actions.splice( i_index, 1 );

                /*
                 * Delete positon from cache;
                 */
                delete o_cache_pos[ arg_id ];

            }
        };
    }

    return {
        getInstance: function () {

            if ( !instance ) {
                instance = init();
            }

            return instance;

        }
    }

} )();

Here's a simplified version of your code:

var ResizeManager = (function(addEvent, getViewportSize){

  var DEBOUNCE_DELAY = 300;
  var debounceTimer = null;
  var trackedEvents = {};

  addEvent(window, 'resize', function() {
    if(debounceTimer) clearTimeout(debounceTimer);
    debounceTimer = setTimeout(function(){
      Object.keys(trackedEvents).forEach(function(handler){
        handler.call(null, getViewportSize());
      });
    }, DEBOUNCE_DELAY);
  });

  return {
    addAction : function(callback){
      var actionId = ('' + Math.random()).slice(2);
      trackedEvents[actionId] = callback;
      return actionId;
    },
    removeAction: function(callbackId){
      delete trackedEvents[callbackId];
    }
  };

}(addEvent, getViewportSize));

Here's the changes:

Dropped getInstance. Since it's a singleton, let's treat the ResizeManager as the instance, instead of still doing a getInstance step.
Using an object (hash) instead of an array to collect your handlers. The advantage is that you can immediately delete the action by ID instead of having to search for it befor delete.
Replaced your ID generator with a much sleeker alternative.
Naming variables properly, as well as using the constant convention (all caps, underscore-spaced) for constant values.
Calling your function with the context of null. That way, your handlers won't expect a context and potentially do something weird.
Localizing references to the global functions. That way, if ever they get re-assigned a value, you still hold a reference to them.


Downsampling boundaries of a 2D array I have a linearized 2D array u (block_height * block_width) containing the values of a physical quantity over a regular 2D mesh. I need to downsample boundaries (top, bottom, left, right) of this array for communication with another process. I've refactored some code that does this, and I'd like people's thoughts on whether the new code is actually an improvement.

Old code:

if (dir == LEFT)
    for(int j = 1; j <= block_height; j += 2)
        left_edge[j/2] = (u[index(1,j)]   + u[index(2,j)] +
                          u[index(1,j+1)] + u[index(2,j+1)]) / 4;
else if (dir == RIGHT)
    for(int j = 1; j <= block_height; j += 2)
        right_edge[j/2] = (u[index(block_width-1, j)]   + u[index(block_width,j)] +
                           u[index(block_width-1, j+1)] + u[index(block_width, j+1)]) / 4;
else if (dir == UP)
    for(int i = 1; i <= block_width; i += 2)
        top_edge[i/2] = (u[index(i,1)]   + u[index(i,2)] +
                         u[index(i+1,1)] + u[index(i+1,2)]) / 4;
else if (dir == DOWN)
    for(int i = 1; i <= block_width; i += 2)
        bottom_edge[i/2] = (u[index(i,block_height-1)]   + u[index(i,block_height)] +
                            u[index(i+1,block_height-1)] + u[index(i+1,block_height)]) / 4;

New code:

double *boundary;
int k, fixed_dim;
int *x, *y;
switch (dir) {
case LEFT: case RIGHT: count = block_height; x = &fixed_dim; y = &k; break;
case UP:   case DOWN:  count = block_width;  y = &fixed_dim; x = &k; break;
switch (dir) {
case LEFT:  boundary = left_edge;   fixed_dim = 1;                break;
case RIGHT: boundary = right_edge;  fixed_dim = block_width - 1;  break;
case UP:    boundary = top_edge;    fixed_dim = 1;                break;
case DOWN:  boundary = bottom_edge; fixed_dim = block_height - 1; break;
}
for (k = 1; k <= count; k += 2)
  boundary[k/2] = (u[index(*x, *y)]   + u[index(*x+1, *y)] +
                   u[index(*x, *y+1)] + u[index(*x+1, *y+1)] )/ 4;

So, I've factored out the repeated for loop and made the logic more declarative, at the expense of introducing some additional indirection (including over which direction the loop iterates through memory!) and the variables to support it.

One obvious direction of future change for this code is increasing from 2 dimensions to 3 (and possible even more). I know which one I'd prefer to do that to, but if the old code is overwhelmingly clearer to people reading it, then the repetition may be worth retaining.

I think the first one is more understandable to a reader, while the second is more efficient and or more easily adapted to higher dimensions. Have you thought about something like:

if (dir == LEFT || dir == RIGHT){
  int x = dir == LEFT ? 1 : block_width-1;
  double *boundary = dir == LEFT ? left_edge : right_edge;
  for(int j = 1; j <= block_height; j += 2)
    left_edge[j/2] = (u[index(x,j)]   + u[index(x+1,j)] +
                      u[index(x,j+1)] + u[index(x+1,j+1)]) / 4;
}
else if (dir == UP || dir == DOWN){
  int y = dir == UP ? 1 : block_height-1
  double *boundary = dir == UP ? top_edge : bottom_edge;
  for(int i = 1; i <= block_width; i += 2)
    boundary[i/2] = (u[index(i,y)]   + u[index(i,y+1)] +
                     u[index(i+1,y)] + u[index(i+1,y+1)]) / 4;
}

Or if the second is more inclined I would recommend just clear comments to show the intention of each variables meaning.

Also in the future adapting to more c++ style arrays (vectors), utilizing a c++ Matrix library, or making your own iterator (boundary iterator) would make for a very nice solution to this problem.


Collatz Conjecture in Python The Collatz's Conjecture states that any number can either be halved (if it is even) or multiplied by three and added one to (if it is odd) and eventually reach 1.

I was wondering if there was a more efficient way to work out the series that a number takes to get to one and the amount of steps it takes.  The Python (3.x) code is:

i, k = 1, 1
fh=open("results.txt", "w")
print("Started")
def colapatz(x):
    seq = [x]
    j = 0
    while x > 1:
       if x % 2 == 0:
         x = x / 2
         j = j + 1
       else:
         x = 3 * x + 1
         j = j + 1
       seq.append(x)
    fh.write("Value number " + str(i) + " takes "+ str(j) + " steps.\n")
    fh.write("It takes the sequence: " + str(seq) + "\n")

#Call the function
while k<10000:
    k+=1
    i+=1
    colapatz(i)
print("Finished")
fh.close()

The is one of my first python programs that I've ever written, so any improvements would be great.

One possible improvement is to use recursion and memoization to implement a "dynamic programming" solution. This uses the results of previous calculations to speed up the process, as you will end up calculating the same sub-sequences repeatedly. For example:

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

@memo
def collatz(n):
    if n == 1:
        count, seq = 0, []
    elif n % 2:
        count, seq = collatz(3 * n + 1)
    else:
        count, seq = collatz(n // 2)
    return count + 1, [n] + seq

(Note the use of a decorator to implement the memoization.)

Now, once you've run:

>>> collatz(10)
7, [10, 5, 16, 8, 4, 2, 1]

then e.g. collatz(8) can just be looked up from the cache, so future calculations aren't needed:

>>> collatz.cache
{(1,): (1, [1]), (2,): (2, [2, 1]), (8,): (4, [8, 4, 2, 1]),
 (4,): (3, [4, 2, 1]), (10,): (7, [10, 5, 16, 8, 4, 2, 1]),
 (5,): (6, [5, 16, 8, 4, 2, 1]), (16,): (5, [16, 8, 4, 2, 1])}

The downsides of this are:

It uses up a lot of space; and
As it's recursive, a long sequence can hit the recursion limit and crash the program.

Also, note that count == len(seq), so you don't necessarily have to return (and store) both.


Encryption functions based on mcrypt I learned here that it is unsafe to design encryption algorithms from scratch. Given that advice, I made a pair of encryption functions based on mcrypt:

function aes128ctr_en($data,$key,$hmac = false) {
    $key = ($hmac===false) ? hash('sha256',$key,true) : hash_hmac('sha256',$key,$hmac,true);
    $data .= hash('md5',$data,true);
    $iv = mcrypt_create_iv(16,MCRYPT_DEV_URANDOM);
    return mcrypt_encrypt('rijndael-128',$key,$data,'ctr',$iv).$iv;
}

function aes128ctr_de($data,$key,$hmac = false) {
    $key = ($hmac===false) ? hash('sha256',$key,true) : hash_hmac('sha256',$key,$hmac,true);
    $iv = substr($data,-16);
    $data = substr($data,0,strlen($data)-16);
    $data = mcrypt_decrypt('rijndael-128',$key,$data,'ctr',$iv);
    $md5 = substr($data,-16);
    $data = substr($data,0,strlen($data)-16);
    return (hash('md5',$data,true)===$md5) ? $data : false;
}

$encrypted = aes128ctr_en('secret text','password');
echo aes128ctr_de($encrypted,'password');

Are these safe?
What about the IV? Is it ok to just add it to the end of the encrypted string?
Would it be better/faster to make all this by module, that is by using mcrypt_module_open?

No it is not secure.
For simplicity assume that your data D is exactly one block long.
Then the ciphertext contains 3 16-byte blocks as follows.

C0 || C1 || C2
=  D xor AES(IV) || MD5(D) xor AES(IV+1) || IV

(The IV is normally prepended to the message, but that is rather irrelevant here).
If an attacker can guess D then the attacker can derive AES(IV) and AES(IV+1) and subsequently
generate another valid ciphertext by computing

D' xor AES(IV) || MD5(D') xor AES(IV+1) || IV

for whatever message D' he likes.
An unkeyed hash just does not give the integrity that you would like. As usual one should use a HMAC or something similar instead of the MD5. Hence the biggest weakness in your protocol is not the primitives that you use, but the fact that you are not using them properly. And just to answer another question posed here: People on SO are far from being competent enough to find every flaw in a new protocol. So you'd better look for an existing standard.
Let me also add, that protocols similar to the one here have been proposed in the 80s. E.g., Kerberos had a similar flaw. Better message authentications such as HMAc were designed exactly to prevent this kind of ciphertext manipulations.


Remove determiners in a string I'm working on a program that aim to take sentences (currently in french) and compact them to a length of 38 characters while retaining as much information as possible.
You can find another part of this project here replacing words with their abbreviations

I've made a function that removes determiners from a string using an external config file containing said determiners. It is functionnal but I think there's a lot to improve.

Here it is:

# determiners is the config file below parsed by configparser, l1 is the string
def remove_determiners(determiners, l1):
    for key in determiners:
        it_l1 = 0

        # the goal is to obtain a string shorter than 38 char
        while(it_l1 < len(l1) and len(l1) > 38):

             # finds possible beginnig of words
             if(l1[it_l1] in ' -' or it_l1 == 0):
                 if(it_l1 == 0):
                      it_l1 = -1

                 # finds possible end of words
                 it_word = 0
                 while(it_word < len(key) and it_l1 + it_word + 1 < len(l1)
                 and l1[it_l1 + it_word + 1] == key[it_word].upper()):
                      it_word += 1
                 if(it_word == len(key) and (it_l1 + it_word + 1 == len(l1)
                 or l1[it_l1 + it_word + 1] in ' -.')):
                      if(it_l1 == -1):
                          it_l1 = 0

                      # cuts the determiner out
                      l1 = l1[:it_l1] + l1[it_l1 + it_word + 1:]

             it_l1 += 1
             if(it_l1 == 0):
                 it_l1 = 1
    return(l1)

Here is the configfile: (mostly french determiners)

[remove]
& = nope
A = nope
AND = nope
AU = nope
AUX = nope
D = nope
DE = nope
DES = nope
DU = nope
EN = nope
FOR = nope
ET = nope
L = nope
LA = nope
LE = nope
LEUR = nope
LEURS = nope
LES = nope
OF = nope
OU = nope
PAR = nope
POUR = nope
SA = nope
SON = nope
SUR = nope
THE = nope
UN = nope
UNE = nope

Here is how its parsed:

determiners = configparser.ConfigParser()
determiners.read('configuration//determiners.ini')

And here are some I/O examples:

'JE SUIS LA BAGUETTE.' --> 'JE SUIS BAGUETTE.'
'LES PILES DE LA TELECOMMANDE.' --> 'PILES TELECOMMANDE.'
'QU ELLE HEURE EST IL ?' --> 'QU ELLE HEURE EST IL ?'

You may note the strings have been formatted to remove special characters and to be all upcase

Thanks for sharing your code.

It's a nice project you have there.

Naming

You should take some time to choose carefully your variable. l1 is not obvious, maybe input, input_string, sentence or text would be more appropriate?

Without the commentary above the function, it would have been hard to find what that was supposed to be.

Same for it_l1 and it_word. After reading your code a few time, I am not sure what they are. Find a descriptive name, it helps tremendously when reading code.

Documentation

You have comments, that's good. But it is way better to have a good docstring for the function, and fewer comments.

Your docstring should describe what the function try to accomplish, why it exists, the parameter (eventually their type) and what does the function return.

And more annoying, some of your comments seem to be outdated/wrong?

# determiners is the config file below parsed by configparser but it seems determiners is not a config file, but a list of string. (True it is the content of a file parsed by configparser, but it's not the same)
# the goal is to obtain a string shorter than 38 char you did change your code but did not change that comment I presume? I couldn't find where it was supposed to restrict the size to 38char

Indentation

Your code here:

while(it_word < len(key) and it_l1 + it_word + 1 < len(l1)
and l1[it_l1 + it_word + 1] == key[it_word].upper()):
    it_word += 1
if(it_word == len(key) and (it_l1 + it_word + 1 == len(l1)
or l1[it_l1 + it_word + 1] in ' -.')):

is very hard to read. Maybe it's because you pasted it into CodeReview, but be carefull when writing long and hard condition. The more difficult it is to read, the more difficult it is to debug/maintain/extend.

Putting it altogether

Note: it_l1 and it_word are still in the code because I don't know how to name them properly but you should probably change them.

def remove_determiners(determiners, text):
    """
    Removes determiners words from text

    :param determiners: the words to remove in `text`
    :param text: a text to remove determiner in
    :type determiners: List[str]
    :type text: str:
    :return: The value of `text` after removing all words present in determiners
    """
    for key in determiners:
        it_l1 = 0

        while(it_l1 < len(text)):

             # Finds possible beginning of words
             if(text[it_l1] in ' -' or it_l1 == 0):
                 if(it_l1 == 0):
                      it_l1 = -1

                 # Finds possible end of words
                 it_word = 0
                 while(it_word < len(key)
                       and it_l1 + it_word + 1 < len(l1)
                       and text[it_l1 + it_word + 1] == key[it_word].upper()):
                      it_word += 1
                 if(it_word == len(key)
                    and (it_l1 + it_word + 1 == len(text)
                         or text[it_l1 + it_word + 1] in ' -.')):
                      if(it_l1 == -1):
                          it_l1 = 0

                      # cuts the determiner out
                      text = l1[:it_l1] + l1[it_l1 + it_word + 1:]

             it_l1 += 1
             if(it_l1 == 0):
                 it_l1 = 1
    return(text)

Algorithm

You could also really simplify your code by using set.

Below is a code that works for the simple case, need to be improved to deal with punctuation. (if you have a text like "LE, LA, ET.", it will not remove the determiners. Or use a clean up step as shown below)

def remove_determiners(determiners, text):
    """
    Removes determiners words from text

    :param determiners: the words to remove in `text`
    :param text: a text to remove determiner in
    :type determiners: List[str]
    :type text: str:
    :return: The value of `text` after removing all words present in determiners
    """

    determiners_set = set(determiners)

    text_list = text.split(' ')

    resultat = []
    for element in text_list:
        if(element not in determiners_set):
            resultat.append(element)

    return ' '.join(resultat)

Or as a two-liner using list comprehension as @Josay pointed out in the comment:

def remove_determiners(determiners, text):
    determiners_set = set(determiners)
    return ' '.join(e for e in text.split(' ') if e not in determiners_set)

Usage:

determiners = ['AND',
               'THE',
               'ET',
               'LE',
               'LA',
               'LES',
               'DE']

text = 'LES PILES DE LA TELECOMMANDE'

remove_determiners(determiners, text)
# returns 'PILES TELECOMMANDE'

Sidenote

Why in you config file you have & = nope instead of &? Is there more values than nope?

EDIT

Code changed slightly since my answer, adding the 38 char constraint. Should be straightforward to modify my response to accomodate this change

EDIT2

Also to address @Josay point about noise in the text, a cleaning step is possible like this:

def clean_text(text)
    """Remove ponctuation from text and replace it by a empty char
       for -_,.?!:; and '" by a space
       :param text: The text to remove punctuation in
       :return: Text cleaned up
    """
    text = re.sub('[-_,.?!:;]', '', text)
    text = re.sub('[\'"]', ' ', text)
    return text

And usage

determiners = ['AND',
               'THE',
               'ET',
               'LE',
               'LA',
               'LES',
               'DE']

text = 'LES PILES DE-LA TELECOMMANDE'

remove_determiners(determiners, clean_text(text))


HTML Compressor with regex I would like to compress a Magento HTML page using some regex, and this is what I have written:

function html_compress($string){

    global $idarray;
    $idarray=array();

    //Replace PRE and TEXTAREA tags
    $search=array(
                    '@(<)\s*?(pre\b[^>]*?)(>)([\s\S]*?)(<)\s*(/\s*?pre\s*?)(>)@',   //Find PRE Tag
                    '@(<)\s*?(textarea\b[^>]*?)(>)([\s\S]*?)(<)\s*?(/\s*?textarea\s*?)(>)@' //Find TEXTAREA
                );
    $string=preg_replace_callback($search,
                                    function($m){
                                        $id='<!['.uniqid().']!>';
                                        global $idarray;
                                        $idarray[]=array($id,$m[0]);
                                        return $id;
                                    },
                                    $string
    );

    //Remove blank useless space
    $search = array(
                    '@( |\t|\f)+@', // Shorten multiple whitespace sequences
                    '@(^[\r\n]*|[\r\n]+)[\s\t]*[\r\n]+@',   //Remove blank lines
                    '@^(\s)+|( |\t|\0|\r\n)+$@' //Trim Lines
                    );
    $replace = array(' ',"\\1",'');
    $string = preg_replace($search, $replace, $string);

    //Replace IE COMMENTS, SCRIPT, STYLE and CDATA tags
    $search=array(
                    '@<!--\[if\s(?:[^<]+|<(?!!\[endif\]-->))*<!\[endif\]-->@',  //Find IE Comments
                    '@(<)\s*?(script\b[^>]*?)(>)([\s\S]*?)(<)\s*?(/\s*?script\s*?)(>)@',    //Find SCRIPT Tag
                    '@(<)\s*?(style\b[^>]*?)(>)([\s\S]*?)(<)\s*?(/\s*?style\s*?)(>)@',  //Find STYLE Tag
                    '@(//<!\[CDATA\[([\s\S]*?)//]]>)@', //Find commented CDATA
                    '@(<!\[CDATA\[([\s\S]*?)]]>)@'  //Find CDATA
                );
    $string=preg_replace_callback($search,
                                    function($m){
                                        $id='<!['.uniqid().']!>';
                                        global $idarray;
                                        $idarray[]=array($id,$m[0]);
                                        return $id;
                                    },
                                    $string
    );

    //Remove blank useless space
    $search = array(
                    '@(class|id|value|alt|href|src|style|title)=(\'\s*?\'|"\s*?")@',    //Remove empty attribute
                    '@<!--([\s\S]*?)-->@',  // Strip comments except IE
                    '@[\r\n|\n|\r]@', // Strip break line
                    '@[ |\t|\f]+@', // Shorten multiple whitespace sequences
                    '@(^[\r\n]*|[\r\n]+)[\s\t]*[\r\n]+@', //Remove blank lines
                    '@^(\s)+|( |\t|\0|\r\n)+$@' //Trim Lines
                    );
    $replace = array(' ','',' ',' ',"\\1",'');
    $string = preg_replace($search, $replace, $string);

    //Replace unique id with original tag
    $c=count($idarray);
    for($i=0;$i<$c;$i++){
        $string = str_replace($idarray[$i][0], "\n".$idarray[$i][1]."\n", $string);
    }

    return $string;
}

It works, but I have got some concerns:

Has it got sense to explit all the \s*? between the tags and
retrieve with this (<)\s*?(style\b[^>]*?)(>)?
Does this script eat resources and considerably delay the page load? Is there any possible optimization?
Is the part that remove the white space redundant?
Is all of this "cacheable"?

See: https://stackoverflow.com/a/6225706/736079

For comments on enabling content compression for your HTML pages, that usually is enough to reduce the payload by more than 50%.

You can also make use of output buffering and combine it with the HTMLMinify function:

<?php
function sanitize_output($content) {
     $content = Minify_HTML::minify($content);
}

ob_start("sanitize_output");
?>

See: https://github.com/mrclay/minify/blob/master/min/lib/Minify/HTML.php

This still uses Regex at the core, which is still not ideal, but it has been tested by a larger audience and looks quite solid (test to make sure). If you are hosting on IIS, you might be able to use a .NET HttpModule or an ISAPI filter as well. This isn't limited to PHP only, sometimes the Web Server itself has plugins that can help you, like Apache's mod_pagespeed.


Method to encode ordinary characters into html format I have had a problem with encoding in a project when trying to convert certain html code into image format. It happens when we receive html that has certain characters with accents and other non standard ones.

To combat this I have the following method which finds all of the text inside html tags and then converts all of it (that aren't already in the encoded format i.e. &nbsp;) to their html escaped representation i.e. + to &#43;.

private string EncodeToHtml(string contents)
{
    Regex textRegex = new Regex("(?<!<[^>]*)(?<Text>[^<>]*)", RegexOptions.Compiled);
    Regex innerRegex = new Regex("(?<=^|;)([^&]|&(?=[^&;]*(?:&|$)))+", RegexOptions.Compiled);

    return textRegex.Replace(contents, new MatchEvaluator(m =>
    {
        return innerRegex.Replace(m.Groups["Text"].Value, new MatchEvaluator(m2 =>
        {
            string result = string.Empty;
            foreach (char c in m2.Value)
            {
                result += $"&#{(int)c};";
            }

            return result;
        }));
    }));
}

I'd appreciate any comments on the code especially anyway to make it more efficient.

You can make the loop more efficient by using the StringBuilder and using the Aggregate extension:

return m2.Value.Aggregate(
    new StringBuilder(),
    (current, next) => current.Append($"&#{next};")
).ToString())

You don't need the new MatchEvaluator, just the m is OK:

textRegex.Replace(contents, m => ..

You can even compress it by removing all returns:

return textRegex.Replace(contents, m =>
    innerRegex.Replace(m.Groups["Text"].Value, m2 =>
        m2.Value.Aggregate(
            new StringBuilder(),
            (current, next) => current.Append($"&#{next};")
        ).ToString())
    );

Regex textRegex = ..
Regex innerRegex = ..

I'm not sure about these two variables if they are not recompiled each time the method is called. It might be better to put them outside the method and make them static.

One final thought. You could split this method into three so that you can create finer tests and reuse any of them.

This method assembles both helpers to encode html.

public string EncodeToHtml(string contents)
{
    return ReplaceHtmlText(contents, m => EncodeText(m.Groups["Text"].Value));
}

This method just allows you to replace the text:

public string ReplaceHtmlText(string text, MatchEvaluator m)
{
    return textRegex.Replace(text, m);
}

This method allows you to encode the text:

public string EncodeText(string text)
{
    return encodeRegex.Replace(text, m =>
        m.Value.Aggregate(
            new StringBuilder(),
            (current, next) => current.Append($"&#{next};")
        ).ToString());
}


Transforming a Python dictionary into CSS Recently I had the idea to write a module for the transformation of the dictionary into valid CSS code. And I would like to hear your comments on my code.
License MIT, BSD etc.

The code is very simple:

#pss.py

class PSS:
    def __init__(self, obj):
        self.obj = obj
        self.__data = {}
        self.__parse(obj)

    def __repr__(self):
        return self.__build(self.obj)

    def __build(self, obj, string = ''):
        for key, value in sorted(self.__data.items()):
            if self.__data[key]:
                string += key[1:] + ' {\n' + ''.join(value) + '}\n\n'

        return string

    def __parse(self, obj, selector = ''):
        for key, value in obj.items():
            if hasattr(value, 'items'):
                rule = selector + ' ' + key
                self.__data[rule] = []
                self.__parse(value, rule)

            else:
                prop = self.__data[selector]
                prop.append('\t%s: %s;\n' % (key, value))

import module:

#test.py

from pss import *

css = PSS({
    'html': {
        'body': {
            'color': 'red',
            'div': {
                'color': 'green',
                'border': '1px'
            }
        }
    }
})

print(css)

Result:

html body {
        color: red;
}

html body div {
        color: green;
        border: 1px;
}

So I need your advice to improve the quality of the code.
Perhaps there are still places that can be done easier?

#pss.py

I'd recommend an actual docstring rather then a comment

__version__ = '1.0'

class PSS:
    def __init__(self, obj):
        if type(obj) is not dict:
            raise RuntimeError('There\'s no any objects to parse!')

What if its a subclass of dict? What if its a dict-like object? In python we generally do not check types. So I'd just remove this bit. I'd also name it something more meaningful then obj

        self.obj = obj
        self.__data = {}

Use of __ is controversial. I'm in the camp that you should only use one undescore.

    def __repr__(self):
        return self.__build(self.obj)

This isn't what __repr__ is for. __repr__ should give a python representation of your object, not produce the string version. Look at repr() vs str() on python strings.

    def __build(self, obj, string = ''):
        self.__parse(obj)

Why parse here instead of in the constructor?

        for i in sorted(self.__data.keys()):

i is a terrible name, it usually means index but you aren't using it that way here

            if self.__data[i]:

Use for key, value in sorted(self.__data.items()): then you won't need to access self.__data[i]

                string += i[1:] + ' {\n' + ''.join(self.__data[i]) + '}\n\n'

Adding strings can be expensive. Its generally better to build the strings in a list and then join them or use StringIO, and write the string pieces by piece.

        return string

    def __parse(self, obj, selector = ''):
        for i in obj:

use for key, value in obj.items() whenever you need both the keys and values in a dict. Python isn't stupid like some other languages

            if type(obj[i]) is dict:

Checking types is considered bad form in python. If you must differentiate between types it is perferred to check some aspect of the dict interface, such as the items method.

                rule = selector + ' ' + i

I'd store these as tuples rather then strings.

                self.__data[rule] = []
                self.__parse(obj[i], rule)

            else:
                prop = self.__data[selector]
                prop.append('\t%s: %s\n' % (i, obj[i]))

Again, I'd store this as something besides a string. Probably keys in a dictionary.

        return self

Why return self

Parsing usually refers to converting text into some internal representation. But you are actually doing the opposite. You are converting python objects into partial python objects, partial text. That's simply not parsing. Its also not clear that you gain anything by doing this pre-processing before converting entirely over to text.

from pss import *

css = PSS({
    'html': {
        'body': {
            'color': 'red',
            'div': {
                'color': 'green',
                'border': '1px'
            }
        }
    }
})

I must admit that looks pretty sweet.

Interface-wise, why is this a class? It seems to me that is should be a function. Its a one way transformation from dicts to css string representation. That seems to me to be a fit for a function not a class.

Here is my tackle at the problem:

from StringIO import StringIO

def write_css(selector, data, output):
    children = []
    attributes = []

    for key, value in data.items():
        if hasattr(value, 'items'):
            children.append( (key, value) )
        else:
            attributes.append( (key, value) )

    if attributes:
        print >> output, ' '.join(selector), "{"
        for key, value in attributes:
            print >> output, "\t", key + ":", value
        print >> output, "}"

    for key, value in children:
        write_css(selector + (key,), value, output)

def PSS(data):
    output = StringIO()
    write_css((), data, output)
    return output.getvalue()


ASP.NET MVC using Repository pattern - code review I am just trying to use ASP.NET MVC using repository pattern. Could somebody see If I am doing something wrong here.

Model - Contains the model

public class Contact
    {
        public int Id { get; set; }
        public string FirstName { get; set; }
        public string LastName { get; set; }
        public string EmailAddress { get; set; }
    }

Repository interface - Interface

    public interface IContactRepository
        {
            void Create(Contact contact);
            void Delete(int contactId);
            void Save(Contact contact);
            Contact Retrieve(int contactId);
            IEnumerable<Contact> Select();
        }

Repository class

public class ContactRepository : IContactRepository
    {
        private AddressBookDb addressBookdb = new AddressBookDb();

        public void Create(Contact contact)
        {
            addressBookdb.Contacts.Add(contact);
            addressBookdb.SaveChanges();
        }

        public void Save(Contact contact)
        {
            Delete(contact.Id);
            Create(contact);
        }

        public void Delete(int contactId)
        {
            Contact contact = Retrieve(contactId);
            if (contact != null)
            {
                addressBookdb.Contacts.Remove(contact);
                addressBookdb.SaveChanges();
            }
        }

        public Contact Retrieve(int contactId)
        {
            return addressBookdb.Contacts.FirstOrDefault<Contact>(c => c.Id == contactId);
        }

        public IEnumerable<Models.Contact> Select()
        {
            return addressBookdb.Contacts.ToList<Contact>();
        }
    }

Controller class

public class ContactController : Controller
    {
        IContactRepository contactRepository = null;
        public ContactController() : this (new ContactRepository())
        {

        }

        public ContactController(IContactRepository contactRepository)
        {
            this.contactRepository = contactRepository;
        }

        [HttpPost]
        public ActionResult Edit(Contact contact)
        {
            if (ModelState.IsValid)
            {
                contactRepository.Save(contact);
                return RedirectToAction("Index");
            }
            return View(contact);
        }
}

You code shows not only your definition of your repository but also it's usage.

What worries me is that i don't see how the DbContext (AddressBookDB in this case) is disposed off.

I think i'd rather pass in the AddressBookDB object as a paramter to the repository constructor for usage so you can ensure it is diposed off properly by creating it in a using block.


MinHeap implementation in Java I have made my own MinHeap in Java. I would like to get review comments on the same.

package heap.minheap;

public class MinHeap {

private int[] heap;
private int size;
private int maxSize;
private static final int FRONT = 1;

public MinHeap(int maxSize){
    this.heap = new int[maxSize+1];
    heap[0] = Integer.MIN_VALUE;
    this.size = 0;
}

private int getParent(int position){
    return position/2;
}

private int getLeftChild(int position){
    return 2*position;
}

private int getRightChild(int position){
    return 2*position+1;
}

private void swap(int position1, int position2){
    int temp = heap[position1];
    heap[position1] = heap[position2];
    heap[position2] = temp;
}

private boolean isLeaf(int position){

    if(position > size/2){
        return true;
    }
    return false;
}

public void insert(int data){
    heap[++size] = data;
    int currentItem = size;
    while( heap[getParent(currentItem)] > heap[currentItem] ){
        swap(getParent(currentItem),currentItem);
        currentItem = getParent(currentItem);
    }
}

public int delete(){
    int itemPopped = heap[FRONT];
    heap[FRONT] = heap[size--];
    heapify(FRONT);
    return itemPopped;
}

private void heapify(int position){
    if(isLeaf(position)){
        return;
    }

    if ( heap[position] > heap[getLeftChild(position)] || heap[position] > heap[getRightChild(position)]){

        if(heap[getLeftChild(position)] < heap[getRightChild(position)]){
            swap(position , getLeftChild(position));
            heapify(getLeftChild(position));
        }
        else{
            swap(position , getRightChild(position));
            heapify(getRightChild(position));
        }
    }
}

@Override
public String toString(){
    StringBuilder output = new StringBuilder();
    for(int i=1; i<= size/2; i++){
        output.append("Parent :"+ heap[i]);
        output.append("LeftChild : "+heap[getLeftChild(i)] +" RightChild :"+ heap[getRightChild(i)]).append("\n");
    }
    return output.toString();
}
}

Style

Dead code should be removed. maxSize is never used.
Be consistent in your style. You should decide wether you use this. If a decision is made you should stick to it.

Here i would suggest refactor the constructor to

public MinHeap(int maxSize){
    heap = new int[maxSize+1];
    heap[0] = Integer.MIN_VALUE;
    size = 0;
}

Simplification

In the isLeaf() method you basically say if the condition is true, return true otherwise return false

private boolean isLeaf(int position){

    if(position > size/2){
        return true;
    }
    return false;
}

This can be simplified to return if the condition is true.

private boolean isLeaf(int position){
    return (position > size/2);
}


Delete engine for deeply nested linked structure The background is the question at https://stackoverflow.com/questions/36634394/nested-shared-ptr-destruction-causes-stack-overflow. To summarize, for a data structure that looks like:

// A node in a singly linked list.
struct Node {
    int head;
    std::shared_ptr<Node> tail;
};

If the list grows too long, a stack overflow might occur in the destructor of Node due to recursive destructions of the shared_ptrs. I'm implementing a thread-safe version of the delete engine proposed in https://stackoverflow.com/a/36635668/3234803.

There are three simple classes involved: SpinLock, ConcurrentQueue, and DeleteEngine.

The SpinLock class implements the C++ Lockable interface with a std::atomic_flag:

class SpinLock {
public:
    void lock()
    { while (lock_.test_and_set(std::memory_order_acquire) {} }

    bool try_lock() // Not camel-case to be compatible with C++ Lockable interface
    { return !lock_.test_and_set(std::memory_order_acquire); }

    void unlock()
    { lock_.clear(std::memory_order_release); }

private:
    std::atomic_flag lock_ = ATOMIC_FLAG_INIT;
};

The ConcurrentQueue class implements a multiple-producer-single-consumer queue that supports thread-safe enqueue and tryDequeue:

template<typename T>
class ConcurrentQueue {
public:
    void enqueue(T item)
    {
        std::lock_guard<SpinLock> lk(lock_);
        queue_.emplace_back(std::move(item));
    }

    bool tryDequeue(T &item)
    {
        std::lock_guard<SpinLock> lk(lock_);
        if (queue_.empty()) {
            return false;
        }
        item = std::move(queue_.front());
        queue_.pop_front();
        return true;
    }

private:
    std::deque<T> queue_;
    SpinLock lock_;
};

The DeleteEngine class implements the delete engine proposed in the answer aforementioned:

template<typename T>
class DeleteEngine {
public:
    ~DeleteEngine()
    {
        std::lock_guard<SpinLock> lk(deleting_);
        deleteAll();
    }

    void enqueue(T *p)
    {
        queue_.enqueue(p);
        if (deleting_.try_lock()) {
            std::lock_guard<SpinLock> lk(deleting_, std::adopt_lock);
            deleteAll();
        }
    }

private:
    void deleteAll()
    {
        T *p = nullptr;
        while (queue_.tryDequeue(p)) {
            delete p;
        }
    }

    ConcurrentQueue<T *> queue_;
    SpinLock deleting_;
};

Now in the deleter of a shared_ptr<Node>, we will hand the raw pointer to a DeleteEngine<Node> instead of directly deleteing it. A recursive delete can handle up to about 10000 nodes, while this method can handle an arbitrarily large number of nodes.

The above code is only a prototype, but I'm particularly concerned about the performance of this implementation: (1) Most of the time the Node class would be used in a single-threaded environment; (2) Occasionally it might be used in highly concurrent applications, e.g. a web server that constantly creates and destroys objects.

To address the locking overhead in the single-threaded case, my first advice is (as with all performance questions) to measure before optimising!  It's likely that the locking overhead is tiny compared to the queue's memory management.
If you really find you need to streamline the locking, then you'll probably want to have two versions of the engine.  The simplest way to do that is probably to add a boolean parameter to the template, and then use if constexpr at the points where the code differs.  Or perhaps make the lock type a template parameter, and supply a no-op lock when creating the single-thread version.


SQL to find table containing all specified columns I have the below code to list all tables which have columns DataAreaId and CountryRegionId.
This works, but requires me to change the code in two places if I amend the column list (i.e. both the name in ('DataAreaId','CountryRegionId') code to list the required column names, and also the having COUNT(1) = 2 to match the number of specified columns.

select *
from sys.tables t
where object_id in
(
    select object_id
    from sys.columns c
    where name in ('DataAreaId','CountryRegionId')
    group by object_id
    having COUNT(1) = 2
)
order by Name

I can tweak it to make things more dynamic (i.e. so I only have to define the list of columns; and not have to remember to amend count(1) = 2 to match the number of values as so:

declare @cols table(name sysname)
insert @cols values('DataAreaId'),('CountryRegionId')
select *
from sys.tables t
where object_id in
(
    select object_id
    from sys.columns c
    where name in (select name from @cols)
    group by object_id
    having COUNT(1) = (select COUNT(distinct name) from @cols)
)
order by Name

But that has a bad smell to it / doesn't look elegant.

Any thoughts on how this could be improved, or is this just one of those scenarios where elegance isn't possible?
I'm thinking the seldom used ALL keyword may help somehow; though not sure how.

In this case, I would consider using a CTE to contain the columns you are interested in. This saves having to declare the variables, and also saves the inserts, etc. The basic concept is the same as your second query though...

with FindColumns as (

    select 'DataAreaId' as Seek
  UNION
    select 'CountryRegionId' as Seek

), MyTables as (

    select object_id as Tab,
           count(*) as ColCount
    from sys.columns inner join FindColumns on name = Seek
    group by object_id

)

select *
from sys.tables
inner join MyTables on object_id = Tab
where ColCount = (select count(*) from FindColumns)

Note how this is really just a re-expression of your second query using Common Table Expressions

I put that query together on the Stack Exchange data explorer as an example using a couple of different column names.... you can see it working there.


Brute force MD5 password checker I'm trying to create a C# brute force MD5 password checker. If I run it with "test1" without MD5, it completes in 15 seconds. I would like to make it faster, but I'm a c# beginner, and don't know how to optimize code anymore.

Basically, instead of nested for loops, if using flags (of type boolean), so if you have 4 char long pass it goes like this:

_ _ _ _ -> try every letter and number at last place, then set last flag on true. If last flag == true then increment counter for third place.

private string bruteForce(int lenght)
    {
        MD5 md55 = System.Security.Cryptography.MD5.Create();
        char[] words =
        {
            'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'r', 'q', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z',
            'A','B','C','D','E','F','G','H','I','J','K','L','M','N','O','P','R','Q','S','T','U','V','W','X','Y','Z',
            '0','1','2','3','4','5','6','7','8','9'

        };

        bool[] konec = new bool[lenght];

        int flag = lenght;
        char[] pass = new char[flag];
        int[] signal = new int[lenght];
        for (int j = 0; j < lenght; j++)
        {
            signal[j] = -1;
            konec[j] = false;
        }
        flag--;
        string tmp = "";
        bool over=false;

        while (!over)
        {

            for (int k = 0; k < lenght; k++)
            {
                if (konec[k])
                {
                    if (k == 0) return "not found";
                    int t = ++signal[k - 1];
                    if (t == words.Length - 1) {konec[k - 1] = true;
                        signal[k - 1] = 0;

                        break;
                    }
                    pass[k - 1] = words[t];
                    konec[k] = false;

                }
            }
            for (int i = 0; i < words.Length; i++)
            {

                if (!konec[flag])
                {
                    pass[flag] = words[i];
                    tmp = new string(pass);
                    byte[] inputBytes = System.Text.Encoding.ASCII.GetBytes(tmp);
                    byte[] hash = md55.ComputeHash(inputBytes);

                    // step 2, convert byte array to hex string
                  string  emde= BitConverter.ToString(hash).Replace("-", "");
                  emde = emde.ToLower();
                  if (tmp == target)
                    {
                        MessageBox.Show(tmp + "    " + target);
                        over = true;
                        break;
                    }
                }

            }
          konec[flag] = true;
        }
        return tmp;
    }

This is call on button click:

private void Button_Click(object sender, RoutedEventArgs e)
    {
        int l = Convert.ToInt32(stevilo.Text);
       // lenght = 1;
       // string a = "";

        if (md5)
        {

            if (brute)
            {
                Stopwatch time = new Stopwatch();
                time.Start();

                target = zgoscevalna.Text;
                for (int i = 1; i < l+1; i++)
                {
                    int _i = i;
                     Thread t = new Thread(() =>
                    {
                        string a = bruteForce(_i);
                        if (a != "not found")
                        {
                            Dispatcher.Invoke(() =>
                            {
                                password.Text = a;
                                tajm.Text = (time.Elapsed.TotalSeconds).ToString();
                            });

                            Thread.CurrentThread.Abort();

                        }
                        else
                        {
                            Thread.CurrentThread.Abort();
                        }

                    });
                    t.Start();

                }

            }

            else
            {
            }
        }

MD5 md55 = System.Security.Cryptography.MD5.Create();

Do yourself a favor, an stick using System.Security.Cryptography; at the top of the code file - then you can shorten this line to MD5 md55 = MD5.Create(); - but md55 is a bad name that doesn't mean anything. Perhaps engine, or hashProvider... anything but md55!

char[] words

That is a confusing name - characters aren't words, a better name for this array could be tokens, perhaps.

if (k == 0) return "not found";

What if the password happened to be "not found"? I think this is a situation where you would want to throw an exception of some sort, instead of tweaking the meaning of the return value.

Thread t = new Thread(() => ...

Starting your own thread is rarely a good idea. Look into the Task Parallel Library (TPL) in the System.Threading.Tasks namespace, and let the framework handle all the threading dirtiness for you. Also, starting a new thread incurs some overhead - if I'm not sticking a foot in my mouth here, using the TPL the framework determines if it's a good idea to start a new thread or to let code run sequentially... which may buy you some extra performance right here.

I would extract a method here, the anonymous method isn't buying you anything and contributes to make your click handler look like it's some "God button" that knows everything and does everything.

Thread.CurrentThread.Abort();

That's part of the threading nastiness you get to avoid with tasks. Aborting a thread is rarely a sane thing to do. I don't know much about multithreading, but this definitely rings a "no-no" bell.

        else
        {
        }

Dead code. Don't think, remove.

tajm.Text = (time.Elapsed.TotalSeconds).ToString();

You really need to work on naming. It's not going to make your code run any faster, but tajm doesn't mean anything to anyone maintaining your code. Be descriptive!


Parsing oscilloscope binary data I wrote a simply parser for Tektronix's internal binary file format .isf. The code is supposed to read in the file and return the header data as well as the actual oszi-data.

Since I mostly code by myself, I would like your opinion on whether my code follows common practice and whether it can be improved.

import numpy as np
import os.path

def read_curve(binaryfile):
    """
    Reads one tektronix .isf file and returns a dictionary containing
    all tags as keys. The actual data is stored in the key "data".
    """
    postfixs = [".isf", ".ISF"]
    if os.path.splitext(binaryfile)[-1] not in postfixs:
        raise ValueError("File type unkown.")

    with open(binaryfile, 'rb') as bfile:

        # read header
        header = {}
        current = 0

        while True:

            current, name = _read_chunk(bfile, " ")

            if name != ":CURVE":
                current, value = _read_chunk(bfile, ";")

                assert name not in header
                header[name] = value
            else:
                # ":CURVE" is the last tag of the header, followed by
                # a '#' and a 7 digit number.
                header[name] = bfile.read(8)
                current = bfile.tell()
                break

        assert header["ENCDG"] == "BINARY"

        # read data as numpy array
        header["data"] = _read_data(bfile, current, header)

    return header

def _read_chunk(headerfile, delimiter):
    """
    Reads one chunk of header data. Based on delimiter, this may be a tag
    (ended by " ") or the value of a tag (ended by ";").
    """
    chunk = []
    while True:
        c = headerfile.read(1)
        if c != delimiter:
            chunk.append(c)
        else:
            return headerfile.tell(), "".join(chunk)

def _read_data(bfile, position, header):
    """
    Reads in the binary data as numpy array.
    Apparently, there are only 1d-signals stored in .isf files, so a 1d-array
    is read.
    Returns a 2d-array with timepoints and datapoints aligned.
    """
    # determine the datatype from header tags
    datatype = ">" if header["BYT_OR"] == "MSB" else "<"
    if header["BN_FMT"] == "RI":
        datatype += "i"
    else:
        datatype += "u"
    datatype += header[":WFMPRE:BYT_NR"]

    bfile.seek(position)
    data = np.fromfile(bfile, datatype)
    assert data.size == int(header["NR_PT"])

    # calculate true values
    data = data * float(header["YMULT"]) + float(header["YZERO"])

    # create timepoints
    t = np.arange(data.size)
    t = t * float(header["XINCR"]) + float(header["XZERO"])

    # create single array
    res = np.concatenate((t[None, :], data[None, :]), axis=0)

    return res

Edit: I posted my revised code here: Parsing oscilloscope data, follow up

This seems pretty nice. I have just some nitpicks about this part:

postfixs = [".isf", ".ISF"]
if os.path.splitext(binaryfile)[-1] not in postfixs:
    raise ValueError("File type unkown.")

Nitpicks:

I generally don't like repeating the same thing in lowercase and uppercase versions. I prefer to use lowercase, and convert untrusted input to lowercase. However, that will make this check less strict: iSF and ISf also become valid extensions, which you might not want.
I'd spell out "postfixs" as "postfixes", or better yet, use the more common term "extensions"
Instead of a list of allowed extensions, I'd use a set. Although in this example you surely won't be hitting performance issues, it's a matter of principle and habit

So I'd write this as:

extensions = set([".isf"])
if os.path.splitext(binaryfile)[-1].lower() not in extensions:
    raise ValueError("File type unknown.")

As for the rest, this seems pretty clean to me. I especially like the assert statements documenting your assumptions.


Caching that resets itself under certain circumstances: Reduce boiler plate code In my machine learning programs I have the typical situation where I need to perform certain operations on data, which results in the following graph:

data --op A--> intermediate_state --op B--> final result type 1
data --op A--> intermediate_state --op C--> final result type 2
data --op A--> intermediate_state --op D--> final result type 3

So in order to not have to repeat operation A all the time I provide a "fit" method, which implements operation A.
Then, I implement transformation methods that implement the other operations.
However, since I usually only need some of the final results in the graph leaves, I want to calculate those on the one hand lazily, but then cache the result.
Since the operations require a parameter, I provide this one in the constructor.
I also need to pass a parametrized object with the same parametrization to different parts of the the program, where the data of operation A changes, without the parametrization changing.
That is the reason why fit is it's own method and not part of the constructor.
Also, because of that I cannot simply cache the results of the operations that are not A, without resetting them after invoking a new fit.
Here is my result in Python:
class MyClass:

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

    # expensive machine learning fitting
    def fit(self, data) -> MyClass:
        self.intermediate_data_ = data + self.parameter + 2
        self._complex_transform1 = None
        self._complex_transform2 = None
        return self

    # expensive machine learning operation version 1
    @property
    def complex_transform1(self):
        if self._complex_transform1 is None:
            self._complex_transform1 = self.intermediate_data_ / self.parameter / 2
        return self._complex_transform1

    # expensive machine learning operation version 2
    @property
    def complex_transform2(self):
        if self._complex_transform2 is None:
            self._complex_transform2 = self.intermediate_data_ / self.parameter / 5
        return self._complex_transform2

The problem I have with this approach is that I am repeating quite a bit of boiler plate code.
(Note, that I have many more operations that are not A.)
Namely:

The variables self._complex_transform1 and self._complex_transform2

The constructs if self._complex_transform1 is None: could be reduced from
     if self._complex_transform1 is None:
         self._complex_transform1 = self.intermediate_data_ / self.parameter / 2
     return self._complex_transform1

to
        return self.intermediate_data_ / self.parameter / 2

and I would save a nesting.
I was thinking of defining a container class that contains the results and is reset by fit. But I do not see how to implement this without basically introducing the same about of boilerplate code into the transform methods.
Some decorator-approach might be great, but I am not sure how to approach this.

This might be a job for @functools.cached_property. It works a lot like @property, except it remembers the value between calls. The value can then be cleared using del in the fit function, or elsewhere if needed.
Unfortunately, the properties can't be deled until they've been set, which means we have to re-add the boiledplate in fit in the form of try: del x; except AttributeError: pass. Maybe that's an improvement since it's at least centralized, but it's still not ideal.
There are workarounds like
def fit(self, data):
    self.intermediate_data_ = ...

    for prop in ["complex_transform1", "complex_transform2"]:
        try:
            delattr(self, prop)
        except AttributeError:
            pass

    return self

Or perhaps something like
def fit(self, data):
    self.intermediate_data_ = ...

    for prop in self.reset_on_fit:
        delattr(self, prop)

    self.reset_on_fit = []
    return self

with the properties looking like
@cached_property
def complex_transform1(self):
    self.reset_on_fit.append("complex_transform1")
    return self.intermediate_data_ / self.parameter / 2

But putting that data into a hardcoded string is not very refactor-friendly. Perhaps we can define a decorator to figure that out for us?
Well, I haven't tested it very thoroughly, but I think we can:
from functools import cached_property, wraps

def resets_on_fit(fn):
    @wraps(fn)
    def wrapper(self):
        self.reset_on_fit.append(fn.__name__)
        return fn(self)

    return wrapper

class MyClass:
    # ...

    def fit(self, data):
        self.intermediate_data_ = data + self.parameter + 2

        for prop in self.reset_on_fit:
            delattr(self, prop)

        self.reset_on_fit = []
        return self

    @cached_property
    @resets_on_fit
    def complex_transform1(self):
        return self.intermediate_data_ / self.parameter / 2


Deck of cards program using a card value map Previous review of this project: Nested Deck class design and implementation

My program finally meets all of my expectations to the best of my ability, but I'd still like another review.  Please give as many critiques as you can.  I will then implement this with future games that I have in mind.

CardValuesMap.h

#ifndef CARDVALUESMAP_H
#define CARDVALUESMAP_H

std::map<char, unsigned> retrieveMap()
{
    std::map<char, unsigned> values;

    values['A'] = 1;
    values['2'] = 2;
    values['3'] = 3;
    values['4'] = 4;
    values['5'] = 5;
    values['6'] = 6;
    values['7'] = 7;
    values['8'] = 8;
    values['9'] = 9;
    values['T'] = 10;
    values['J'] = 11;
    values['Q'] = 12;
    values['K'] = 13;

    return values;
}

#endif

Deck.h

#ifndef DECK_H
#define DECK_H

#include <iostream>
#include <array>
#include <map>
#include <string>

class Deck;

class Card
{
private:
    friend class Deck;
    unsigned value;
    char rank;
    char suit;
    Card::Card(unsigned v, char r, char s) : value(v), rank(r), suit(s) {}

public:
    Card::Card() : value(0), rank(' '), suit(' ') {}
    bool operator<(const Card &rhs) const {return (value < rhs.value);}
    bool operator>(const Card &rhs) const {return (value > rhs.value);}
    bool operator==(const Card &rhs) const {return (suit == rhs.suit);}
    bool operator!=(const Card &rhs) const {return (suit != rhs.suit);}
    friend std::ostream& operator<<(std::ostream &out, const Card &aCard)
        {return out << '[' << aCard.rank << aCard.suit << ']';}
};

class Deck
{
private:
    static const unsigned MAX_SIZE = 52;
    static const std::map<char, unsigned> values;
    static const std::string SUITS;
    std::array<Card, MAX_SIZE> cards;
    int topCardPos;

public:
    Deck();
    void shuffle();
    Card deal();
    unsigned size() const {return topCardPos+1;}
    unsigned empty() const {return topCardPos == -1;}
    friend std::ostream& operator<<(std::ostream &out, const Deck &aDeck);
};

#endif

Deck.cpp

#include "Deck.h"
#include "CardValuesMap.h"
#include <algorithm>

const std::map<char, unsigned> Deck::values = retrieveMap();
const std::string Deck::SUITS = "HDCS";

Deck::Deck() : topCardPos(-1)
{
    for (auto valueIter = values.cbegin(); valueIter != values.cend(); ++valueIter)
    {
        for (auto suitIter = SUITS.cbegin(); suitIter != SUITS.cend(); ++suitIter)
        {
            topCardPos++;
            cards[topCardPos] = Card(valueIter->second, valueIter->first, *suitIter);
        }
    }

    shuffle();
}

void Deck::shuffle()
{
    topCardPos = MAX_SIZE-1;
    std::random_shuffle(&cards.front(), &cards.back()+1);
}

Card Deck::deal()
{
    if (empty())
    {
        std::cerr << "\nDECK IS EMPTY -- RESHUFFLING NOW\n";
        shuffle();
    }

    topCardPos--;
    return cards[topCardPos+1];
}

std::ostream& operator<<(std::ostream &out, const Deck &aDeck)
{
    for (int i = aDeck.size()-1; i > -1; --i)
    {
        out << "\n" << aDeck.cards[i];
    }

    return out;
}

I haven't followed your previous reviews, so a few of these may be things that have already been pointed out.

Anyway, here's my go at it.

The Card:: is invalid on the following:

Card::Card(unsigned v, char r, char s) : value(v), rank(r), suit(s) {}
Card::Card() : value(0), rank(' '), suit(' ') {}

The suit should likely be an enum (in particular, a strongly typed one as you're using C++11).

The comparators in Card are non-sensical. Why do you sometimes compare value and sometimes compare suit? How are a Jack of Spades and a 2 of Spades equal just because they're both a Spade?

For maintainability, try to define comparators in terms of each other: bool operator!=(const Card &rhs) const {return !(suit == rhs.suit);} and so on.

Don't use bare unsigned. Use unsigned int.

(Personal preference really on this one)

Even better yet, define the types as public typedefs. Like Deck::size_type, Card:suit_type and so on (standard library-esque).

Why make the maximum size be 52?

You could either have the size be templated and default to 52, or you could have it be a constructor parameter (in which case you'd have to use a vector rather than std::array).

Is the mapping in retrieveMap ever meant to be used by consuming code? If not, make it static in Deck.cpp.

Card::deal has no business outputting anything. If dealing can't be done, throw an exception.

Rather than having to think in reverse iteration, you could think of the top of the deck as being 0. This would likely simplify a lot of the logic.

Are you happy being stuck with a 52 card deck? What if you don't want Jokers? What if you want some completely different kind of card system? A

Basically what I'm saying is that a deck shouldn't be responsible for filling itself up with cards.

In fact, constructing objects in a construct tends to be an anti-pattern: http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/. Note that "new" in this context is talking about Java, not C++. Though the terminology is Java centric, what it actually means is is construction in general. Constructing objects in a construct tends to imply that the objects should be provided to the constructor rather than be created in there.

Rather than having the deck populate itself with a traditional set of cards, have a utility method:

Deck classDeck = Deck::makeClassicDeck();

This lets you have the convenience that you currently have, but it doesn't hold you to a certain set of cards (you could also go the two constructors route).

Why are you trying to hide the Card class? You should probably trust your consuming code to handle cards. How do you know that the code isn't going to be used for some very odd game that involves a deck composed of a normal deck and 6 extra jokers? If you let the consumer of Deck decide what to put it in, it's a lot more flexible.

As a person about to make a program that involves card game logic, why should I use your Deck class? What does it offer me over using a simple std::deqeue (or std::stack or std::vector) of Card? The way I see it at the moment, it offers basically nothing over a vector. It just has a more convenient shuffle.

MAX_SIZE should be a std::size_t

empty should return a bool

size should return a std::size_t

I'm not sure if I'd bother defining an ostream<<. Might be more useful to let each application determine how it wants to print a deck rather than tie it to one format.

I'd go with a more idiomatic shuffling approach:

std::random_shuffle(cards.begin(), cards.end());

I don't like SUITS. Why is it the only all caps variable?

I don't like aDeck and aCard. A const Deck& is obviously a Deck. Just call it deck or something. Names beginning with an article seem odd (imagine if Deck::cards were instead theCards).

Since it's private, it's not as important, but Card::Card(unsigned v, char r, char s) should still have proper names for the parameters.

Crazy paranoid moment: deal() is not exception safe (well, technically it is since Card::Card never throws, but there's no hard gaurantee of that in place).

You could theoretically encounter an exception in the card constructor which would mean the card would properly be removed from the deck, but the consuming code would not be able to have it.

Then again, this is complete paranoia, especially since Card's constructor never throws for the time being.

(Just happened to think of it since the oddity of std::stack's pop() returning void has always been an interesting little necessity to me.)


Using Java to Add the Contents of two EditText views in Android I have a toy app that contains:

editText1 for entering a number. Uses inputType of number.
editText2 for entering a second number. Uses inputType of number.
adderButton for triggering addition.
textVeiw  for displaying the result of the addition.

I know:

The naming could be better
I should check for overflow/underflow on the addition and elsewhere.

This code is working for simple cases.

 public void adder_click(View view) {
        EditText et1 = (EditText) findViewById(R.id.editText1);
        EditText et2 = (EditText) findViewById(R.id.editText2);
        Integer s = Integer.valueOf(et1.getText().toString()) + Integer.valueOf(et2.getText().toString());
        TextView tv = (TextView) findViewById(R.id.textView);
        tv.setText(s.toString());
    }

Is there a more direct way of reading the values and performing the addition?

That code is a bit dangerous. Integer.valueOf() can throw NumberFormatException which you make no effort to catch. This will happen if you have commas in numbers or non-digit characters. Your other comments are also very important.

As for simplifying the method, it is not good that you call findViewById 3 times every time you press a button. I would store references to editText1, editText2 and textView externally (of this method), as then you can reuse all three here and elsewhere in your code with subtract/multiply etc. You can then remove three lines of code and just have:

Integer s = Integer.valueOf(et1.getText().toString()) + Integer.valueOf(et2.getText().toString());
tv.setText(s.toString());

Remember that you should probably wrap the code in a try catch block unless you can guarantee the text field will only contain decimal numbers. Also, add in your overflow/underflow logic as well.


Implement an algorithm to find the Kth to last element of a singly linked string I wrote this algorithm to find the Kth element of a singly linked list and I would like your feedback because I want to know if there is a better version.

In my eyes, this is the most optimized way of doing it and I've covered every edge case (I also assumed that the only possible input is a number, if I removed that assumption, I would add another if block), but I really would appreciate your feedback.

PS: I also only added these two main methods (add and findLast) because I figured that there were the only two required to find a solution, but I'm not sure if I should add more in an interview (i.e. remove, removeAt, addAt, isEmpty, etc.)

function LinkedList() {
  let length = 0;
  let head = null;

  function Node(element) {
    this.element = element;
    this.next = null;
  }

  this.length = function() {
    return length;
  }

  this.head = function() {
    return head;
  }

  this.add = function(element) {
    let node = new Node(element);

    if (head == null) {
      head = node;
    } else {
      currentNode = head;

      while (currentNode.next) {
        currentNode = currentNode.next;
      }
      currentNode.next = node;
    }
    length++;
  }

  this.findLast = function(index) {
    if(index > length) {
      return false;
    }
    if (index < 0) {
      return false;
    }

    let realIndexes = length-1; // if length = 5, realIndexes = 4
    let realPosition = index-1; // if index = 2, realPosition = 1
    // We are looking for the real position here.
    let realTarget = realIndexes - realPosition;
    let currentNode = head;
    let counter = 0;

    while (counter < realTarget) {
      currentNode = currentNode.next;
      counter++;
    }
    return currentNode.element;
  }
}

let congo = new LinkedList();
console.log(congo)
congo.add(5);
congo.add("Homeless");
congo.add("Real eyes realize real lies");
congo.length()
congo.head()

Output: => 5

To get the sub-list of the last n elements without computing the length first (see comments):

Go ahead with an end pointer until you have skipped n elements.
Now go ahead in parallel with a start pointer and the end pointer until the end pointer reaches the end of the list (the start and end pointers will stay n elements apart as you go).
Now start points to the desired sub-list.

In an interview, before you implement anything, it's often a good idea to first describe your plan. Counting first works and is no different in terms of the O notation, but I doubt it's what the interviewer wanted to see. Describing your plan can avoid spending time on something the interviewer might not really be interested in.