ROT13 algorithm in C I am trying to learn C and I came across the ROT13 scrambling system used to store some passwords.

Assuming the user types everything in correctly (uses 1 argument, uses a string not an int, etc.) would this be correct/safe to use? Or is there anything at all that I am doing wrong you can point out to me (techniques, indentation, anything at all)?

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

int main(int argc, char** argv)
{
    char* word = argv[1];
    int key = 13;

    // all the letters in the first argument
    for (int n = 0, len = strlen(word); n < len; n++)
    {
        int currentLetter = word[n];

        char cipher = currentLetter + key;

        // make sure the next letter isn't over 26 or it isn't a ascii letter
        // if it is, do %26
        if ((currentLetter - 'a') + key > 26)
        {
            key = (currentLetter - 'a') + key) % 26;
            cipher = 'a' + key;
        }

        printf("%c", cipher);
        // reset the key and do the next letter
        key = 13;
        }
    }
    printf("\n");
    return 0;
}

Test your program with a variety of inputs.
Include "abcdefghijklmnopqrstuvwxyz".

Have a look what happens for 'n'.
Always try to make sure you test low values, high values and edge cases.
'Wrapping round' is a classic edge case.

Then look at this line:

if ((currentLetter - 'a') + key > 26)

This program only works for lower case 'passwords'.

You might look at the functions in #include <ctype.h> such as tolower(.) or islower(.) to extend to mixing upper and lower case.

NB Obviously ROT13 has no real cryptographic value of any kind.
I mention that because I have seen it (and things not better) used in applications.
You're doing this as a learning exercise. Right?


Review my ruby for performance So I'm using the acts_as_taggable_on gem to tag 3 different models: Question, Blog, and Video

Each one has a site_id to distinguish which site they show up on (SaaS-ish).

I wanted to get all tags for a specific site without saving site_id right on a Tag or Tagging.

I came up with the following, but I feel like there might be something more efficient I could do.

ActsAsTaggableOn::Tag.class_eval do
  extend FriendlyId
  friendly_id :name, use: :slugged

  def self.find_by_site site, limit=0
    klasses = [Blog, Question, Video]
    sql = []
    site_ids = []
    klasses.each do |k|
      sql << "
        SELECT tags.*, (SELECT COUNT(*) FROM taggings t WHERE t.tag_id = tags.id) AS `count`
              FROM tags
              LEFT JOIN taggings
              ON (taggings.tag_id = tags.id)
              LEFT JOIN #{k.table_name}
              ON (#{k.table_name}.id = taggings.taggable_id)
              WHERE taggings.taggable_type = '#{k.name}'
              AND #{k.table_name}.site_id = ?
      "
    end
    sql = sql.join(" UNION ")
    sql += " GROUP BY id "
    sql += " ORDER BY COUNT DESC "
    sql += "LIMIT #{limit}" if limit > 0
    klasses.count.times { site_ids << site.id }
    find_by_sql([sql, *site_ids])
  end
end

Here is the current functionality with timing:

irb(main):011:0> ActsAsTaggableOn::Tag.find_by_site Site.find(3); nil
  Site Load (0.4ms)  SELECT `sites`.* FROM `sites` WHERE `sites`.`id` = 3 LIMIT 1
  ActsAsTaggableOn::Tag Load (0.5ms)
 SELECT tags.*, (SELECT COUNT(*) FROM taggings t WHERE t.tag_id = tags.id) AS `count`
 FROM tags
 LEFT JOIN taggings
 ON (taggings.tag_id = tags.id)
 LEFT JOIN blogs
 ON (blogs.id = taggings.taggable_id)
 WHERE taggings.taggable_type = 'Blog'
 AND blogs.site_id = 3
 UNION
 SELECT tags.*, (SELECT COUNT(*) FROM taggings t WHERE t.tag_id = tags.id) AS `count`
 FROM tags
 LEFT JOIN taggings
 ON (taggings.tag_id = tags.id)
 LEFT JOIN questions
 ON (questions.id = taggings.taggable_id)
 WHERE taggings.taggable_type = 'Question'
 AND questions.site_id = 3
 UNION
 SELECT tags.*, (SELECT COUNT(*) FROM taggings t WHERE t.tag_id = tags.id) AS `count`
 FROM tags
 LEFT JOIN taggings
 ON (taggings.tag_id = tags.id)
 LEFT JOIN videos
 ON (videos.id = taggings.taggable_id)
 WHERE taggings.taggable_type = 'Video'
 AND videos.site_id = 3
 GROUP BY id ORDER BY COUNT DESC

Assuming these are all AR models with appropriate associations, I'd create a method on Site that iterates over a site instance's Blogs, Questions and Videos and collects their tags.  Something like this maybe:

def get_site_tags
  tags = []
  self.blogs.each do |blog|
    (tags <<  blog.tags)
  end
  self.questions.each do |question|
    (tags <<  question.tags)
  end
  self.videos.each do |video|
    (tags <<  video.tags)
  end
  tags.flatten!
end

Would this work for you or am I missing something?


Python function to strip inline repr'd unicode strings I have to write a python function that takes a string that contains raw unicode strings (e.g. "u'hello' there") and transform it into a string that strips the u'' identifiers from it. For example:

strip_inline_unicode("u'hello' there")  # -> 'hello there'

Without getting into too much detail as to why I have to do this, sufficed to say I cannot simply replace the logic that generates these strings, and I have to strip these because it is being returned as human-readable output to a user.

Constraints:

Strings can have "malformed" unicode identifiers which should be handled correctly (e.g. "hello u'there" -> "hello there")
Strings can have empty unicode identifiers (e.g. "hello u''there" -> "hello there")
Strings will never have "nested" unicode identifiers (e.g. 'u"u\'foo\'"')
Quotes will always be single-quotes (so never u"<stuff>")

Here is what I came up with:

def strip_inline_unicode(stupid_string):
    """Takes a string that looks like "u'hello' there" and returns
    "hello there" """

    in_unicode = False
    pos = 0

    new_str = ''

    while (pos < len(stupid_string)):
        if pos + 1 >= len(stupid_string):
            if in_unicode and stupid_string[-1] == "'":
                new_str += stupid_string[pos:-1]
            else:
                new_str += stupid_string[pos:]
            break

        cur = stupid_string[pos]
        nxt = stupid_string[pos + 1]
        if cur == 'u' and nxt == "'" and not in_unicode:
            in_unicode = True
            pos += 1
        elif in_unicode and cur == "'":
            in_unicode = False
        else:
            new_str += cur

        pos += 1

    return new_str

When I plug it into the interpreter it seems to work correctly:

In [12]: strip_inline_unicode("u'hello' there")
Out[12]: 'hello there'

In [14]: strip_inline_unicode("hello there")
Out[14]: 'hello there'

In [15]: strip_inline_unicode("hello u'there")
Out[15]: 'hello there'

In [16]: strip_inline_unicode("hello u''there")
Out[16]: 'hello there'

In [17]: strip_inline_unicode("au'b'")
Out[17]: 'ab'

In [18]: strip_inline_unicode("u'abc'")
Out[18]: 'abc'

However, I am by no means a Python expert, and it seems like I could accomplish something similar in a simpler and more robust manner, perhaps by using regexes. I was hoping to get some feedback on the implementation and maybe simplify it/make it better.

Yes, regexps were my first thought:

re.sub(r"u'([^']*)'?", r'\1', string)

Dissected:

A literal u'.
Then anything that is not a ' zero or more times: [^']*.
Store that for later retrieval: ([^']*).
End with an optional '.


Calculate the cost of cement for a project I am a newbie in a programming, and I just randomly chose a task for training from some group on Facebook. The task is to calculate the cost of cement for a construction project. The input is the number of pounds of cement required (guaranteed not to be a multiple of 120). The store sells cement in 120-pound bags, each costing $45.

Example input: 295.8
Output: 135

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

#define PRICE 45
#define CAPACITY 120
#define MAXDIGITS 5

int sum(int);

int main(int argc, char *argv[])
{
    int val = 0;
    char inp[MAXDIGITS];

    if ((argc > 1) && (argv[1] > 0))
        val = strtol(argv[1], NULL, 0);
    else
    {
        do
        {
            printf("Please input the value of cement: ");
            scanf("%s", inp);
            val = strtol(inp, NULL, 0);
        }
        while (!val);
    }

    if (val)
        printf("Money you need: %d\n", sum(val));

    return 0;
}

int sum(int need)
{
    int mon = PRICE;
    int n = CAPACITY;

    while (n < need)
    {
        n += CAPACITY;
        mon += PRICE;
    }

    return mon;
}

I'm interested in code style, rational memory usage, etc.

Buffer overflow
Consider what entering "295.8" does.
#define MAXDIGITS 5
char inp[MAXDIGITS];
printf("Please input the value of cement: ");
scanf("%s", inp);

Code used the dangerous scanf("%s", inp); with no width limits.  scanf("%s", inp); being ignorant of the size of inp[], stored the 5 read characters and the appended null character.
This results in undefined behavior (UB).  Code may work as expected today and fail in strange ways tomorrow.
I recommend to only use fgets() for user input.  After reading a line of user input, it is saved as a string.  Then parse the string.
No need for such a tight buffer size.  Recommend twice the expected max width needed.
#define BUFFER_SIZE (2*MAXDIGITS + 1)
char buffer[BUFFER_SIZE];

printf("Please input the value of cement: ");
fflush(stdout);  // Insure output is seen before reading

fgets(buffer, sizeof buffer, stdin);
double val = strtod(buffer, NULL);


Javascript of property path in object I am trying to find the path of property in an object in JavaScript.  For example, given

var myObj = { 'a': {'b': {'c': {'x': 1, 'y': 2 }}} }

the path of 'x' in myObj would be a.b.c.x (dot notation).

I wrote the code below using a recursive mechanism.

function findPropPath(obj, name, currentPath) {
    var currentPath = currentPath || '';

    for (var i in obj)
    {
        if (i == name)
        {
            return name;
        }
        else if (obj[i] && typeof obj[i] == "object")
        {
            var result = findPropPath(obj[i], name, currentPath + '.' + i);
            if (result == name)
            {
                return currentPath += '.' + name;
            }
            else if (result && result.endsWith('.' + name)) {
                return result;
            }
        }
    }
}

It works as expected, I would like to know if it can be optimized or other better ways find property path. Also I assume the call stack size would grow for  enormous object. Any review would be of great help!

Please pick a consistent brace style.

i has the connotation of being an integer counter.  I would pick a different variable name for iterating over the properties of an object.

I don't see any need for obj[i] && in the else if test.

If you use recursion properly, you shouldn't need to have a third parameter at all.

function findPropPath(obj, name) {
    for (var prop in obj) {
        if (prop == name) {
            return name;
        } else if (typeof obj[prop] == "object") {
            var result = findPropPath(obj[prop], name);
            if (result) {
                return prop + '.' + result;
            }
        }
    }
    return null;    // Not strictly needed, but good style
}

var myObj = {'a': {'b': {'c': {'x': 1, 'y': 2 }}}};
console.log(findPropPath(myObj, 'x'));


Disposing the Context in the Repository pattern I have a question about the Repository pattern:

public abstract class RepositoryBase<T> : IDisposable, IRepository<T> where T : class, IEntity
    {
        /// <summary>
        /// For iterations using LINQ extension methods on the IQueryable and IEnumerable interfaces
        /// it is required that the context which was used to make the query was not disposed before the iteration
        /// otherwise it will throw an exception
        /// </summary>
        private Context _context;

        protected Context Context
        {
            get { return _context ?? (_context = new Context()); }
        }

        public virtual void AddOrUpdate(T entity)
        {
            var dbSet = Context.Set<T>();

            if (dbSet.Contains(entity))
                entity.ChangeDateTime = DateTime.Now;
            else
                entity.AddeDateTime = DateTime.Now;

            Context.Set<T>().AddOrUpdate(entity);
            Context.SaveChanges();
        }

        public virtual void Delete(T entity)
        {
            Context.Set<T>().Remove(entity);
            Context.SaveChanges();
        }

        public void Drop()
        {
            var dbSet = Context.Set<T>();

            foreach (var entity in dbSet)
                dbSet.Remove(entity);

            Context.SaveChanges();
        }

        public virtual IQueryable<T> Find(Expression<Func<T, bool>> predicate)
        {
            return Context.Set<T>().Where(predicate);
        }

        public IEnumerable<T> GetAll()
        {
            return Context.Set<T>();
        }

        public void Dispose()
        {
            if (_context != null)
            {
                _context.Dispose();
                _context = null;
            }
        }
    }

Why I am not embracing each method implementation with a using and disposing the Context as soon as the query is done?

Some of my methods, as Find and GetAll return collections. I would want to be able to iterate through them using LINQ, only this is not possible if the Context is disposed, as it would be in the original Repository model.

Is this the best approach to go around this issue?

Reusing the same context could have some benefits if you need entities to be cached but you could also end up with contexts with a large memory sizes.

Depending on the requirements, I have usually created a new context on every call and in the case of database contexts, played it safe by forcing a ToList() within the function, before returning the entities.

This ensures that the database connection is only open for just enough time to retrieve the rows and the context is destroyed without holding on to the entities retrieved.

The approach to take here, really depends on your requirements.


Modular design with Require.js I am trying to get a better understanding of the modular design pattern and I am using Requirejs to manage my dependencies.

I understand separating code into modules (using define with RequireJS) so they take on only single responsibilities and also splitting them into separate JavaScript files so Require JS can load them.

In my main.js, the entry points looks like this (which depends on work module).  For brevity, I have left out the require.config, paths, etc.:

require(
  ['work'],
  function(work) {
     var url = "urlstring";
     work.buildHTMLTable(url);
  }
);

the method buildHTMLTable will append html table to a div tag.

I want to build a re-usable generic js library that will handle vertical/horizontal scroll event and arrow key navigation event.  I need direction on how to incorporate these events.  The below method is working, but I wanted to make sure I was setting everything up correctly.

Should I have one define method that has a function which takes care of handling the scrolling like so?

define('scrollingGrid',
    ['jquery'],
    function ($) {

       var bindScroll = function (hs, vs) {

       $(hs).on('scroll', function (e) {

       }

       $(vs).on('scroll', function (e) {

       }

        }; return {
        bindScroll: bindScroll
        };

    }

);

and then I can call it from the main as so:

require(
      ['work', 'grid'],
      function(work) {
         var url = "urlstring";
         work.buildHTMLTable(url);

         grid.bindScroll('#hscroll', '#vscroll');

      }
    );

Should I also handle the arrow key navigation in the same scrollingGrid module (see below)?

require(
          ['work', 'grid'],
          function(work) {
             var url = "urlstring";
             work.buildHTMLTable(url);

             grid.bindScroll('#hscroll', '#vscroll');
             grid.bindArrow('#tab');

          }
        );

In summary, I am creating a re-usable JavaScript control that could handle events on an HTML table.  Is what I am doing okay or is there a better way?

First, unless the work module is about force times distance in a physics simulator, it is a terribly over-generic name! What does the name work communicate to someone who is sitting down with your application for the first time? Name modules after what they do and be as specific as possible but not more so.

Second, if you're only exposing a single method in that module just return that method directly, no need for an object.

Next, won't your scrolling handlers depend on a specific structure being present already? You might want to make it explicit. I can't really make a recommendation how without understanding better how you plan to use this (I'm really confused what work.buildHTMLTable(url) could possibly do).

As for where key navigation should go, it really is all about naming. So if you view the arrow key navigation to be a part of the grid scrolling then yes, it should be in the module that sets that up, otherwise it should not be.

Finally, What if you have two grids on the same page? seems like you're trying to apply a lot of stuff globally? Perhaps your modules can accept the grid you want to modify as a parameter?


Find index of lowest unique integer in each line of the file The code below finds the index of the lowest unique integer on each line of the file. The numbering starts at 1, and when there's no unique value, the 0 should be output.

The sample input looks like this:

3 3 9 1 6 5 8 1 5 3
9 2 9 9 1 8 8 8 2 1 1

And the sample output is this:

5
0

I wonder if this code can be simplified or made more elegant and idiomatic in any way. It's also interesting to me, could I get by w/o the IntMap and solve the problem with i.e. lists beautifully.

module Main where

import Prelude hiding (filter)
import Control.Applicative ((<$>))
import Data.IntMap (IntMap, alter, empty, filter, findMin, fromList, size)

import System.Environment

counts :: [(Int, Int)] -> IntMap [Int]
counts = foldl f empty
  where f m (i,v) = alter f' v m
          where f' :: Maybe [Int] -> Maybe [Int]
                f' (Just is) = Just (i:is)
                f' (Nothing) = Just [i]

g :: IntMap [Int] -> Int
g a = i
  where rs = filter ((== 1) . length) a
        h m | size m == 0 = fromList [(0, [0])]
            | otherwise = m
        (_, [i]) = findMin $ h rs

minUnique :: [Int] -> Int
minUnique l = g r
  where r = counts (zip [1..] l)

main =
  head <$> getArgs >>= readFile >>=
    putStrLn . unlines . map (show . minUnique . map read . words) . lines

First off, your usage of alter can easily be replaced by insertWith:

counts :: [(Int, Int)] -> IntMap [Int]
counts = foldr f empty
  where f (i,v) = insertWith (const (i:)) v [i]

You could also use fromListWith, which is neater but less efficient due to the need to concatenate lists:

counts :: [(Int, Int)] -> IntMap [Int]
counts = fromListWith (flip (++)) . map (\(i,v) -> (v,[i]))

Second, you could shorten g quite a bit as well using a view:

g a = case minView $ filter ((== 1) . length) a of
        Nothing      -> 0
        Just ([i],_) -> i

Which is also probably slightly less efficient, as it will construct at least a thunk for the rest of the IntMap.

Concerning different approaches - you could make a recursive function that removes all duplicates:

import Data.List
import Data.Ord

dedup :: [(Int,Int)] -> [(Int,Int)]
dedup [] = []
dedup ((i,v):ps)
  | null dups = (i,v):dedup ps
  | otherwise = dedup ndups
  where (dups, ndups) = partition ((==v) . snd) ps

minUnique :: [Int] -> Int
minUnique l
  | null ddups = 0
  | otherwise  = fst $ minimumBy (comparing snd) $ ddups
  where ddups = dedup $ zip [1..] l

Once more this trades for a bit of efficiency, as the traversal in partition makes it O(n²). Not sure whether that can be helped though without involving more complex data structures.


jQuery plugin to highlight text spans in various colors I am trying to tackle two fairly massive concepts through JavaScript and jQuery, the first being object-oriented programming and the second being how to build robust modular plugins with an OOP approach. So far I've learned it's hard.

Here is an example plugin I built, that I somewhat based my much larger project on; any opinions you guys have on my approach would be awesome.

;(function($, window, document, undefined) {
  "use strict";

  $.fn.myPlugin = function(opts) {

  var settings = $.extend({},{ color  : 'blue', normal : 'black' }, opts);

  var proto  = {
    show : function () { $(this.self).css( "color", this.settings.color ); },
    hide : function () { $(this.self).css( "color", this.settings.normal); },
    log  : function () { console.log(this); }
  }

  return this.each(function(id, elem) {

    if (!$.data(elem, 'parent')) // Base parent object
      $.data(elem, 'parent', Object.create(proto, {
        'settings' : { value        : settings,
                       writable     : true,
                       enumerable   : true,
                       configurable : true
        }
      }));

      $(elem).children('span').each(function(idx, elmx) {

        if (!$.data(elmx, 'child')) // Child obj w/ delegation from parent
          $.data(elmx, 'child', Object.create($.data(elem, 'parent'), {
            'foo'  : { value        : 'bar',
                       writable     : true,
                       enumerable   : true,
                       configurable : true
            },

            'self' : { value        : elmx,
                       writable     : false,
                       enumerable   : true,
                       configurable : false
            }
          }));

        $(elmx).hover(
          function() {
            $.data(elmx, 'child').log();
            $.data(elmx, 'child').show();
          }, function() {
            $.data(elmx, 'child').hide();
          });
        });
      });
  };
}(jQuery, window, document));

Here is where the code is running, basically all it does is search for <span> tags inside of the <p> tags the plugin is instantiated on and highlights them with a given user setting or the defaults. It shows the use of delegation for inheritance and very basic event management (no callbacks as of yet)

The example you posted makes no use of inherited behaviour as far as I can tell, and can be implemented as the example below.

I think however that you are preparing for something that this example does not showcase, since you're asking about delegated implementations.
It's hard to tell what exactly you are aiming for though, so for now I'll just supply the simplification of your example.

;(function($, window, document, undefined) {
  "use strict";

  function show(elem, settings) {
    $(elem).css("color", settings.color);
  }

  function hide(elem, settings){
    $(elem).css("color", settings.normal);
  }

  var defaultSettings = {
      color: 'blue',
      normal: 'black'
  };

  function setHoverHandlers(elem, settings) {
    $(elem).hover(function() {
      show(elem, settings);
    }, function() {
      hide(elem, settings);
    });
  }

  $.fn.myPlugin = function(opts) {
    var elemSettings = $.extend({}, defaultSettings, opts);

    return this.each(function(id, elem) {
      $(elem).children('span').each(function(_, elem){
        setHoverHandlers(elem, elemSettings);
      });
    });
  };
}(jQuery, window, document));

As a side note, I generally discourage the use of Object.create, in favour of using plain data structures and functions working on them, or encapsulating state using closures. Crockford elaborates on it in this talk.

As another side note, it's funny that Crockford holds this talk, when he is in fact the single reason thatObject.create is in JavaScript in the first place. It was literally added to the standard after him nagging about it long enough.


Handling newly devices added to or removed from a list I have two lists that I need to intersect. This is what I'm doing.

    var dfuDevices = _dfuMonitor.GetDevices();

    // new devices
    dfuDevices.Except(Devices).AsParallel().ForAll(device => {
        if (device.GetType() == typeof(DFUDeviceModel)) {
            OnDfuDeviceDetected(device);
        }
    });

    // removed devices
    Devices.Except(dfuDevices).AsParallel().ForAll(device => {
        if (device.Equals(ConnectedDevice))
            Disconnect();

        if (device is DFUDeviceModel)
            OnDfuDeviceRemoved(device);
    });

    // refresh list
    Devices = Devices.Intersect(dfuDevices);

I need to react accordingly to each item that is either removed or added. Is there any way to reduce this code?

Detecting changes

What you need is an ObservableCollection:

  Represents a dynamic data collection that provides notifications when items get added, removed, or when the whole list is refreshed.

Your code is very short so there is not much to comment on but there is one more thing that you can improve.

dfuDevices.Except(Devices).AsParallel().ForAll(device => {
    if (device.GetType() == typeof(DFUDeviceModel)) {
        OnDfuDeviceDetected(device);
    }
});

You can simplify this by using the OfType<T>() extension that

  Filters the elements of an IEnumerable based on a specified type.

dfuDevices
    .Except(Devices)
    .OfType<DFUDeviceModel>()
    .AsParallel()
    .ForAll(OnDfuDeviceDetected);


Calculating the biggest number of consecutive ones in a binary number The following code calculates the biggest number of consecutive ones in a binary representation of a number. For example 6 -> 2 (110), 5 -> 1 (101):

String numberAsBinaryString = Integer.toBinaryString(number);
List<String> listOfConsecutiveOneStrings = Arrays.asList(numberAsBinaryString.split("0"));

System.out.println(listOfConsecutiveOneStrings.stream()
                                                  .max(Comparator.comparing(String::length))
                                                  .map(onesSequence -> onesSequence.length())
                                                  .get());

I wonder if there is a more elegant way of writing this especially since this solution involves quite a few conversions. Any opinions are appreciated.

How about this? (maybe needs some adaption in Java):

int consBits(int x) {
    int consecutiveBits = 0;
    for(;x;consecutiveBits++)
        x &= (x<<1u);

    return consecutiveBits;
}

The idea is to binary & a number with a shifted version of itself. If it is not 0 there is a consecutive sequence of ones. Repeat until the result is 0 and we get the total length of consecutive ones.


Leetcode Two Sum code in Python Here's my solution for the Leet Code's Two Sum problem -- would love feedback on (1) code efficiency and (2) style/formatting.

Problem:

  Given an array of integers, return indices of the two numbers such
  that they add up to a specific target.

  You may assume that each input would have exactly one solution, and
  you may not use the same element twice.

  Example:

  Given nums = [2, 7, 11, 15], target = 9,

  Because nums[0] + nums[1] = 2 + 7 = 9, return [0, 1].

My solution:

def twoSum(nums, target):
    """
    :type nums: List[int]
    :type target: int
    :rtype: List[int]
    """

    num_lst = list(range(len(nums)))

    for indx, num in enumerate(num_lst):
        for num_other in num_lst[indx+1:]:
            if nums[num] + nums[num_other] == target:
                return [num, num_other]
            else:
                continue
    return None

Code Style

Your code contains a few lines that accomplish nothing and obfuscate your intent:

    else:
        continue

If the conditional is false, you'll automatically continue on to the next iteration without having to tell the program to do that.

    return None

All Python functions implicitly return None. While PEP 8 appears to endorse this practice ("explicit is better than implicit"), it seems noisy to me.
num_lst = list(range(len(nums))) effectively generates a list of all the indices in the nums input list. Then, you immediately enumerate this list, which produces pairs of identical indices indx, num. If all you're attempting to do is iterate, this is significant obfuscation; simply call enumerate directly on nums to produce index-element tuples:

def twoSum(self, nums, target):
    for i, num in enumerate(nums):
        for j in range(i + 1, len(nums)):
            if num + nums[j] == target:
                return [i, j]

This makes the intent much clearer: there are no duplicate variables with different names representing the same thing. It also saves unnecessary space and overhead associated with creating a list from a range.
Following on the previous item, indx, num and num_lst are confusing variable names, especially when they're all actually indices (which are technically numbers).

Efficiency

This code is inefficient, running in quadratic time, or  \$\mathcal{O}(n^2)\$. Leetcode is generous to let this pass (but won't be so forgiving in the future!). The reason for this is the nested loop; for every element in your list, you iterate over every other element to draw comparisons. A linear solution should finish in ~65 ms, while this takes ~4400 ms.

Here is an efficient solution that runs in \$\mathcal{O}(n)\$ time:

hist = {}

for i, n in enumerate(nums):
    if target - n in hist:
        return [hist[target-n], i]
    hist[n] = i

How does this work? The magic of hashing. The dictionary hist offers constant \$\mathcal{O}(1)\$ lookup time. Whenever we visit a new element in nums, we check to see if its sum complement is in the dictionary; else, we store it in the dictionary as a num => index pair.

This is the classic time-space tradeoff: the quadratic solution is slow but space efficient, while this solution takes more space but gains a huge boost in speed. In almost every case, choose speed over space.

For completeness, even if you were in a space-constrained environment, there is a fast solution that uses \$\mathcal{O}(1)\$ space and \$\mathcal{O}(n\log{}n)\$ time. This solution is worth knowing about for the practicality of the technique and the fact that it's a common interview follow-up. The way it works is:

Sort nums.
Create two pointers representing an index at 0 and an index at len(nums) - 1.
Sum the elements at the pointers.

If they produce the desired sum, return the pointer indices.
Otherwise, if the sum is less than the target, increment the left pointer
Otherwise, decrement the right pointer.

Go back to step 3 unless the pointers are pointing to the same element, in which case return failure.

Be wary of list slicing; it's often a hidden linear performance hit. Removing this slice as the nested loop code above illustrates doesn't improve the quadratic time complexity, but it does reduce overhead.

Now you're ready to try 3 Sum!


Reduce a JavaScript array into named objects The code below reduces an array into "named" objects (not sure if that's the correct terminology!)

It works, but I'm sure the code could be improved. There is some repetition going on in the reduce.

It checks if a key exists (if (accumulator[name])). If not then initialize the results array, if it does then push onto the results array.

let response = {
  columns: [
    'n'
  ],
  data: [
    {
      graph: {
        nodes: [
          {
            id: '169',
            labels: [
              'Container'
            ],
            properties: {
              reference: 'REF002',
              name: 'Cupboard',
              id: '003'
            }
          }
        ],
        relationships: []
      }
    },
    {
      graph: {
        nodes: [
          {
            id: '170',
            labels: [
              'Container'
            ],
            properties: {
              reference: 'REF003',
              name: 'Cupboard A',
              id: '03a'
            }
          }
        ],
        relationships: []
      }
    },
    {
      graph: {
        nodes: [
          {
            id: '964',
            labels: [
              'Equipment'
            ],
            properties: {
              reference: 'REF004',
              name: 'Cupboard B',
              id: '03b'
            }
          }
        ],
        relationships: []
      }
    }
  ]
}

const result = response.data.reduce(
  (accumulator, currentValue, currentIndex, array) => {
    const name = currentValue.graph.nodes[0].labels[0];

    if (accumulator[name]) {
      accumulator[name].results.push({
        title: currentValue.graph.nodes[0].properties.name,
        description: currentValue.graph.nodes[0].properties.reference
      });
    } else {
      accumulator[name] = {
        name,
        results: [
          {
            title: currentValue.graph.nodes[0].properties.name,
            description: currentValue.graph.nodes[0].properties.reference
          }
        ]
      };
    }

    return accumulator;
  },
  {}
);

console.clear();
console.log(result);

Output Required

{
  Container: {
    name: 'Container',
    results: [
      {
        title: 'Cupboard',
        description: 'REF002'
      },
      {
        title: 'Cupboard A',
        description: 'REF003'
      }
    ]
  },
  Foo: {
    name: 'Foo',
    results: [
      {
        title: 'Cupboard B',
        description: 'REF004'
      }
    ]
  }
}

As it looks like you are using ES6 syntax, you could also throw in some destructuring assignments, but I guess the main point is that one would need to create the output object only once. One could also extract the accumulation into a descriptive name:

byFirstLabel = (acc, {graph: {nodes: [node]}}) => {
  let label = node.labels[0]
  let { name: title, reference: description } = node.properties
  let entry = { title, description }

  acc[label] ? acc[label].results.push(entry) :
    acc[label] = { name: label, results: [ entry ] }

  return acc
}

response.data.reduce(byFirstLabel, {})


Creating a table for use in Excel I'm looking for some assistance with a PHP script I'm writing. It creates a table that is then exported to an Excel file for download.

The issues as I see them are as follows:

$students contains 559 items and the $classes has 22 items and $attributes has 6 items:

    <?php foreach($students as $student){
    $pupil = Students::retrieveByPK($student['adno']); ?>

    <?php echo strtoupper($pupil->surname); ?>
    <?php echo $pupil->first_name; ?>
    <?php echo $pupil->year_group; ?>
    <?php foreach($classes as $class){
        $shortYear = str_replace('Year ', '',$pupil->year_group);
        $slashPos = strpos($class['class_name'],'/');
        $className = $shortYear.substr($class['class_name'],1);
        $classId = Classes::idFromClassName($className);
        foreach($attributes as $attribute){
           $attr = AttributeResults::getExportAttribute($pupil->adno, $classId, $attribute['attribute_id']); ?>
        <?php echo $attr[0]['attribute_value']; ?>
        <?php } ?>
     <?php } ?>

    <?php } ?>

I'm essentially running the AttributeResults::getExportAttribute method in the region of 74000 times. Is there a better way of bringing this data back?

I think what your better off doing is to have a join on your database call, so that the class data and student data is returned in one object.


SQL Injection and php performance checking I hope this is not a really bad question for a first time here beginner.

This piece of code selects from the left table and will list the content in the right hand table. This is a working code but I would like to see how a professional would protect and make it faster.

Any suggestion (with some code) would be appreciated. Thanks a lot

PS: There is also a little glitch with it: after deleting it lose the selected item on the right list.

<?php include("db_con1.php");?>

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

<?php
  $queryl = $pdo->prepare('SELECT id, name FROM test1 ORDER BY name ASC');
  $queryl->execute();
?>

  <?php foreach ($queryl as $i => $rowl) { ?>

   <?php if ($i) {?>
  <input name="checkbox1_del[]" id="test_<?php echo $i ?>" type="checkbox" value="<? echo $rowl['id']; ?>"/>
  <label for="test_<?php echo $i ?>">
   <a href="test1.php?gid=<?php echo $rowl['id']; ?>"><?php echo $rowl['name']; ?>

  <?php } ?>

<?php
  if(isset($_GET['gid'])) {
   $gid=$_GET['gid'];
   $queryr = $pdo->prepare('SELECT test3.name FROM test1, test2, test3 WHERE test1.id=test2.groupid AND test3.id=test2.peopleid AND test1.id='.$gid.' ORDER BY test3.name ASC');
   $queryr->execute();
  }
?>

  <?php foreach ($queryr as $i => $rowr) { ?>

      <?php if ($i) {?>
      <input name="checkbox2_del[]" id="test_<?php echo $i ?>" type="checkbox" value="<? echo $rowr['id']; ?>"/>
      <label for="test_<?php echo $i ?>"><?php echo $rowr['name']; ?>

  <?php } ?>

<input type="submit" name="del" value="Delete the selected items">
</form>

<?php
if (isset($_POST['del'])) {
echo "Don't delete:)";
  for ($c = 0; $c < count($_POST['checkbox1_del']); $c++){
    $checkbox1_del = $_POST['checkbox1_del'][$c];
    $sql = 'UPDATE test1 SET status=0, log="'.date("Y-m-d").'"WHERE id='.$checkbox1_del;
    echo $sql;
    $query = $pdo->prepare($sql);
    $query->execute();
  }

  for ($c = 0; $c < count($_POST['checkbox2_del']); $c++){
    $checkbox2_del = $_POST['checkbox2_del'][$c];
    $sql = 'UPDATE test2 SET status=0, log="'.date("Y-m-d").'"WHERE id='.$checkbox2_del;
    echo $sql;
    $query = $pdo->prepare($sql);
    $query->execute();
   }

    if($query){
      echo "<meta http-equiv=\"refresh\" content=\"0;URL=test1.php\">";
     }
 }
?>

Revise 1: This is a extracted part of my program, however here is the definition of this function:

Having 3-5000 contact, to send emails them easier to group them to Distribution list.
So this part of the program provide 3 tables:

Distribution list   |    Members of selected group   |   People

you can make group names and can add people into it.

So basic functions that I wrote for this part:

new distribution list can be added
By clicking on any Distribution list you'll get the Members table ($_GET)
Also by clicking on Distribution list should be BOLD (class="bold") as highlighted
Selecting any members in the member list can be deleted (checkbox2_del())
From people list anyone can be added to the list

This is what I tried to write and this test shows that basic steps of it. I just didn't want to overload  anybody to read through the whole script. If anyone would like to see I happy to replace with it. Thanks

You are using prepared SQL statements, but not using parameterised queries in them, so you're still wide open to SQL injections. This is how you should be doing it:

$gid = $_GET['gid'];
$queryr = $pdo->prepare('SELECT test3.name FROM test1, test2, test3 WHERE test1.id = test2.groupid AND test3.id = test2.peopleid AND test1.id = :gid ORDER BY test3.name ASC');
$queryr->bindParam(':gid', $gid, PDO::PARAM_STR);
$queryr->execute();

Assuming that your $gid is supposed to be numeric, you should probably extract it this way too:

if(is_numeric($_GET['gid'])) {
  $gid = (int)$_GET['gid'];
  // ...
  $queryr->bindParam(':gid', $gid, PDO::PARAM_INT);

The same applies to your later queries that are being generated from $_POST.

This segment also seems to be missing a set of braces. If it works, then it's likely only a coincidence. if without braces is hard enough to read in just PHP, but if you drop out of PHP, I honestly have no idea how PHP will react.

  <?php foreach ($queryl as $i => $rowl) { ?>

   <?php if ($i)?>
  <input name="checkbox_del[]" id="test_<?php echo $i ?>" type="checkbox" value="<? echo $rowl['id']; ?>"/>
  <label for="test_<?php echo $i ?>">
   <a href="test1.php?gid=<?php echo $rowl['id']; ?>"><?php echo $rowl['name']; ?>

  <?php } ?>

There is a larger problem with this script in general. Mixing logic with the output can cause design problems. Ideally you would load all your data at the top, then generate your page all at once. I don't know how big a system this is, but in general it's much easier to manage the system as a whole if you structure your pages this way. What happens if you want to conditionally show something at the top of the page, but the condition isn't generated till later when you are processing data?

Eg, you might end up with something like the following:

<?php
include("db_con1.php");
$queryl = $pdo->prepare('SELECT id, name FROM test1 ORDER BY name ASC');
$queryl->execute();

$lists = array();
while (($rowl = $queryl->fetch(PDO::FETCH_ASSOC)) !== false) {
  $lists[] = $rowl;
}

// And so on for all the data you need to load
?>

It seems your right bar and left bar aren't associated directly, but if they were then you could put more in that while loop to load stuff into $rowl before adding it to $lists. You can then just iterate over this array in your code. All your SQL is at the top of the file and easy to change, all your presentation is at the bottom of the file and will read a lot nicer too, with just some loops and some echo's.


Beginner number guessing game in Java I am a first year CS student. We are currently learning Java and my latest assignment was to create this random number game. I am hoping to get some feedback on code style etc so that I know what not to do in the future and so that I can avoid getting into bad habits from the start. I don't know all that much so any explanations/tips at all will be amazing!

import java.util.Random;
import java.util.Scanner;

public class Main {

    public static void main(String[] args) {
        Scanner scan = new Scanner(System.in);
        boolean win = false;
        boolean running = true;
        System.out.println("Please enter the upper limit: ");
        Game game = new Game(scan.nextInt());

        while (running) {

            System.out.println("Please enter a guess ");

            while (!win) {
                if (game.guess(scan.nextInt())) {
                    win = true;

                }
            }
            System.out.println("Would you like to play again y/n?");
            String playAgain = scan.next();
            if (playAgain.equalsIgnoreCase("y")) {
                game.reset();
                running = true;
                win = false;
            } else {
                running = false;
                System.out.println("Game over");
            }
        }
    }
}

Game class:

import java.util.Random;

public class Game {

    private int randomNumber;
    private int tries = 0;
    private int range;

    public Game(int range) {
        this.range = range;
        generateRandomNumber();
    }

    private void generateRandomNumber() {
        Random rand = new Random();
        this.randomNumber = rand.nextInt(range) + 1;
    }

    public boolean guess(int guess) {
        tries++;
        if (guess == randomNumber) {
            if (tries > 1) {
                System.out.println("You got the number in " + tries + " tries.");
                return true;
            } else {
                System.out.println("You got the number in " + tries + " try.");
                return true;
            }
        } else if (guess > randomNumber) {
            System.out.println("Too high try again");
            return false;
        } else if (guess < randomNumber) {
            System.out.println("Too low try again ");
            return false;
        }
        return false;

    }

    public void reset() {
        this.tries = 0;
        generateRandomNumber();

    }

}

This is a pretty good implementation of what you've set out to do. The naming of things is clear and the code is pretty well split up into methods.

Main Class

I'd maybe put a slightly clearer explanation of what the user is expected to do at the start, as well as your "enter an upper limit". This is usually an excuse to break out your finest ascii art.

Something else to consider is what if your user doesn't enter an integer? That will break your program, so putting in some error handling that asks your error-prone meatbag to try again and get it right this time, please. Your way of checking is to put the scan.nextInt() in a try/catch block and catch an InputMismatchException.

When you're asking the player if they want to play again, you set running to true, but there's no way for it not to be true, so that's unnecessary.

Game class

You're seeding rand every time you're calling generateRandomNumber(), which isn't really going to be an issue here, as you're not calling that method quickly/often, but you should really only seed a random number generator once in a program, otherwise you'll start having it return the same value over and over again, so maybe move your Random rand = new Random() to the constructor?

In guess(), I'd rearrange the first section to this:

 if (guess == randomNumber) {
        if (tries > 1) {
            System.out.println("You got the number in " + tries + " tries.");
        } else {
            System.out.println("You got the number in " + tries + " try.");
        }
        return true;
 }

You're repeating yourself less and the return value is the same regardless. The same goes for the next part, instead, I'd have:

 else if (guess > randomNumber) {
        System.out.println("Too high try again");
    } else if (guess < randomNumber) {
        System.out.println("Too low try again ");
    }
    return false;


Summation of Arithmetic Progression Modulo Series Here is the link to the problem: Aladin on Kattis.com

  Problem

  Aladin was walking down the path one day when he found the strangest
  thing: N empty boxes right next to a weird alien machine. After a bit
  of fumbling around he got the machine to do something. The machine now
  accepts 4 integers \$L\$, \$R\$, \$A\$ and \$B\$. After that hitting the big red
  glowing button labeled "NE DIRAJ"1 causes the machine to go crazy and
  follow the next routine:

  Set the number of stones in the box labeled L to A modulo B.
  It proceeds to fly to the box labeled L + 1, and set the number of stones there to (2 ⋅ A) mod B.
  It procedes to fly to the box labeled L + 2, and set the number of stones there to (3 ⋅ A) modB.
  Generally, it visits each box labeled between L and R, and set the number of stones there to ((X − L + 1) ⋅ A) mod B, where X is the box label.
  After it visits the box labeled R. It settles down for further instructions.

  During the game Aladin wonders what is the total number of stones in some range of boxes.

  Write a program that simulates the device and answers Aladin's questions.

  Input

  The first line contains two integers: the number of boxes \$N\$ (\$1 \le N \le 1000000000\$) and the number of queries \$Q\$ (\$1 \le Q \le 50000\$).

  The next \$Q\$ lines contain information about the simulation.

  If the line starts with 1, than it follows the format "1 L R A B"
  (\$1 \le L \le R \le N\$) (\$1 \le A, B \le 1000000\$), meaning that Aladin keyed in numbers \$L\$, \$R\$, \$A\$ and \$B\$ in the device and allowed the device to do its job.

  If the line starts with 2, then it follows the format "2 L R"
  (\$1 \le L \le R \le N\$), meaning that Aladin wonders how many stones in total are
  ther stones are in boxes labeled \$L\$ to \$R\$ (inclusive).

  Output

  For each query beginning with 2 output the answer to that particular query. Queries should be processed in the order they
  are given in the input.

  Sample Input 1

6 3
2 1 6
1 1 5 1 2
2 1 6

  Sample Output 1

0
3

  Sample Input 2

4 5
1 1 4 3 4
2 1 1
2 2 2
2 3 3
2 4 4

  Sample Output 2

3
2
1
0

  Sample Input 3

4 4
1 1 4 7 9
2 1 4
1 1 4 1 1
2 1 4

  Sample Output 3

16
0

#include<iostream>
#include<fstream>
#include<string>

using namespace std;

long countStones(long, long, long, long, long, long);

int main(int __argc, char *__argv[])
{
    std::string filepath;
    long N, Q, *boxes;

  if(__argc == 2) {
    filepath = std::string(__argv[1]);
  }

  std::ifstream* infile = !filepath.empty() ? new std::ifstream(filepath.c_str()) : static_cast<std::ifstream*>(&std::cin);

  *infile >> N >> Q;

  long opt, L1 = 0, R1 = 0, L2 = 0, R2 = 0, A, B;

  for(long i = 1; i <= Q; i++) {
    *infile >> opt;

    if(opt == 1) {
      *infile >> L1 >> R1 >> A >> B;
    }
    if(opt == 2) {
      *infile >> L2 >> R2;
      cout << countStones(L1, R1, L2, R2, A, B) << endl;
    }
  }

    return 0;
}

long countStones(long L1, long R1, long L2, long R2, long A, long B) {
  long ret = 0, start = L1, end = R1;

  if(R2 < L1) return 0;
  if(L2 > R1) return 0;

  if (L2 > L1) start = L2;
  if (R2 < R1) end = R2;

  for(long i = start; i <= end; i++) {
    ret += ((i - L1 + 1) * A) % B;
  }

  return ret;
}

The problem works fine with all the with Sample Cases #1, #2, & #3. But when I upload the code on the website (open.kattis.com) it fails on the case #4, which is unknown to me. I couldn't find a logical error in my code.

Also, when I had written the code which was storing the modulo calculations for each box (array) the problem did pass the case #4 but failed #5 (Memory Exceeded). So I changed the code to NOT STORE the values in the array & just calculate on the fly & add it to the count of stones.

I need to modify the code so that it passes Test #4, whose input values are unknown to me. That is to find the logical error in the code.

Don't use underscore as a prefix. In most situations this is a reserved identifier. Defiantly never use double underscore in an identifier. This is reserved in all situations.

__argc  /// This is so wrong. It will break.

Read a couple of other C++ reviews. They all explain why this is a bad idea:

using namespace std;

This is not old school C. You don't need to declare the variables at the top of the function.

long N, Q, *boxes;

Declare them as close to the point of use as possible. This becomes important when your types have constructor/destructor (as they execute code). But it also makes the code easier to read as the type information is close to the usage of the object.

Please declare one variable per line (easier to read).

long    N;
long    Q;
long*   boxes; // The * belongs close to the type (as it is part of the type info).
               // PS you probably should not be using pointers. In modern C++ they
               // are vanishingly rare (as there is no ownership semantics associated
               // with a pointer).

Don't do this.

std::ifstream* infile = !filepath.empty() ? new std::ifstream(filepath.c_str()) : static_cast<std::ifstream*>(&std::cin)

Should I call delete on this? Yes if it is a file. No if it is std::cin. So not a good solution. Also std::cin is not a file stream so casting it is going to do nasty things. You should never cast things (it means you have something wrong with your design (or you are working at very low level (which most people should not be doing))).

But they are both std::istream. So you could do this.

std::ifstream   file;
if (!filepath.empty()) {
    file.open(filepath);       // Modern C++ allows std::string here.
}

std::istream& infile = !filepath.empty()
                             ? file
                             : std::cin;

You may want to check that the read works:

*infile >> N >> Q;

Very easily done with:

if (infile >> N >> Q) {
    // Read was good.
    // Do stuff.
}

Main is special. So does not need a return code.

return 0;

So if your application never fails (ie always returns 0). Use this info. A missing return in main is an indication that the application never fails.


Building a 64 × 64 particle accelerator frame in Minecraft with a computercraft turtle I dont think this will work anywhere besides in minecraft with computercraft but it's all correct syntax. I just feel like it has some lines of code I could eliminated somehow and made it a cleaner script. I appreciate any constructive criticism to help me further understand what I'm misunderstanding.

function placef()
  turtle.select(1)
  x = 1
  if turtle.getItemCount(x) == 0 then
  repeat turtle.select(x+1)
  x = x + 1
  if x == 17 then
  x = 1
  y = 2
  end
  if y == 2 then
  os.reboot()
  end
  until turtle.getItemCount(x) > 0
 end
 turtle.place()
end

function placeup()
  turtle.select(9)
  x = 9
  if turtle.getItemCount(x) == 0 then
  repeat turtle.select(x+1)
  x = x + 1
  if x == 17 then
  x = 9
  y = 2
  end
  if y == 2 then
  os.reboot()
  end
  until turtle.getItemCount(x) > 0
  end
  turtle.placeUp()
end

function place()
  turtle.select(9)

x = 9
  if turtle.getItemCount(x) == 0 then
  repeat turtle.select(x+1)
  x = x + 1
  if x == 17 then
  x = 9
  y = 2
  end
  if y == 16 then
  os.reboot()
  end
  until turtle.getItemCount(x) > 0
  end
  turtle.placeDown()
end

function repairOT()
  turtle.select(1)
  x = 1
  if turtle.getItemCount(x) == 0 then
  repeat turtle.select(x+1)
  x = x + 1

  if x == 17 then
  x = 1
  y = 2
  end
  if y == 2 then
  os.reboot()
  end
  until turtle.getItemCount(x) > 0
  end
  if turtle.compareDown() == false then
  place()
  turtle.turnRight()
  else
  turtle.turnRight()
  end
  if turtle.compare() == false then
  placef()
  turtle.turnLeft()
  else
  turtle.turnLeft()
  end

turtle.forward()
end

function repairBI()
  turtle.select(1)
  x = 1
  if turtle.getItemCount(x) == 0 then
  repeat turtle.select(x+1)
  x = x + 1
  if x == 17 then
  x = 1
  y = 2
  end
  if y == 16 then
  os.reboot()
  end
  until turtle.getItemCount(x) > 0
  end
  if turtle.compareUp() == false then
  placeup()
  turtle.turnLeft()
  else
  turtle.turnLeft()
  end
  if turtle.compare() == false then
  placef()
  turtle.turnRight()
  else
  turtle.turnRight()
  end
 turtle.forward()
end

These functions are where I think I could do away with alot of lines of code but I'm not sure how. Maybe an anonymous function and a class. Don't really quite understand using those very well yet.

https://pastebin.com/JDZSibmn There is the full script just in case anyone wants to see it. The rest is just loops.

First of all, use more local. It will save you a lot of headache in the future.

Instead of the three place functions, you could just do

local function selectAnyBlock()
  for i=1,16 -- 4 x 4 inventory
    if turtle.getItemCount(i) > 0 then
      return true
    end
  end
  os.reboot() -- No idea why you'd want to reboot here though
end

and then call turtle.place[up|down]() after that. That saves you two functions and some code.

As for the last two functions, I have no idea what they are supposed to do. They seem to place a block up or down, then turn left and place a block in front?

They also make use of that construct that selects a block in the inventory, so you could replace that with the above selectAnyBlock() function. There's also some places where you have the same instruction (turtle.turnLeft()) in both code paths of a condition. Just place them after the if and you'll only have to write them once.

You don't have to compare booleans to true or false; you can just check for them directly in a condition:

if turtle.compareUp() == false then
  placeup()
  turtle.turnLeft()
else
  turtle.turnLeft()
end

turns into

if not turtle.compareUp() then
  placeup()
end
turtle.turnLeft()

I don't really see a need for object orientation here. Maybe it makes sense in the program overall, but your example works well with just functions. Same goes for anonymous functions; useful as they are, sometimes you just don't need them.

Overall, try indenting your code like everybody else does (each new scope has its own indentation level) and add some comments to clarify your intentions. This not only helps others reading your code, but also yourself in the future :)


Random playlist builder is slow I wrote a Python script to scan my music library, randomly select 50 songs, and play them in VLC media player. It works great, just a little slow (takes several seconds to launch).

I know that walking through my entire music library (thousands of songs) is a brute force (slow) way to do it. What would be a faster approach?

I would also appreciate any suggestions on syntax or Pythonic improvements.

from subprocess import Popen
import os, sys
import random

# http://ubuntuforums.org/showthread.php?t=2136340
# http://stackoverflow.com/questions/19859840/excluding-directories-in-os-walk
def scanfolder(rootpath, exclude_dirs, file_ext):
    musiclist = []
    for path, dirs, files in os.walk(rootpath):
        dirs[:] = [d for d in dirs if d not in exclude_dirs]
        for f in files:
            if f.endswith(file_ext):
                mp3 = os.path.join(path, f)
                musiclist.append(mp3)
    return musiclist

# input variables
vlcpath = r'C:\Program Files (x86)\VideoLAN\VLC\vlc.exe'
musicpath = r'G:\Music Collection'
excludedirectories = ['_baby music','Buble, Michael','ballroom dancing']
filetype = r'.mp3'
len_playlist = 50

# run script
print 'party on... filling glasses!'
mp3_list = scanfolder(musicpath, excludedirectories, filetype)
playlist = random.sample( mp3_list, min(len_playlist, len(mp3_list)) )
Popen([vlcpath] + playlist)
print 'done: added %i items from library containing %i \'%s\' files' % (len(playlist), len(mp3_list), filetype)

This is a different approach to the problem. Here is the idea:

I am making the assumption that your music collection does not change too much between start-ups of your script and that all your songs are longer than it takes to scan your entire music collection.

Introduce a function to save the mp3_list to a file
On start-up look for that file and start playing a first random song (where we need to check that it still exists) from that file if it exists.
Meanwhile or if the file does not exist yet, (re-)scan the music collection in the background, while the first song is playing and write it back to the file once it is finished.
After the first song, just play n - 1 songs from the now up-to-date music database (if the scanning is finished, otherwise use the old file).

This approach might be slower than your approach, if many music files disappeared since the last start. It might fail if all of them disappeared (but we can check for that).

This approach has a different random distribution for the first song. (Consider that you only had one song in your directory during the first start of the script, and 1000 now. It will chose that one song on start-up the next time).

And here is the code. I used the python bindings of vlc, which are available here from their official github. Just download vlc.py and save it in the same directory as the script.

The functions read_db and save_db should be self-explanatory. get_existing_file gives one random track from mp3_list, making sure it actually exists.

When calling media_player.play(), this starts the playback in the background, so we are free to scan the folders now and save the new state.

After that I included some rudimentary playback controls.

By default, the player will continue looping through the playlist. Type stop, followed by Enter to stop it and exit the script.

This could use some more encapsulation of the methods, but it is a start.

import os
import sys
import random
import vlc
import time

# http://ubuntuforums.org/showthread.php?t=2136340
# http://stackoverflow.com/questions/19859840/excluding-directories-in-os-walk

def scan_folder(rootpath, exclude_dirs, file_ext):
    musiclist = []
    for path, dirs, files in os.walk(rootpath):
        dirs[:] = [d for d in dirs if d not in exclude_dirs]
        for f in files:
            if f.endswith(file_ext):
                mp3 = os.path.join(path, f)
                musiclist.append(mp3)
    return musiclist

def save_db(mp3_list, file_name):
    """Write `mp3_list` to `file_name`"""
    with open(file_name, "w") as db_file:
        db_file.write("\n".join(mp3_list))

def read_db(file_name):
    """read `mp3_list` from `file_name`"""
    mp3_list = []
    with open(file_name) as db_file:
        for line in db_file:
            mp3_list.append(line.strip())
    return mp3_list

def get_existing_file(mp3_list):
    """Get a random file name from `mp3_list` that exists"""
    first_song = random.choice(mp3_list)
    while not os.path.isfile(first_song):
        first_song = random.choice(mp3_list)
    return first_song

if __name__ == "__main__":
    # input variables
    vlcpath = r'/usr/bin/vlc'
    musicpath = r'/home/andreas/Musik'
    excludedirectories = {'_baby music', 'Buble, Michael', 'ballroom dancing'}
    db_file = os.path.join(os.getenv('HOME'), ".music.db")
    filetype = r'.mp3'
    len_playlist = 50

    # get mp3_list
    print 'party on... filling glasses!'
    try:
        mp3_list = read_db(db_file)
    except IOError:
        mp3_list = scan_folder(musicpath, excludedirectories, filetype)
        save_db(mp3_list, db_file)

    # setup vlc
    vlc_instance = vlc.Instance()
    media_player = vlc_instance.media_list_player_new()

    playlist = vlc_instance.media_list_new([get_existing_file(mp3_list)])
    media_player.set_media_list(playlist)
    media_player.play()

    # Rescan library
    print "Scanning music library for changes..."
    mp3_list = scan_folder(musicpath, excludedirectories, filetype)
    save_db(mp3_list, db_file)
    # time.sleep(10)
    print "done"

    # add songs to playlist
    all_songs = [first_song] + \
        random.sample(mp3_list, min(len(mp3_list), len_playlist - 1))
    playlist = vlc_instance.media_list_new(all_songs)
    media_player.set_media_list(playlist)

    # add rudimentary playback controls
    actions = {"next": media_player.next,
               "previous": media_player.previous,
               "pause": media_player.pause,
               "stop": sys.exit}
    # keep alive
    while True:
        key_pressed = raw_input("enter next, previous, pause or stop: ")
        if key_pressed in actions:
            actions[key_pressed]()

As a next iteration, I encapsulated all media_player functionality into a VLCPlayer class and all functionality surrounding the mp3_list into a MediaLibrary class.

import os
import sys
import random
import vlc
import time

# http://ubuntuforums.org/showthread.php?t=2136340
# http://stackoverflow.com/questions/19859840/excluding-directories-in-os-walk

class VLCPlayer():

    def __init__(self):
        self.vlc_instance = vlc.Instance()
        self.media_player = self.vlc_instance.media_list_player_new()
        self.actions = {"next": self.media_player.next,
                        "previous": self.media_player.previous,
                        "pause": self.media_player.pause,
                        "play": self.media_player.play,
                        "stop": sys.exit}

    def set_media_list(self, playlist):
        self.playlist = self.vlc_instance.media_list_new(playlist)
        self.media_player.set_media_list(self.playlist)

    def play(self):
        self.media_player.play()

    def playback_control(self):
        key_pressed = raw_input("enter next, previous, pause or stop: ")
        if key_pressed in self.actions:
            self.actions[key_pressed]()
        else:
            print "Unrecognized command"

class MediaLibrary(list):

    def __init__(self, file_name, path, excluded=set(), extensions={".mp3"}):
        self.file_name = file_name
        self.path = path
        self.excluded = excluded
        self.extensions = extensions
        list.__init__(self, [])
        try:
            self.read_db()
        except IOError:
            self.scan()

    def scan(self):
        self.scan_folder()
        self.save_db()

    def scan_folder(self):
        self[:] = []
        for path, dirs, files in os.walk(self.path):
            dirs[:] = [d for d in dirs if d not in self.excluded]
            for f in files:
                for extension in self.extensions:
                    if f.endswith(extension):
                        track = os.path.join(self.path, f)
                        self.append(track)

    def save_db(self):
        """Write self to `file_name`"""
        with open(self.file_name, "w") as db_file:
            db_file.write("\n".join(self))

    def read_db(self):
        self[:] = []
        with open(self.file_name) as db_file:
            for line in db_file:
                self.append(line.strip())

    def get_existing_file(self):
        first_song = random.choice(self)
        while not os.path.isfile(first_song):
            first_song = random.choice(self)
        return first_song

    def get_songs(self, n):
        if n > len(self):
            print "Warning: less files in library than requested"
            n = len(self)
        else:
            n -= 1
        return random.sample(self, n)

if __name__ == "__main__":
    # input variables
    vlcpath = r'/usr/bin/vlc'
    musicpath = r'/home/andreas/Musik'
    excluded = {'_baby music', 'Buble, Michael', 'ballroom dancing'}
    db_file = os.path.join(os.getenv('HOME'), ".music.db")
    filetype = r'.mp3'
    len_playlist = 50
    print 'party on... filling glasses!'

    # get music list
    library = MediaLibrary(db_file, musicpath, excluded, {filetype})

    # setup vlc and start playing first song
    media_player = VLCPlayer()
    first_song = library.get_existing_file()
    media_player.set_media_list([first_song])
    media_player.play()

    # Rescan library
    library.scan()

    # add more songs to playlist
    media_player.set_media_list(library.get_songs(len_playlist))
    # add rudimentary playback controls and keep alive
    while True:
        media_player.playback_control()

This is still not perfect, VLCPlayer could inherit from vlc.MediaListPlayer, but that makes it harder, because it is immutable.


ToPaginatedListAsync I'm using this method and I would likely have a code review. Also, I have one concern and is that source.Count() is executing synchronously and I don't know how to make it async in this context.

public static class IQueryableExtensions
    {
        public static Task<PaginatedList<T>> ToPaginatedListAsync<T>(this IQueryable<T> source, int pageSize, int pageIndex = 1)
        {
            var count = source.Count();
            return
                source.Skip((pageIndex - 1) * pageSize).Take(pageSize)
                .AsAsyncEnumerable()
                .Aggregate(new PaginatedList<T>(
                    totalCount: count,
                    pageSize : pageSize,
                    pageIndex: pageIndex), (list, x) => { list.Add(x); return list; });
        }

    }

    public class PaginatedList<T> : List<T>
    {
        private int pageIndex;
        private int pageSize;
        public PaginatedList(IEnumerable<T> collection, int totalCount, int pageSize, int pageIndex) : base(collection)
        {
            TotalCount = totalCount;
            PageSize = pageSize;
            PageIndex = pageIndex;
        }

        public PaginatedList(int totalCount, int pageSize, int pageIndex) : base()
        {
            TotalCount = totalCount;
            PageSize = pageSize;
            PageIndex = pageIndex;
        }

        public PaginatedList(int capacity, int totalCount, int pageSize, int pageIndex) : base(capacity)
        {
            TotalCount = totalCount;
            PageSize = pageSize;
            PageIndex = pageIndex;
        }

        public int PageIndex
        {
            get { return pageIndex; }
            private set
            {
                if (value <= 0) throw new ArgumentOutOfRangeException(nameof(PageIndex));
                pageIndex = value;
            }
        }

        public int PageSize
        {
            get { return pageSize; }
            private set
            {
                if (value <= 0) throw new ArgumentOutOfRangeException(nameof(PageSize));
                pageSize = value;
            }
        }

        public int TotalPages => (int)Math.Ceiling(TotalCount / (double)PageSize);

        public bool HasPreviousPage => PageIndex > 1;

        public bool HasNextPage => PageIndex < TotalPages;

        public int TotalCount { get; }
    }

This code and updates are available at: https://gist.github.com/Bartmax/5a55ff88413174d49301

public static Task<PaginatedList<T>> ToPaginatedListAsync<T>(
  this IQueryable<T> source, int pageSize, int pageIndex = 1)

Your method says Async, returns a Task<PaginatedList<T>>, but... it doesn't say async.

public static async Task<PaginatedList<T>> ToPaginatedListAsync<T>(
  this IQueryable<T> source, int pageSize, int pageIndex = 1)

With the async keyword specified, you can now await your result - not sure what AsAsyncEnumerable does, but it doesn't seem necessary then:

return await Task.Run(() =>
{
    var count = source.Count();
    return
        source.Skip((pageIndex - 1) * pageSize).Take(pageSize)
        .Aggregate(new PaginatedList<T>(
            totalCount: count,
            pageSize: pageSize,
            pageIndex: pageIndex), (list, x) => { list.Add(x); return list; });
});

On the other hand, you're not really aggregating with Aggregate; LINQ stands for Language-INtegrated Query, and a query that has side effects isn't really a query.

source.Skip((pageIndex - 1) * pageSize).Take(pageSize)

That part is fine. Or is it? Why do you need to -1? Is pageIndex 1-based? That's pretty confusing, given the list itself is 0-based. What's the meaning of "page 0" then? Why make it throw when assigned to 0, in the setter, when the only caller for that setter is in the constructor? Better fail early and put a guard clause in the constructor and throw that ArgumentOutOfRangeException with a shorter stack trace.

I don't know why, the more I look at the Aggregate call, the more I want the method to be returning an IEnumerable<T>.

The caller already knows the totalCount (well it can have it), the pageIndex and the pageSize, and you're only ever returning a single page - I don't see why a PaginatedList<T> needs to know this metadata at that point.

If you can return an IEnumerable<T>, then you could yield the results and have this:

public static IEnumerable<T> OfPage<T>(this IQueryable<T> source, int pageIndex, int pageSize = 10)
{
    foreach (var item in source.Skip((pageIndex - 1) * pageSize).Take(pageSize))
    {
        yield return item;
    }
}

Notice I made the pageIndex mandatory, and pageSize optional - anyone using this API would expect to be able to pass a page index as a first argument to a method called OfPage.

Now, yield is pretty much the opposite of async, and it's not really buying anything here, since Take should already materialize the query, so you could have this async one-liner then:

public static async Task<IEnumerable<T>> OfPageAsync<T>(this IQueryable<T> source, int pageIndex, int pageSize = 10)
{
    return await Task.Run(() => source.Skip((pageIndex - 1)*pageSize).Take(pageSize));
}

If PaginatedList<T> doesn't need to know the totalCount, pageIndex and pageSize, then it doesn't need to exist - it's not adding any functionality, merely carrying metadata that the caller already knows, and if it needs to be carried along with the items on that page, then PaginatedList<T> can exist, but then as a composition:

public class PageItems<T> // not a list
{
    private readonly IEnumerable<T> _items;
    private readonly int _page;
    private readonly int _pageSize;

    public PageItems(IEnumerable<T> items, int page, int pageSize)
    {
        _items = items;
        _page = page;
        _pageSize = pageSize;
    }

    public IEnumerable<T> Items { get { return _items; } }
    public int PageIndex { get { return _pageIndex; } }
    public int PageSize { get { return _pageSize; } }
}

Notice that's exposing IEnumerable<T> and not List<T> or even IList<T>: if you don't need to do anything other than iterate these items, you don't need a List, you need an IEnumerable. The code doesn't change much:

public static async Task<PageItems<T>> OfPageAsync<T>(this IQueryable<T> source, int pageIndex, int pageSize = 10)
{
    return await Task.Run(() =>
    {
        var items = source.Skip((pageIndex - 1)*pageSize).Take(pageSize)
        return new PageItems(items, pageIndex, pageSize);
    });
}


Classifying and counting database entries using Scala map and flatMap I am new to Spark and Scala and I have solved the following problem. I have a table in database with following structure:

     id       name      eid        color
     1        John      S1         green
     2        Shaun     S2         red
     3        Shaun     S2         green
     4        Shaun     S2         green
     5        John      S1         yellow

And now I want to know how many times a person is red, green or yellow. So the result should be like this

   name     red       yellow        green
   John     0           1             1
   Shaun    1           0             2

I have written this code and it solves the problem, But I am not sure is this the best way to do it. It think my code is large for this small problem and it can be done with smaller code and with best practice. I need some guidance

  val rdd = df.rdd.map {
  case Row(id: Int, name: String, eid: String, color: String) => ((eid),List((id, name, eid, color)))
}.reduceByKey(_ ++ _)
val result = rdd.map({
  case (key, list) => {
    val red = list.count(p => p._4.equals("red"))
    val yellow = list.count(p => p._4.equals("yellow"))
    val green = list.count(p => p._4.equals("green"))
    val newList = list.map(x => (x._2, red, yellow, green))
    (key, newList.take(1))
  }
}).flatMap {
  case ((eid), list) =>
    list.map {
      case (name, red, yellow, green) =>
        (eid, name, red, yellow, green)
    }
}
import SparkConfig.sc.sqlContext.implicits._
val rDf = result.toDF("eid", "name", "red", "yellow", "green");
rDf.show()

Let me start with out-of-the-box solution I would use if I were and after that we'll discuss your code. I assume df is equivalent to the following structure:

val df = Seq(
  (1, "John", "S1", "green"), (2, "Shaun", "S2", "red"),
  (3, "Shaun", "S2", "green"), (4, "Shaun", "S2", "green"),
  (5, "John", "S1", "yellow")
).toDF("id", "name", "eid", "color")

All you really need to achieve desired output is pivot:

df.groupBy("name", "eid").pivot("color").count().na.fill(0).show

// +-----+---+-----+---+------+
// | name|eid|green|red|yellow|
// +-----+---+-----+---+------+
// |Shaun| S2|    2|  1|     0|
// | John| S1|    1|  0|     1|
// +-----+---+-----+---+------+

About your code:

Don't fetch more data from a DataFrame than you really need. Once you convert DataFrame you don't longer benefit from early projections, selections and other Catalyst optimizations.

It means you have to fetch all the data from external source (like a database) or off-heap storage even if downstream processing requires only a small fraction of it. In other words project early:

df.select($"name", $"eid", $"color").rdd.map { ... }

Avoid dealing with RDD[Row]. It is like a Seq[Any] - nothing you really want in your code. Instead you can use Dataset encoders:

df.select($"name", $"eid", $"color").as[(String, String, String)].rdd.map {
  ...
}

Never use list concatenation for reduction. Since List.++ is O(N) operation and you apply it in a loop overall complexity is roughly O(N2). If you really want to group data use groupByKey.
But don't group if operation can be expressed using reduceByKey with a truly reducing (requiring roughly constant memory) function.

There many ways you can approach this for example with aggregateByKey (note that I intentionally use mutable buffer):

import scala.collection.mutable.Map

val pairs = df
  .select($"name", $"eid", $"color")
  .as[(String, String, String)]
  .rdd.map { case (name, eid, color) => ((name, eid), color) }

def seqOp(acc: Map[String, Long], x: String) = {
  acc(x) = 1L + acc.getOrElse(x, 0L)
  acc
}

def mergeOp(acc1: Map[String, Long], acc2: Map[String, Long]) = {
  acc2.foreach { case (k, v) => acc1(k) = v + acc1.getOrElse(k, 0L) }
  acc1
}

pairs
  .aggregateByKey(Map.empty[String, Long])(seqOp, mergeOp)
  .map {
    case ((name, eid), vs ) =>
      (name, eid, vs.get("red"), vs.get("green"), vs.get("yellow"))
  }.toDF("name", "eid", "red", "green", "yellow")


Fast image binarization on Android I'm working on an application that turns a raw camera image into a binary (pure black/white) image and I need this to happen as fast as possible for swift further processing. This is what my code currently looks like:

public static boolean[][] createBinaryImage( Bitmap bm )
{
    int[] pixels = new int[bm.getWidth()*bm.getHeight()];
    bm.getPixels( pixels, 0, bm.getWidth(), 0, 0, bm.getWidth(), bm.getHeight() );
    int w = bm.getWidth();

    // Calculate overall lightness of image
    long gLightness = 0;
    int lLightness;
    int c;
    for ( int x = 0; x < bm.getWidth(); x++ )
    {
        for ( int y = 0; y < bm.getHeight(); y++ )
        {
            c = pixels[x+y*w];
            lLightness = ((c&0x00FF0000 )>>16) + ((c & 0x0000FF00 )>>8) + (c&0x000000FF);
            pixels[x+y*w] = lLightness;
            gLightness += lLightness;
        }
    }
    gLightness /= bm.getWidth() * bm.getHeight();
    gLightness = gLightness * 5 / 6;

    // Extract features
    boolean[][] binaryImage = new boolean[bm.getWidth()][bm.getHeight()];

    for ( int x = 0; x < bm.getWidth(); x++ )
        for ( int y = 0; y < bm.getHeight(); y++ )
            binaryImage[x][y] = pixels[x+y*w] <= gLightness;

    return binaryImage;
}

As you can see, it uses a global threshold to tell apart the black and white pixels. I recently switched from bm.getPixel to bm.getPixels and this yielded a speed improvement of ~33%. Now my question is if there is anything else I can do to speed things up?

It currently takes <0.5 seconds to process 558x256 image and this is reasonable for my application, but I wonder if there's room for any other simple optimizations.

You can get rid of the nested loop, replacing this

for ( int x = 0; x < bm.getWidth(); x++ )
 {
     for ( int y = 0; y < bm.getHeight(); y++ )
     {
         c = pixels[x+y*w];
         lLightness = ((c&0x00FF0000 )>>16) + ((c & 0x0000FF00 )>>8) + (c&0x000000FF);
         pixels[x+y*w] = lLightness;
         gLightness += lLightness;
     }
 }

With this:

int size = bm.getWidth()*bm.getHeight();
for (int i = 0; i < size; i++) {
  c = pixels[i];
 // etc.
}

It will at least save one x+y*w operation per pixel.

It may be advantageous to calculate size into a separate variable as above, instead of doing it inside the loop, as in for (int i = 0; i < bm.getWidth()*bm.getHeight(); i++). The condition is checked at each round so you would end up calling the getters at each round, and who knows what's in them? The JIT compiler may be able to optimize this kind of stuff away - and probably will, if the getters are simple "return class member" statements - but at least it's not guaranteed.

More substantial improvements can be got by sacrificing perfection. Maybe you needn't take each pixel into account for calculating the threshold? Perhaps count only every 10th on each direction? Then you would probably use the nested loop again. Like this:

int width = bm.getWidth();
int height = bm.getHeight();

for ( int x = 0; x < width; x+=10; )
{
  for ( int y = 0; y < height; y+=10; )
  {
   // etc.
  }
}

gLightness /= (width / 10) * (height / 10);


Query containing array I want to get the query result that contains all entries from array and some data from the DB.
It's like the array is a DB table and by left join I connect the data that I need.

I've solved this task in a clumsy way and am asking for tips on how to rewrite this solution.
Here is my query:

        $subquery = 'Select \'' .
        implode('\' as id union Select \'', $var_array) . '\'';

        $query = "Select tmp.variable,
        fields.content is NULL as content FROM  ({$subquery}) tmp
        LEFT JOIN fields
        ON tmp.id= fields.id";

Option 1 - Use a temporary table

CREATE TEMPORARY TABLE foo (`id` INT, `variable` VARCHAR(255), PRIMARY KEY (`id`));

Insert the data into the primary table and do a normal JOIN.

Note that temporary tables are created in memory and are dropped when the connection is closed (when the php script is done).

Option 2 - Query and add to result

Assuming that $vars are key/value pairs of id/variable

$ids = join(',', array_keys($vars));
$result = $db->query("SELECT * FROM `fields` WHERE `id` IN ($ids)");

$data = [];
while ($row = $result->fetch_assoc()) {
    $variable = $vars[$row['id']];
    $data[] = $row + ['variable' => $variable];
}


Compressing a file using zip or gzip Write a Compress class defines two static methods, gzipFile(), which
  compresses a file using GZIP compression format, and zipDirectory(),
  which compresses the files (but not directories) in a directory using
  the ZIP archive and compression format. gzipFile() uses the
  GZIPOutputStream class, while zipDirectory() uses the ZipOutputStream
  and ZipEntry classes, all from java.util.zip.

I am outputting the compressed file in the same directory as the input file.
For zipDirectory() method I have recursively found out the files present in the directory(including those in the sub directories), stored them in a List<File>, and added them to the zip.
How can I improve this code?

package beg_assignment8;

import java.io.BufferedInputStream;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.OutputStream;
import java.util.ArrayList;
import java.util.List;
import java.util.Scanner;
import java.util.zip.GZIPOutputStream;
import java.util.zip.ZipOutputStream;
import java.util.zip.ZipEntry;

public class Compress {
    public static void main(String[] args) throws FileNotFoundException, IOException {
        Scanner sc = new Scanner(System.in);
        int op = sc.nextInt();
        String source = sc.next();
        switch (op) {
        case 1:
            GZIPFile(source);
            break;
        case 2:
            zipDirectory(source);
            break;
        }
    }

    public static void GZIPFile(String sourcePath) throws FileNotFoundException, IOException {
        File file = new File(sourcePath);
        String destPath = sourcePath + ".gz";

        try (
            FileOutputStream fos = new FileOutputStream(destPath);
            GZIPOutputStream gzos = new GZIPOutputStream(fos)
        ) {
            writeFileToOutputStream(file, gzos);
        }
    }

    public static void zipDirectory(String sourcePath) throws FileNotFoundException, IOException {
        File dir = new File(sourcePath);
        List<File> allFiles = getAllFiles(dir);
        String destPath = sourcePath + ".zip";

        try (
            FileOutputStream fos = new FileOutputStream(destPath);
            ZipOutputStream zos = new ZipOutputStream(fos)
        ) {
            for (File file : allFiles) {
                String zipFile =
                    file.getCanonicalPath().substring(dir.getCanonicalPath()
                            .length() + 1);
                ZipEntry ze = new ZipEntry(zipFile);
                zos.putNextEntry(ze);
                writeFileToOutputStream(file, zos);
                zos.closeEntry();
            }
        }
    }

    private static List<File> getAllFiles(File dir) {
        File[] files = dir.listFiles();
        List<File> allFiles = new ArrayList<>();
        for (File file : files) {
            if (file.isDirectory()) {
                List<File> allFilesRecur = getAllFiles(file);
                allFiles.addAll(allFilesRecur);
            } else {
                allFiles.add(file);
            }
        }
        return allFiles;
    }

    private static void writeFileToOutputStream(File file,
            OutputStream os) throws FileNotFoundException, IOException {
        try (
            FileInputStream fis = new FileInputStream(file);
            BufferedInputStream bis = new BufferedInputStream(fis)
        ) {
            byte[] buffer = new byte[1024];
            int nBytesRead;
            while ((nBytesRead = bis.read(buffer, 0, buffer.length)) >= 0) {
                os.write(buffer);
            }
        };
    }
}

nBytesRead ignored!

        int nBytesRead;
        while ((nBytesRead = bis.read(buffer, 0, buffer.length)) >= 0) {
            os.write(buffer);
        }

You forgot to use the nBytesRead variable when writing to the other stream, putting garbage data in the target file.

        int nBytesRead;
        while ((nBytesRead = bis.read(buffer, 0, buffer.length)) >= 0) {
            os.write(buffer, 0, nBytesRead);
        }

zipDirectory() throws a NPE

Your code throws a NullPointerException if the directory doesn't exists. (Caused by dir.listFiles(); returning null, failing the for loop after)

You should check if dir.exists is true, before continuing with the method, and throw a FileNotFoundException if it is false.

Possible IndexOutOfBoundsException

String zipFile = file.getCanonicalPath().substring(dir.getCanonicalPath()
                        .length() + 1);

While this line usually works correctly, it fails on unix systems if the file is symlinked to a directory with less characters, this will throw a IndexOutOfBoundsException


Storing icons statically as strings in custom class I have a class that I call "Icons"

Icons.h

#import <Foundation/Foundation.h>

@interface Icons : NSObject

+ (UIImage *) someIcon;
+ (UIImage *) someOtherIcon;

+ (NSString *)imageToNSString:(UIImage *)image;
+ (UIImage *)stringToUIImage:(NSString *)string;

@end

Icons.m

#import "Icons.h"

@implementation Icons

+ (UIImage *) someIcon {
    return [self stringToUIImage:[self someIconStr]];
}
+ (UIImage *) menuLines {
    return [self stringToUIImage:[self someOtherIconStr]];
}

+ (NSString *)imageToNSString:(UIImage *)image {
    NSData *data = UIImagePNGRepresentation(image);
    return [data base64EncodedStringWithOptions:NSDataBase64EncodingEndLineWithLineFeed];
}

+ (UIImage *)stringToUIImage:(NSString *)string {
    NSData *data = [[NSData alloc]initWithBase64EncodedString:string options:NSDataBase64DecodingIgnoreUnknownCharacters];
    return [UIImage imageWithData:data];
}

+ (NSString *) someIconStr {
    return @"iVBORw0KGgoAAAANSUhEUgAAABYAAAAoCAYAAAD6xArmAAAACXBIWXMAABYlAAAWJQFJUiTwAAAAHGlET1QAAAACAAAAAAAAABQAAAAoAAAAFAAAABQAAAB5EsHiAAAAAEVJREFUSA1iYKAimDhxYjwIU9FIBgaQgZMmTfoPwlOmTJGniuHIhlLNxaOGwiNqNEypkwlGk9RokoIUfaM5ijo5Clh9AAAAAP//ksWFvgAAAEFJREFUY5g4cWL8pEmT/oMwiM1ATTBqONbQHA2W0WDBGgJYBUdTy2iwYA0BrILDI7VMmTJFHqv3yBUEBQsIg/QDAJNpcv6v+k1ZAAAAAElFTkSuQmCC";
}

+ (NSString *) someOtherIconStr {
    return @"iVBORw0KGgoAAAANSUhEUgAAADIAAAAKCAYAAAD2Fg1xAAAACXBIWXMAABYlAAAWJQFJUiTwAAAAHGlET1QAAAACAAAAAAAAAAUAAAAoAAAABQAAAAUAAACZxAe6RgAAAGVJREFUOBFiYACCmTNn8k+ePLl+0qRJ74H4PxS/B4mB5EkFA2Ye0MHnkTwA8wiYBsmR6pEBMQ8U6rg8ARMHqSHWMwNmHtCxyMkJJTZgHgGpIdYjA2YekmNxeQIsToxHQHljoMwDAAAA//+psxowAAAAWUlEQVRjmDRp0n9iMAMRYObMmfzEmAVSQ4RxDCSZBzT0PRGWvyfGYpCaATNv8uTJ9YQ8AlJDrEcG1Dyg5edxeQYkR6wnYOoG1DxoSCIns/cgMZjjSKXpbR4A1NvIaZrhxd8AAAAASUVORK5CYII=";
}

This way, whenever I need to add an icon to a button or whatever, it's

UIButton * btn;
[btn setImage:[Icons someIcon] forControlState:UIControlStateNormal];

It doesn't seem like a big fix, but here's the main reasons I do it.

Easy To Use Same Icon Library Across Projects
No misspelled image errors
No need to create image file keys
Changing an icon quickly updates all iterations across project

 Questions

Anyone else using something similar to this?
Should I store the image strings statically as opposed to returning them via method as is now?
Any other thoughts?

I do something similar, except rather than creating my own class, I create a UIImage class category.

File names are called UIImage+Icon.h and UIImage+Icon.m.

In the .h, we have this:

@import UIKit.UIImage;

@interface UIImage (Icon)

+ (UIImage*)someIcon;
+ (UIImage*)someOtherIcon;

@end

And then put your code in the .m as normal.

Be sure to note @import UIKit.UIImage; versus the #import.  You don't need to import the entire UIKit.  @import is new in Xcode 5 and vastly speeds up compile time as it only imports the required modules.

Anyway, the advantage to this versus what you're doing is that we call our methods as such:

[UIImage someIcon];

It's clear as day that we're definitely returning a UIImage object, because we're calling a UIImage class method.

EDIT: Having just noticed that your class also includes a method which returns NSString, I still recommend the class category.  In this case, the imageToNSString method would look like this:

- (NSString*)toEncodedString;

The method doesn't take an argument because it is an instance method.  Because this is a UIImage class category, you can refer to self, which gives you the UIImage instance on which this method was called.

I personally like the UIImage class category, but another option that is still potentially better than what you're doing would be to create C-style functions.

So you'll have a .h file that would include this function declaration:

UIImage * imageFromString(NSString *str);

As well as a list of NSString * const objects that you send as arguments.  Then your .m, you've just got the logic for returning an image from a string.

The main point here is that there are two options here that don't involve a class called Icon.  The problem is, if there's a class called Icon, I kind of expect to be working with Icon objects in some way.  And I might even try to instantiate an Icon object, which makes no sense...

Ultimately though, if you're NOT going to create a UIImage class category OR use C-style functions and still work with the Icon class, since you only have class methods and your class certainly shouldn't be instantiated, I highly recommend adding this to your .h file:

+ (id)alloc __attribute__((unavailable("Icon class can not be instantiated")));
- (id)init __attribute__((unavailable("Icon class can not be instantiated")));

This will throw red errors and prevent compilation if anyone tries to instantiate the Icon class.  This is the worst option of the three I purposed in this situation, but it can be acceptable in some cases perhaps.


Throw an exception which contains a nested set of previous exceptions I have a set of domain objects which try to find a solution to a problem.

The top level object has an algorithm which splits up the problem and delegates it to lower level objects, which in turn do the same.

I want the top level algorithm to throw an exception if it did not find any solutions, giving a breakdown of possible causes.

My question is, how should(n't) this be implemented?

The way I was thinking of doing this:

The algorithms at all levels throw an exception if it fails to find a solution.

For a given level, call it the parent algorithm, it would accumulate exceptions thrown by its children, and if all attempts by its children throw an exception, the parent will itself throw an exception which contains its childrens' exceptions.

My implementation is as follows:

class ExceptionWithCauses extends \Exception {

    private $causes = array();

    public function addCause(\Exception $ex){
        $this->causes[] = $ex;
        $this->message .= "\n".$ex->getMessage();
    }

}

I'm using it like this:

function findAnOption()
{
    $noOptionsException = new ExceptionWithCauses("findAnOption(): No options found");

    $possibilities = array(1, 2, 3, 4);
    foreach ($possibilities as $possibility)
    {
        try {
            useOption($possibility);
            return $possibility;
        } catch (Exception $e) {
            $noOptionsException->addCause($e);
        }
    }
    throw $noOptionsException;
}

function useOption($x)
{
    if ($x < 5){
        throw new ExceptionWithCauses("useOption(): $x is too small");
    }
    return $x;
}

And when findAnOption() is called, a exception is thrown with the following message:

findAnOption(): No options found
useOption(): 1 is too small
useOption(): 2 is too small
useOption(): 3 is too small
useOption(): 4 is too small

I'm worried that this is bad practice. I've read that exceptions should only be used in exceptional circumstances. So with that in mind, perhaps I should only be throwing an exception at the top level. But then at that top level, how should the exception accumulate details on what was tried by lower levels? The only time that information will be needed is when the top level throws that exception, so in a way it makes sense that it's all handled by exceptions, and that I have exceptions at every level?

On the one hand, this question on Programmers SE explains why using exceptions in control-flow is an anti-pattern, but at the same time, it seems like the tidiest way of accumulating a log for why a top level exception occurred?

The other way you could do this, noting that you don't actually use the return value of useOption, is to have it return an error message or false, along the lines of:

function findAnOption()
{
    $problems = array();

    $possibilities = array(1, 2, 3, 4);
    foreach ($possibilities as $possibility)
    {
        $problem = useOption($possibility);
        if ($problem === false) return $possibility;
        $problems[] = $problem;
    }
    throw new SomeExceptionType($problems);
}

function useOption($x)
{
    if ($x < 5) {
        return "useOption(): $x is too small";
    }

    // No problems
    return false;
}

The exception approach can certainly be clearer, and I wouldn't discard it just because it's "using exceptions for control flow". Exceptions are always control flow: the question is whether they're the appropriate one for a given case.

However, I would make one change: I prefer the name AggregateException (stolen unashamedly from Microsoft's .Net API) to ExceptionWithCauses.


Time Limit Exceeded for CodeChef Easy problem - STRQ (Chef and Strings) I was working through a codechef easy problem here.

I've got to a solution, but it only passes the Subtask #1. For the other two subtasks, it shows TLE (Time Limit Exceeded - which is 1 second).

package com.codechef.solutions;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.PrintStream;
import java.util.StringTokenizer;

public class ChefAndStrings {

  private static final BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
  private static StringTokenizer tokenizer;

  private static String specialString = "";
  private static int numberOfQueries = 0;

  public static void main(String[] args) {
    PrintStream out = System.out;
    try {
      specialString = br.readLine();
      numberOfQueries = Integer.parseInt(br.readLine());

      for (int i = 0; i < numberOfQueries; i++) {
        String nextQuery = nextQuery();
        out.println(getGoodStringCounts(nextQuery));
      }
    } catch (IOException ex) {
      out.println("Exception occurred while reading input");
    }
  }

  private static String nextQuery() throws IOException {
    return br.readLine();
  }

  private static int getGoodStringCounts(String query) {
    tokenizer = new StringTokenizer(query, " ");
    char startLetter = tokenizer.nextToken().charAt(0);
    char endLetter = tokenizer.nextToken().charAt(0);
    int startIndex = Integer.parseInt(tokenizer.nextToken()) - 1;
    int endIndex = Integer.parseInt(tokenizer.nextToken()) - 1;

    int[] startLetterIndices = allIndicesGreaterThanMin(startLetter, startIndex);
    int[] endLetterIndices = allIndicesLessThanMax(endLetter, endIndex);

    int total = 0;
    int beginEndIndexFrom = 0;

    for (int i = 0; i < startLetterIndices.length; i++) {
      int index = startLetterIndices[i];
      for (int j = beginEndIndexFrom; j < endLetterIndices.length; j++) {
        if (endLetterIndices[j] > index) {
          beginEndIndexFrom = j;
          total += (endLetterIndices.length - beginEndIndexFrom);
          break;
        }
      }
    }

    return total;
  }

  private static int[] allIndicesGreaterThanMin(char letter, int startIndex) {
    int[] indices = new int[specialString.length() / 2];
    int currentIndex = 0;
    int index = specialString.indexOf(letter, startIndex);
    while (index != -1) {
      indices[currentIndex++] = index;
      startIndex = index + 1;
      index = specialString.indexOf(letter, startIndex);
    }
    int[] result = new int[currentIndex];
    System.arraycopy(indices, 0, result, 0, result.length);
    return result;
  }

  private static int[] allIndicesLessThanMax(char letter, int endIndex) {
    int[] indices = new int[specialString.length() / 2];
    int currentIndex = 0;
    int index = specialString.indexOf(letter);
    while (index != -1 && index <= endIndex) {
      indices[currentIndex++] = index;
      index = specialString.indexOf(letter, index + 1);
    }
    int[] result = new int[currentIndex];
    System.arraycopy(indices, 0, result, 0, result.length);
    return result;
  }
}

Initially I used List<Integer> to store the indices, but then moved to int[], after I got the error first time. Still it isn't accepting. Can you see some possible optimization there?

Here is an efficient algorithm that requires O(n) time for initialization and O(1) time per query.

Let's fix a pair (start letter, end letter)(there are only 12 such combinations).
Now we can precompute the following value: count(len) - the number of strings that start with the start letter, end with the end letter and are located inside the prefix of length len. Here is a pseudo that does it:

for start_letter <- letters:
    for end_letter <- letters:
        if start_letter != end_letter:
            start_letter_count = 0
            strings_count = 0
            for i <- 0 ... s.lenght() - 1:
                if s[i] == start_letter:
                    start_letters_count++
                else if s[i] == end_letter:
                    strings_count += start_letters_count
                count(i, start_letter, end_letter) = strings_count

The answer to a (left, right, start_letter, end_letter) is
count(right, start_letter, end_letter) - count(left - 1, start_letter, end_letter) - count_letter(left, right, end_letter) * count_letter(0, left - 1, start_letter), where count_letter(left, right, c) is the number of occurrences of a char c between left and right inclusively(it can be done in O(1) time with O(n) preprocessing using prefix sums).  Why does this formula work?
We can take all string that end before right. Now we need to subtract the number of strings that end before left. We also should subtract the number of such strings that start before left but end between left and right.

The time complexity is O(n + q) because we traverse the given string once for each pair of start and end letters, and then we use one formula that is computed in O(1) to answer each query.