leetcode 987. Vertical Order Traversal of a Binary Tree anyone can provide some suggestion about my answer for the questions
987. Vertical Order Traversal of a Binary Tree

  Given a binary tree, return the vertical order traversal of its nodes values.  For each node at position (X, Y), its left and right children respectively will be at positions (X-1, Y-1) and (X+1, Y-1). Running a vertical line from X = -infinity to X = +infinity, whenever the vertical line touches some nodes, we report the values of the nodes in order from top to bottom (decreasing Y coordinates). If two nodes have the same position, then the value of the node that is reported first is the value that is smaller.

  Return an list of non-empty reports in order of X coordinate.  Every report will have a list of values of nodes.

  Input: [3,9,20,null,null,15,7]

  Given binary tree [1, 2, 3, 4, 6, 5, 7]

      1
    /   \
   2     3
  / \   / \
 4   6 5   7

  return its vertical order traversal as

  Output:  [[4], [2], [1, 5, 6], [3], [7]]

class Solution:
    def verticalOrder(self, root: TreeNode) -> List[List]:
        stack = [(root, 0)]
        node_map = dict()
        while stack:
            tmp = dict()
            for _ in range(len(stack)):
                node, index = stack.pop(0)
                if node:
                    tmp[index] = tmp.get(index, []) + [node.val]
                if node.left: stack.append((node.left, index - 1))
                if node.right: stack.append((node.right, index + 1))

            tmp = {k: sorted(i) for k, i in tmp.items()}
            node_map = {k: node_map.get(k, []) + tmp.get(k, []) for k in list(tmp.keys()) + list(node_map.keys())}
        res = sorted([(index, val) for index, val in node_map.items()], key=lambda x: x[0])
        return [i for _, i in res]

Your code seems to have the right general structure, but is doing some extra work.

Comments would help people (including you in the future) understand what your code does and why (e.g the comment below explaining why -y is used).

Also, the problem says that nodes at the same x-coord are listed in order of their y-coord (top-to-bottom) and then by value, but you don't seem to keep track of the y-coord.

from collections import defaultdict

class Solution:
    def verticalOrder(self, root: TreeNode) -> List[List]:
        # the stack keeps track of nodes that are waiting to be
        # processed, along with their x and y coordinates
        stack = [(root, 0, 0)]

        # the key is x-coordinate of a tree node, the value is a list of (-y, node_value) tuples.
        # We use -y so so that when the list is sorted later, the tuples are sorted top-to-bottom
        # and then by node_value
        node_map = defaultdict(list)

        # while there are nodes left to be processed, get the next one and add it to the node_map
        while stack:
            node, x, y = stack.pop()
            node_map[x].append((-y, node.val))

            # add the node's children to the stack for processing
            if node.left:
                stack.append((node.left, x - 1, y - 1))

            if node.right:
                stack.append((node.right, x + 1, y - 1))

        result = []

        # process groups of nodes in order from smallest to largest x-coordinate
        for x_coord in sorted(node_map.keys()):

            # sort the tuples by y-coordinate and then value
            values_at_this_x = [node_value for _,node_value in sorted(node_map[x_coord])]

            result.append(values_at_this_x)

        return result


Forward values from JSON file as class attributes I have a settings file, which is loaded in once and takes the values from the settings json file and uses them as the attribute values. I was thinking of using the @property instead of overriding __getattribute__ but I feel as if there is a better, more elegant solution. Note that the application it is used in is multi-threaded.

settings.py

class SettingsMeta(type, JsonFileWrapperBase):
    _file_path = os.path.join(os.getcwd(), "..", "data", "settings.json")

    # Declarations to avoid PyCharm highlighting
    manga_save_dir, database_path, total_scrapper_threads, log_file = None, None, None, None

    _default_data = {
        "manga_save_dir": r"D:\dir",
        "database_path": r"D:\db.sqlite3",
        "log_file": os.path.join(os.getcwd(), "..", "data", "log.log"),

        "total_scrapper_threads": 1
    }

    @classmethod
    def __getattribute__(mcs, item):
        data = mcs._get_data()

        if item in data:
            return data[item]

        elif hasattr(mcs, item):
            return getattr(mcs, item)

        raise AttributeError(f"Settings has no attribute {item}")

class Settings(metaclass=SettingsMeta):
    pass

json_file_wrapper_base.py

class JsonFileWrapperBase:
    _loaded_data = None

    _file_path = ""
    _default_data = ""

    @classmethod
    def _write_default(cls):
        cls._write_data(cls._default_data)

    @classmethod
    def _write_data(cls, d):
        os.makedirs(os.path.dirname(os.path.abspath(cls._file_path)), exist_ok=True)

        try:
            with open(cls._file_path, "w") as f:
                json.dump(d, f)

        except FileNotFoundError as e:
            print(e)

    @classmethod
    def _get_data(cls):
        """
        Reads and caches the JSON file, if the file has already been cached then return the cached file, I cache
        the file to avoid users editing the file during execution and causing errors
        """

        # File has not been cached yet
        if cls._loaded_data is None:
            cls._read_file()

        return cls._loaded_data

    @classmethod
    def _read_file(cls) -> bool:
        assert cls._loaded_data is None, "_read_file should only be called once per object"

        # File doesn't exist or is not valid
        if not os.path.isfile(cls._file_path):
            cls._write_default()

        try:
            with open(cls._file_path, "r") as f:
                data = json.load(f)

                print(f"Log: Loaded {cls._file_path}")

                cls._loaded_data = data

        except FileNotFoundError as e:
            print(e)

            return False

        return True

Pathlib

Use it. It makes stuff like this:

os.path.join(os.getcwd(), "..", "data", "log.log"),

much nicer. It can also avoid this kind of thing:

"D:\dir"

namely, OS-specific directory separators. If you ever hope to have this be cross-platform, you'll want to remove your backslashes and use the libraries for path manipulation.


Determine if a string is a sequence of dictionary words This is Leetcode problem 139: "Wordbreak"

  Problem:  Given a non-empty string s and a dictionary wordDict
  containing a list of non-empty words, determine if s can be segmented
  into a space-separated sequence of one or more dictionary words.

Note:

  The same word in the dictionary may be reused multiple times in the
  segmentation. You may assume the dictionary does not contain duplicate
  words.

Example:

Input: s = "applepenapple", wordDict = ["apple", "pen"]
Output: true

Explanation: Return true because "applepenapple" can be segmented as "apple pen apple".
             Note that you are allowed to reuse a dictionary word.
my solution

class Solution:
    def wordBreak(self, s: str, wordDict: List[str]) -> bool:
        """

        """

        def prune(i):
            if i == len(s):
                return True
            elif  i >len(s):
                return False
            else:
                for word in wordDict:
                    m = len(word)
                    if s[i:i+m] == word and prune(i+m):
                        return True

                return False
        wordDict.sort(key = lambda x:-len(x))
        return prune(0)

I believe the time complexity for my solution to be O(m^n), where m is the amount of words in wordDict and n is the length of the string s. Space wise is just the recursion stack so O(n). I'm having a hard time figuring out how to optimize my solution by either using bfs/dfs or dynamic programming. What would I memoize?

"""DocStrings"""

+1 for adding a doc string to the wordBreak() method, but -1 for not having any content in it.

PEP-008

Your code diverges from the PEP-008 guidelines in several areas:

Use snake_case for functions and variables, not mixedCase.
Use a single space around operators (elif i > len(s):), not 2 spaces, and then no spaces.
Add a blank line between the inner prune() function and the outer function, as well as between the class Solution and its first method.

Use pylint or pyflakes or similar to ensure your code follows proper conventions.

Variable Names

Variable names like s are very terse, but since it is given that name in the question, is forgivable.

Variable names like wordDict are misleading, and should be avoided!  wordDict is actually a list, not a dict.  Although that name was given in the question, you should correct it to word_list, or simply words to avoid confusion.  (+1 for the type hint; it helps avoid some confusion.  But rename the parameter anyway.)

i is acceptable as a loop index, but there is no obvious loop.  idx or pos may improve clarity.

m is unacceptable.  It in no way suggests it is the length of a word.  Use a more descriptive variable name.

Dead Code

elif i > len(s):
    return False

At what point will this branch every be taken?  The only way is if prune(i+m) is called when i+m is greater than len(s).  But if m is the length of word, and s[i:i+m] == word is true, it is impossible for i+m to exceed len(s).  This branch is dead code, and can be removed.

Algorithmic Improvements

You are sorting your "dictionary" by length, so that you check the longest words first.  However, this doesn't actually guarantee any speed improvements.  At any step, if the next possible word is the shortest one, you've guaranteed it will be the last one checked.  Ditch the sort.

What you want to do is reduce the number of words from the "dictionary" you are testing at each step.  One obvious way would be to turn the "dictionary" into an actual dict, say, storing the list of words which start with a given letter:

word_dict = {
              "a": ["apple"],
              "p": ["pen"],
            }

Or more programmatically:

word_dict = defaultdict(list, ((word[:1], word) for word in wordDict))

Then at each step, instead of comparing s[i:i+len(word)] to each and every word in wordDict, you would compare to the list of words in word_dict[s[i:i+1]], which should be significantly smaller.

This method will degenerate if all the dictionary words start with the same letter (["aaaa", "aaab", "aaac", "aaad"]), but you could handle this by storing the dictionary as a tree structure instead.

Another alternative would be to separate wordDict into sets of words of different lengths:

words_by_length = { 3: { "pen" }, 5: { "apple" } }

Then, at each stage, you could do a single set containment test for each word length:

for word_length, word_list in words_by_length.items():
    if s[i:i+word_length] in words_list:
        # ...


Menu Model class using Model-View-Presenter The following code is for my Object Oriented Programming class. My program simulate the cashier's machine to order food. The code uses Model View Presenter. This class is the model.

I have an ArrayList to hold the possible order possible. The View uses it to create JButtons.

I have an ArrayList to hold data to know what is ordered.

I am wondering if I followed Model-View-Presenter correctly.
Is this class holding too much responsibility.

public class MenuModel {
    // ArrayList hold MenuItem
    private ArrayList<MenuItem> menuItems;
    // Textfile name
    private String fileName = "Menu.txt";
    // Use to read textfile
    private MenuReader menuReader;
    // Data to keep menu's price and the menu's order
    //Keep for what was ordered
    private ArrayList<MenuItem> order;
    //Listeners
    private ArrayList<ChangeListener> listeners;
    private Formatter formatter;

    /**
     *
     * @param file name
     */
    public MenuModel(String f){
        listeners = new ArrayList<ChangeListener>();
        formatter = new Formatter();
        order = new ArrayList<MenuItem>();
        fileName = f;
        menuItems = new ArrayList<MenuItem>();
        menuReader = new MenuReader(fileName, '|', menuItems);
        menuReader.read();
    }

    public void addChangeListener(ChangeListener listener){
        listeners.add(listener);
    }
    // update the menu & price
    public void add(MenuItem menuItem){
        order.add(menuItem);
        ChangeEvent event = new ChangeEvent(this);
        for(ChangeListener lister : listeners)
            lister.stateChanged(event);
    }
     /**
      *
      * @return Menu Order Size
      */
    public int size(){
        return menuOrder.size();
    }

    // clear price and menu
    public void clear(){

        order.clear();
        ChangeEvent event = new ChangeEvent(this);
        for(ChangeListener lister : listeners)
            lister.stateChanged(event);

    }
    // Text to be sent to the screen
    public String display(){
        StringBuilder sb = new StringBuilder();
        sb.append(formatter.formatHeader());
        Iterator<MenuItem> iter = getItems();
        while(iter.hasNext())
            sb.append(formatter.formatOrder(iter.next()));

        sb.append(formatter.formatFooter());
        return sb.toString();
    }
    // Possible order available
    public ArrayList<MenuItem> getMenuItem(){ return menuItems; }
    // Get what was ordered
    public ArrayList<MenuItem> getOrderList(){ return order; }
    /**
     *
     * @return String filename
     */
    public String getFileName(){ return fileName; }

    public Iterator<MenuItem> getItems(){
        return new Iterator<MenuItem>(){
            private int current = 0;
            @Override
            public boolean hasNext(){
                return current < order.size();
            }
            @Override
            public MenuItem next() {
                 return order.get(current++);
            }
            @Override
            public void remove() {
                throw new UnsupportedOperationException();
            }
        };
    }
}

I would separate the responsibility to load the file from this class.I think this class have more responsibilities of a Presenter than a model class. A good indicator of that is the method display and the method clear and seems to me that you are using the Observer pattern to inform the view that the model has changed, and this seems to be the work of a middle-man class (Presenter).

So the only 2 thing I would change is to create a class to make the load of the file, and I would use for-each instead iterator, seems to me that iterator is something used much more in C++ than in Java.

I hope this can help :)


Compare an array with a file and form groups from elements of an array I have a text file with letters (tab delimited), and a numpy array (obj) with a few letters (single row). The text file has rows with different numbers of columns. Some rows in the text file may have multiple copies of same letters (I will like to consider only a single copy of a letter in each row). Also, each letter of the numpy array obj is present in one or more rows of the text file.

Letters in the same row of the text file are assumed to be similar to each other. Imagine a similarity metric (between two letters) which can take values 1 (related), or 0 (not related). When any pair of letters are in the same row then they are assumed to have similarity metric value = 1. In the example given below, the letters j and n are in the same row (second row). Hence j and n have similarity metric value = 1.

Here is an example of the text file (you can download the file from here):

b   q   a   i   m   l   r
j   n   o   r   o
e   i   k   u   i   s

In the example, the letter o is mentioned two times in the second row, and the letter i is denoted two times in the third row. I will like to consider single copies of letters rows of the text file.

This is an example of obj:

obj = np.asarray(['a', 'e', 'i', 'o', 'u'])

I want to compare obj with rows of the text file and form clusters from elements in obj.

This is how I want to do it. Corresponding to each row of the text file, I want to have a list which denotes a cluster (In the above example we will have three clusters since the text file has three rows). For every given element of obj, I want to find rows of the text file where the element is present. Then, I will like to assign index of that element of obj to the cluster which corresponds to the row with maximum length (the lengths of rows are decided with all rows having single copies of letters).

import pandas as pd
import numpy as np

data = pd.read_csv('file.txt', sep=r'\t+', header=None, engine='python').values[:,:].astype('<U1000')
obj = np.asarray(['a', 'e', 'i', 'o', 'u'])

for i in range(data.shape[0]):
    globals()['data_row' + str(i).zfill(3)] = []
    globals()['clust' + str(i).zfill(3)] = []
    for j in range(len(obj)):
        if obj[j] in set(data[i, :]): globals()['data_row' + str(i).zfill(3)] += [j]

for i in range(len(obj)):
    globals()['obj_lst' + str(i).zfill(3)] = [0]*data.shape[0]

    for j in range(data.shape[0]):
        if i in globals()['data_row' + str(j).zfill(3)]:
            globals()['obj_lst' + str(i).zfill(3)][j] = len(globals()['data_row' + str(j).zfill(3)])

    indx_max = globals()['obj_lst' + str(i).zfill(3)].index( max(globals()['obj_lst' + str(i).zfill(3)]) )
    globals()['clust' + str(indx_max).zfill(3)] += [i]

for i in range(data.shape[0]): print globals()['clust' + str(i).zfill(3)]

>> [0]
>> [3]
>> [1, 2, 4]

The code gives me the right answer. But, in my actual work, the text file has tens of thousands of rows, and the numpy array has hundreds of thousands of elements. And, the above given code is not very fast. So, I want to know if there is a better (faster) way to implement the above functionality and aim (using Python).

I can't understand your algorithm as written, but some very general advice applies:

Use format() or template strings to format strings.
Rather than creating dynamic dictionary keys, I would create variables data_row, clust (but see naming review below), etc. and assign to indexes in these lists. That way you get rid of the global variables (which are bad for reasons discussed at great length elsewhere), you won't need to format strings all over the place, and you won't need to do the str() conversions. You should also be able to get rid of the array initialization this way, something which is a code smell in garbage collected languages.
Can there really be multiple tab characters between columns? That would be weird. If not, you might get less surprising results using a single tab as the column separator.
Naming could use some work. For example:

In general, don't use abbreviations, especially not single letter ones or ones which shorten by only one or two letters. For example, use index (or [something]_index if there are multiple indexes in the current context) rather than indx, idx, i or j.
data should be something like character_table.
I don't know what obj is, but obj gives me no information at all. Should it be vowels?


Processing random matrices I am doing my research about scheduling by using Matlab. I am a new Matlab user and I have a problem with time execution of my following code:

clc
clear all
A=8;
B=12;
C=10;
ProcessTime= [ 11 11 11 11 4 4 4 4 4 4 4 2 ]; %Converting Matrice

RandomMatriceA=zeros(A,B,C);
RandomMatriceB=zeros(A,B,C);
SumRandomMatriceB=zeros(1,A,C);
ConvertMatriceA=zeros(A,B,C);
ProcessRandomMatriceA=zeros(A,B,C);
StartProcess=zeros(A,B,C);
EndProcess=zeros(A,B,C);
StartPost=zeros(A,B,C);

for ii=1:C;
    for x=1:2:A-1;
        %make first random matrice
        [vals RandomMatriceA(x,:,ii)]=sort(rand(1,B),2); %batasan tidak boleh satu kelompok melakukan lebih dari satu aktivitas dalam satu waktu
        done=false;
        while ~done,
              %make row of second random matrice
              NewRowRandomMatriceB=randi([0 2],1,B);
              %Make sure sum all element per row in RandomMatriceB <=11
              done=sum(NewRowRandomMatriceB)<12;
        end
        RandomMatriceB(x,:,ii)=NewRowRandomMatriceB;
        %After making RandomMatriceA and RandomMatriceB, then Make New
        %Matrice
        %To know varible of new matrice which is result of combining
        %RandomMatriceA,RandomMatriceB and ProcessTime
        for y=1:B,
            ConvertMatriceA(x,y,ii)=ProcessTime(RandomMatriceA(x,y,ii));%FirstVarible:Consecutive The Number of value in all element of RandomMatriceA
        end
        ProcessRandomMatriceA(x,:,ii)=ProcessTime(RandomMatriceA(x,:,ii))+RandomMatriceB(x,:,ii);
        EndProcess(x,:,ii)=cumsum(ProcessRandomMatriceA(x,:,ii),2);%secondVaribale:to know in which column The Consecutive value RandomMatrice will be end
        StartProcess(x,:,ii)=EndProcess(x,:,ii)-(ProcessTime(RandomMatriceA(x,:,ii))-1);%ThirdVariable:to know in which column The Consecutive Value will be start
        for yy=1:B;
            N=RandomMatriceA(x,yy,ii);
            StartPost(x,N,ii)=StartProcess(x,yy,ii);
        end
    end
    for h=2:2:A;
        doneA=false;
        while ~doneA,
          [vals RandomMatriceA(h,:,ii)]=sort(rand(1,B),2);
          doneB=false;
          while ~doneB,
                 NewRowRandomMatriceB=randi([0 2],1,B);
                 doneB= sum(NewRowRandomMatriceB)<12;
          end
          RandomMatriceB(h,:,ii)=NewRowRandomMatriceB;
          for y=1:B;
              ConvertMatriceA(h,y,ii)=ProcessTime(RandomMatriceA(h,y,ii));
          end
          ProcessRandomMatriceA(h,:,ii)=ProcessTime(RandomMatriceA(h,:,ii))+RandomMatriceB(h,:,ii);
          EndProcess(h,:,ii)=cumsum(ProcessRandomMatriceA(h,:,ii),2);%secondVaribale:to know in which column The Consecutive value RandomMatrice will be end
          StartProcess(h,:,ii)=EndProcess(h,:,ii)-(ProcessTime(RandomMatriceA(h,:,ii))-1);%ThirdVariable:to know in which column The Consecutive Value will be start
          for z=1:B;
            N=RandomMatriceA(h,z,ii);
            StartPost(h,N,ii)=StartProcess(h,z,ii);
          end
           doneA =all(ismember(StartPost(h,1:4,ii),StartPost(h-1,1:4,ii)));
        end
        %arrange value in column 1 until 4 in order that it will have same
        %value
        StartPost(h,1:4,ii)=StartPost(h-1,1:4,ii);
    end
end

After using the profile viewer, I know that my problem is in this following code. This part takes too much execution time.

....
end
           doneA =all(ismember(StartPost(h,1:4,ii),StartPost(h-1,1:4,ii)));
        end
        %arrange value in column 1 until 4 in order that it will have same
        %value
        StartPost(h,1:4,ii)=StartPost(h-1,1:4,ii);
    end

How can I improve it and make it run faster?

My best guess given the information:
You are doing some expensive ismember operations. You can try to get rid of them by tracking Done in a different way.

Perhaps you can use a more efficent way to do it in the same location, or perhaps you need to move it inside the loop and do it in a very cheap way there.


Calculate all possible combinations of an array of arrays or strings I'm using code adapted from this Stack Overflow question to calculate all possible combinations from an array, which may contains strings.

For example:

[ "x", "y", "z" ] => "x y z"
[ [ "a", "b"] , "c", "d" ] => [ "a c d", "b c d" ]
[ [ "a", "b"] , ["c" "d"], "e"] => [ "a c e", "b c e", "a d e", "b d e" ]

However as the items could be strings or array I need to check for all possible combinations of array/string pairs when building the results.

This has ended in rather messy code, and I'd appreciate any hints on how to tidy it up. I've attempted to refactor it, but I guess I'm tired and missing something!

function allPossibleCases(arr) {
  if (arr.length === 0) {
    return [];
  }
  else if (arr.length === 1){
    return arr[0];
  }
  else {
    var result = [];
    var allCasesOfRest = allPossibleCases(arr.slice(1));  // recur with the rest of array

    if(typeof allCasesOfRest === 'string') {
      if(typeof arr[0] === 'string') {
          result.push(arr[0] + " " + allCasesOfRest);
      } else {
        for (var i = 0; i < arr[0].length; i++) {
            result.push(arr[0][i] + " " + allCasesOfRest);
        }
      }
    }

    else {
      for (var c in allCasesOfRest) {
        if(typeof arr[0] === 'string') {
          result.push(arr[0] + " " + allCasesOfRest[c]);
        } else {
          for (var i = 0; i < arr[0].length; i++) {
            result.push(arr[0][i] + " " + allCasesOfRest[c]);
          }
        }
      }
    }
    return result;
  }

}

You don't need the else if/else blocks at the top; it can just be if blocks, since they return and thus exit the function anyway.

if (arr.length === 0) {
  return []; // stop here
}

if (arr.length === 1){
  return arr[0]; // or here
}

// or continue with the rest of the function

As you yourself noted, normalize the input, so you don't need to be type-checking inside the loop

But I'd advice you ignore that you're dealing with strings specifically, and just worry about "combinations of stuff", and try for a generic solution.

Still, the manual array-building makes your code cryptic. I'd suggest a more functional approach, using map and reduce.

Not saying it's easier to follow at first glance, but there's less code, and it's generic:

// this produces a 2d array of combinations
function combinations(array) {
  if(!array.length) {
    return [];
  }

  // wrap non-array values
  // e.g. ['x',['y','z']] becomes [['x'],['y','z']]
  array = array.map(function (item) {
    return item instanceof Array ? item : [item];
  });

  // internal recursive function
  function combine(list) {
    var prefixes, combinations;

    if(list.length === 1) {
      return list[0];
    }

    prefixes = list[0];
    combinations = combine(list.slice(1)); // recurse

    // produce a flat list of each of the current
    // set of values prepended to each combination
    // of the remaining sets.
    return prefixes.reduce(function (memo, prefix) {
      return memo.concat(combinations.map(function (combination) {
        return [prefix].concat(combination);
      }));
    }, []);
  }

  return combine(array);
}

To convert the output of the above function to strings, just add another map call:

function allPossibleCases(arr) {
  return combinations(arr).map(function (combination) {
    return combination.join(" ");
  });
}

You get this:

[ 'x', 'y', 'z' ]                   => [ 'x y z' ]
[ [ 'a', 'b' ], 'c', 'd' ]          => [ 'a c d', 'b c d' ]
[ [ 'a', 'b' ], [ 'c', 'd' ], 'e' ] => [ 'a c e', 'a d e', 'b c e', 'b d e' ]

Note that the order of the last one is different from yours, though it contains the same combinations.


Modifying an image URL based on hash options The code I'm trying to improve modifies a URL according to the options passed in a hash:

{
  :image_aspect_ratio => "square",
  :image_size => 50
}

or

{
  :image_size => {
    :width => 50,
    :height => 60
  }
}

The code looks like this:

module X
  module Y
    class Z

def image_url
  if raw_info['picture'] && image_size_opts_passed?
    image_url_with_size_opts
  else
    raw_info['picture']
  end
end

def image_size_opts_passed?
  !!(options[:image_size] || options[:image_aspect_ratio])
end

def image_url_with_size_opts
  params_index = raw_info['picture'].index('/photo.jpg')
  if params_index
    raw_info['picture'].insert(params_index, image_params)
  else
    raw_info['picture']
  end
end

def image_params
  image_params = []
  if options[:image_size].is_a?(Integer)
    image_params << "s#{options[:image_size]}"
  elsif options[:image_size].is_a?(Hash)
    image_params << "w#{options[:image_size][:width]}" if options[:image_size][:width]
    image_params << "h#{options[:image_size][:height]}" if options[:image_size][:height]
  end
  image_params << 'c' if options[:image_aspect_ratio] == 'square'

  '/' + image_params.join('-')
end

Here's the stuff I hate about this code:

Lots of raw_info['picture'] being called, but I'm not sure if using a local variable is better than accessing a hash twice.
I'm seeing some duplication in the else branch in the image_url and image_url_with_size_opts methods but I don't know how to improve that.
This code is all inside a class which is inside a module which is inside another module, so I'm not sure if I could also memoize the image_params result.

Overall, it's reasonably tight code. You've taken steps to reduce method size, which is good.

To answer your questions specifically:

I think an instance variable would be better, because then you can avoid the else clauses you mention in 2. Those methods can then be reduced to 1 or 2 lines.
see 1. :)
If you create an image or picture object, it provides a convenient place to memoize results. I'm not sure this is that important though, as it's probably not likely you'd need to generate more than one URL for a given image in a single request.


Paginating large SQL-Results It feels like pagination is one of the most discussed questions on the web, which always ends up with some quick and dirty hacks on how to paginate. I want to paginate a large result set from an SQL query on a website where I use Spring-JDBC for querying and Displaytag for display. I thought about it for a while and eventually came up with implementing a ResultSetExtractor like this:

import java.sql.ResultSet;
import java.sql.SQLException;
import java.util.ArrayList;
import java.util.List;

import org.displaytag.pagination.PaginatedList;
import org.displaytag.properties.SortOrderEnum;
import org.springframework.dao.DataAccessException;
import org.springframework.jdbc.core.ColumnMapRowMapper;
import org.springframework.jdbc.core.ResultSetExtractor;

public class PaginatingResultMapExtractor implements
        ResultSetExtractor<PaginatedList>
{
    private final int page;
    private final int pageSize;
    private final String sortColumn;
    private final SortOrderEnum sortDirection;

    public PaginatingResultMapExtractor(int page, int pageSize,
            String sortColumn, SortOrderEnum sortDirection)
    {
        super();
        this.page = page;
        this.pageSize = pageSize;
        this.sortColumn = sortColumn;
        this.sortDirection = sortDirection;
    }

    @Override
    public PaginatedList extractData(ResultSet rs) throws SQLException,
            DataAccessException
    {
        final ColumnMapRowMapper mapper = new ColumnMapRowMapper();
        final List<Object> result = new ArrayList<Object>(
                pageSize);
        int i;
        for (i = 0; rs.next(); ++i)
        {
            if (i > (page - 1) * pageSize
                    && i < page * pageSize)
                result.add(mapper.mapRow(rs, i));
        }
        return new PaginatedListImpl<Object>(result, i,
                pageSize, page, sortColumn, sortDirection);
    }
}

PaginatedListImpl is simply stores the values that I pass in the constructor and makes them available with getter-methods, fulfilling the requirements of the PaginatedList interface by displaytag.

I can use this by simply calling:

jdbcTemplate.query("select ... from ...", new PaginatingResultMapExtractor(page, pageSize, sortColumn, sortDirection));

At least this solution is very easy to use. But after all I have to iterate the whole result set. Can this code be improved?

Pagination should be done by the database server where applicable.  There is really no point pushing data down the wire if it is not being used, so what you should think about doing, is passing in your page number and count variables to the query or preferable a stored procedure, and use SQL to select the subset of data you want. There are SQL constructs in most SQL language variants to do this.

Here is an example using MySql:

SELECT `field1`, `field2` from `mycatalog` LIMIT start, (start + cnt)

And something similar with Sql Server:

WITH #set AS (SELECT [Field1], [Field2], ROW_NUMER() OVER (ORDER BY [Field1]) AS [Index] FROM [MyDatabase])
SELECT [Field1], [Field2] FROM #set WHERE [INDEX] BETWEEN @start AND (@start + @count)


Finding Permutations in Haskell I wrote a function to find all the possible permutations of a list in Haskell. I know it can definitely be optimized, but I'm not sure how. I'm pretty sure foldl or foldl' can be used, but I'm not sure how. It gets kinda slow when the size of the argument for perms is more than 6 items, but I don't know if this is avoidable.

What could I do to improve this function, mainly to simply it, improve it stylistically, and boost performance?

perms :: [a] -> [[a]]
perms = perms' 0
  where
    perms' _ [x, y] = [[x, y], [y, x]]
    perms' c xs
      | c == length xs = []
      | otherwise = (sub_perm xs) ++ (perms' (c + 1) (shift xs))
    sub_perm (x:xs) = fmap (\a -> x:a) $ perms xs
    shift xs = (last xs):(init xs)

It's better to base recursion at the length 1 or 0. It's usually trivial and reduces the chance of making an error. In your case, the code doesn't work for lists of length 1, and this can be easily fixed by setting the base case to perms' _ [x] = [[x]].

The costly operations in your code are repeated traversals of the input list. In particular, length xs is called every time, and as lists in Haskell are lazy linked lists, it costs you O(n). You could pass the length of the list as another argument instead.

Similarly last and init are O(n). You could use splitAt to traverse the list just once, or even better, rotate the other way around, something like shift (y:ys) = ys ++ [y] where you need to traverse the list just once (for ++) and pattern matching is also somewhat safer than using partial functions such as init/head/last/..., especially if you cover all cases and use -fwarn-incomplete-patterns.  You might also consider using Seq which has O(1) costs for manipulating its ends and O(log n) splitting/merging sequences in the middle, but has higher constant factor.

Another source of inefficiencies could be the ++ in the otherwise branch, as ++ needs to traverse the whole left argument. You might again try out Seq, or constructing the result using difference lists, which eliminates this problem.

You could solve several of these problems by introducing a helper function that'd return all possible splits of an input list, something like

splits :: [a] -> [(a, [a])]

for example splits [1,2,3] = [(1, [2, 3]), (2, [1, 3]), (3, [1, 2])]. And then recursively process the second part, prepending the picked element to all sub-results.

It's good that you provided the type of the top-level function.

Also (\a -> x : a) can be abbreviated to (x :) using η-reduction.

Below is code based on the above ideas, with some more optimizations (to improve sub-list sharing), left as an exercise to analyze:

 perms :: [a] -> [[a]]
 perms = go []
   where
     go rs [] = [rs]
     go rs xs = concatMap (\(y, ys) -> go (y : rs) ys) (splits xs)

 splits :: [a] -> [(a, [a])]
 splits = go []
   where
     go ys [] = []
     go ys (x : xs) = (x, ys ++ xs) : go (x : ys) xs


Best way to write generic factory in Java I need to write a factory of generic converters to produce converters from/to MongoDB objects to/from Java objects. Here is my implementation, it doesn't look good enough to me, so I would like to see your advice.

public static <T> Converter<T> getConverter(final Class<T> clazz) {
    return new Converter<T>() {
        public DBObject convert(T t) {
            if (clazz == Manager.class) {
                BasicDBObject dbObject = new BasicDBObject();
                dbObject.append("age", ((Manager) t).getAge());
                dbObject.append("id", ((Manager) t).getId());
                return dbObject;
            }
            throw new IllegalArgumentException("Unsuported Object Type " + clazz);
        }

        public T convert(DBObject o) {
            if (clazz == Manager.class) {
                int age= Integer.valueOf((String) o.get("age"));
                int id= Integer.valueOf((String) o.get("id"));
                Manager t = new Manager(id, age);
                return (T) t;
            }
            throw new IllegalArgumentException("Unsuported Object Type " + clazz);
        }
    };
}

When you see yourself going and editing the same function again and again to extend its functionality then thats a code smell. The problem is that whenever you add a type you have to add an if condition to your converter. So the conversion could be the class responsibility. in Java 8 you can use Suppliers for that and if you don't have Java 8 then Guava is your friend

 Supplier<DBObject> convertToDBObject(){ // java 8 code
   Supplier<DBObject> supplier = ()-> {
     BasicDBObject dbObject = new BasicDBObject();
     dbObject.append("age", this.getAge());
     dbObject.append("id", this.getId());
     return dbObject;
   }
  return supplier;
 }

Or implement Guava suppliers anonymously if you running java 6 or 7.
You can add an interface for convertable things that your classes can implement.

interface DBObjectConvertable{
   Supplier<DBObject> convertToDBObject();
}

and by doing so your types will be responsible for converting to DBObject in their own ways. The code smell in your code is adding ifs to the factory.Suppliers are factories but in a declarative way, so instead of returning converted object immediately, I will return you a function that knows how to convert to MongoDB objects.


Refactoring code for more readability I've finally got all my code working as expected and it handles the data quickly.  But now I'm looking to refactor it because there is some duplicate sections that just looks like can be broken down for more efficiency and readability.  With out making this too long, here is the original:

I created functions createNumLabel and createPctLabel to basically do what is in the if statement.  So if codeID = 1,32,28,33,10 call createNumLabel else call createPctLabel.  While that doesn't crash the map no labels on the map are drawn.  Any ideas would be helpful.

    function labels(graphics) {
    globals.map.on("update-end", function () {

        var font = new esri.symbol.Font(10, esri.symbol.Font.STYLE_NORMAL, esri.symbol.Font.VARIANT_NORMAL, esri.symbol.Font.WEIGHT_BOLDER, "Arial");//create font object
        //var gl = globals.featureLayers[1].graphics;//low res
        var gl2 = globals.featureLayers[2].graphics;//high res

        console.log("Initial zoom level is :" + globals.map.getZoom());
        console.log("Scale is :" + globals.map.getScale());

        globals.map.graphics.clear();
        if(globals.map.getZoom() >=9)  {
            console.log(gl2.length);
            //globals.featureLayers[1].setVisibility(false);
            for (var i = 0; i < gl2.length; i++) {

                if (codeID == 1 || codeID == 32 || codeID == 28 || codeID == 33 || codeID == 10) {
                    //createNumLabel(graphics);
                    var g2 = globals.featureLayers[2].graphics[i];
                    var strLabel2 = g2.attributes.NAME + ":" + $.formatNumber(findFips(g2), { format: '#,###', locale: "us" });//creates string label formatted
                    var textSymbol2 = new esri.symbol.TextSymbol(strLabel2, font);

                    textSymbol2.setColor(new dojo.Color([0, 0, 0]));

                    var pt2 = g2.geometry.getExtent().getCenter();
                    var labelPointGraphic2 = new esri.Graphic(pt2, textSymbol2);
                    //add label to the map
                    globals.map.graphics.add(labelPointGraphic2);
                }//end if
                else {
                    //createPctLabel(graphics);
                    var g2 = globals.featureLayers[2].graphics[i];
                    var strLabelPct2 = g2.attributes.NAME + " : " + $.formatNumber(findFips(g2), { format: '#,###.0', locale: "us" }) + "%";
                    var textSymbol2 = new esri.symbol.TextSymbol(strLabelPct2, font);//create symbol with attribute name

                    textSymbol2.setColor(new dojo.Color([0, 0, 0]));//set the color

                    var pt2 = g2.geometry.getExtent().getCenter(); //get center of county
                    var labelPointGraphic2 = new esri.Graphic(pt2, textSymbol2); //create label graphic

                    //add label to the map
                    globals.map.graphics.add(labelPointGraphic2);

                }//end else
            }//end for
        }//ends if
    });//ends update end
}//ends labels function

Addressing the duplication, it seems that both conditions are processed the same with the exception of format. You could either provide an inline conditional (ternary statement) to determine the format to use or pass the codeID to a function and return the respective format. At that point the if/else narrows in context and is only applied at the building of strLabel/strLabelPct2.

The function usage would be more extensible if the need for code modifications were to arise, as such, it would be my preference.

Function:

var applyFormatting = function(codeID, data) {
  if (codeID == 1 || codeID == 32 || codeID == 28 || codeID == 33 || codeID == 10) {
    return  $.formatNumber(data, { format: '#,###', locale: "us" });
  }
  return $.formatNumber(data, { format: '#,###.0', locale: "us" }) + "%";
  };

for (var i = 0; i < gl2.length; i++) {
  var g2 = globals.featureLayers[2].graphics[i];

  var affectedValue = applyFormatting(codeID, findFips(g2));

  var strLabel2 = g2.attributes.NAME + ":" + affectedValue;
  var textSymbol2 = new esri.symbol.TextSymbol(strLabel2, font);
  textSymbol2.setColor(new dojo.Color([0, 0, 0]));
  var pt2 = g2.geometry.getExtent().getCenter();
  var labelPointGraphic2 = new esri.Graphic(pt2, textSymbol2);

  //add label to the map
  globals.map.graphics.add(labelPointGraphic2);
}

Ternary:

 var codesThatAffectLabel = [1, 32, 28, 33, 10];

 for (var i = 0; i < gl2.length; i++) {
  var g2 = globals.featureLayers[2].graphics[i];
  var data = findFips(g2);

  var affectedValue = (-1 != codesThatAffectLabel.indexOf(codeID))
                    ? $.formatNumber(data, { format: '#,###', locale: "us" })
                    : $.formatNumber(data, { format: '#,###.0', locale: "us" }) + "%";

  var strLabel2 = g2.attributes.NAME + ":" + affectedValue;
  var textSymbol2 = new esri.symbol.TextSymbol(strLabel2, font);
  textSymbol2.setColor(new dojo.Color([0, 0, 0]));
  var pt2 = g2.geometry.getExtent().getCenter();
  var labelPointGraphic2 = new esri.Graphic(pt2, textSymbol2);

  //add label to the map
  globals.map.graphics.add(labelPointGraphic2);
}


split-string for R7RS A split-string function seems to be missing in R7RS.

I came up with the following implementation today:

(define (split-string delim str)
  (let ((add (lambda (current output)
               (cons (list->string (reverse current)) output))))
    (let loop ((input (string->list str))
               (output #f)
               (current #f))
      (if (null? input)
          (if output
              (reverse (if current
                           (add current output)
                           output))
              '())
          (let ((char (car input))
                (input (cdr input)))
            (if (char=? char delim)
                (loop input (add current output) '())
                (loop input output (cons char current))))))))

Is there anything which can be optimized due to run time or memory usage?

This test case I have used:

(import (scheme write))

(define-syntax test
  (syntax-rules ()
    ((test function index (predicate (argument ...) result))
     (let ((value (function argument ...)))
       (let ((test-result (predicate value result)))
         (display 'function)
         (if index
             (begin
               (display " ")
               (display index)))
         (display ": ")
         (if test-result
             (display "ok")
             (begin
               (display "FAILURE")
               (newline)
               (display "  Expression: ")
               (write '(function argument ...))
               (newline)
               (display "  returns: ")
               (display value)
               (newline)
               (display "  expecting: ")
               (write result)))
         (newline)
         test-result)))
    ((test function index ((argument ...) result))
     (test function index (equal? (argument ...) result)))
    ((test function (predicate (argument ...) result))
     (test function #f (predicate (argument ...) result)))
    ((test function ((argument ...) result))
     (test function #f (equal? (argument ...) result)))))

(define-syntax test*
  (syntax-rules ()
    ((test* function definition ...)
     (let ((counter (let ((value 1))
                      (lambda ()
                        (let ((this value))
                          (set! value (+ value 1))
                          this)))))
       (and (let ((index (counter)))
              (test function index definition))
            ...)))))

(test* split-string
       ((#\space "abc def") '("abc" "def"))
       ((#\space " ") '("" ""))
       ((#\space "") '())
       ((#\space " a") '("" "a"))
       ((#\space "a ") '("a" ""))
       ((#\space "  ") '("" "" "")))

Converting strings into lists of characters, then reassembling strings, is quite wasteful of memory. The usual implementation technique for this function should be to scan for the character you're looking for, and then use substring to extract the substring.

However, as the OP correctly points out, in R7RS, string-ref should be avoided because it's O(n). The only sensible fast solutions involve string cursors (which are not portable) and string ports. The below implementation uses the latter:

(define (string-split str delim)
  (define in (open-input-string str))
  (let recur ((out (open-output-string)))
    (define c (read-char in))
    (cond ((eof-object? c)
           (list (get-output-string out)))
          ((char=? c delim)
           (cons (get-output-string out)
                 (recur (open-output-string))))
          (else
           (write-char c out)
           (recur out)))))

That's a left-unfolding (non-tail-recursive) solution. It's easy enough to adapt to a right-unfolding solution using an accumulator:

(define (string-split str delim)
  (define in (open-input-string str))
  (let loop ((rv '()) (out (open-output-string)))
    (define c (read-char in))
    (cond ((eof-object? c)
           (reverse (cons (get-output-string out) rv)))
          ((char=? c delim)
           (loop (cons (get-output-string out) rv)
                 (open-output-string)))
          (else
           (write-char c out)
           (loop rv out)))))

For reference, here's the "ideal" solution using Chibi's string cursor interface:

(import (chibi string))
(define (string-split str delim)
  (define start (string-cursor-start str))
  (let loop ((rv '())
             (cur (string-cursor-end str))
             (last (string-cursor-end str)))
    (if (string-cursor=? cur start)
        (cons (substring-cursor str cur last) rv)
        (let ((prev (string-cursor-prev str cur)))
          (if (char=? (string-cursor-ref str prev) delim)
              (loop (cons (substring-cursor str cur last) rv) prev prev)
              (loop rv prev last))))))

Notice that I iterate through the string backwards so that I can use a right-unfolding implementation (tail-recursive) that does not require reversing at the end. This is the ideal no-compromises implementation, but of course it's Chibi-specific.


Finding common elements in two arrays I just had this question in an interview.  I had to write some code to find all the common elements in two arrays.  This is the code I wrote.  I could only think of a 2-loop solution, but something tells me there must be a way to accomplish this with only 1 loop.  Any ideas?

public List<Integer> findCommonElements(int[] arr1, int[] arr2) {
    List<Integer> commonElements = new ArrayList<>();

    for (int i = 0; i < arr1.length; i++) {
        for (int j = 0; j < arr2.length; j++) {
            if (arr1[i] == arr2[j]) {
                commonElements.add(arr1[i]);
                break;
            }
        }
    }

    return commonElements;
}

There is a reason why "Data Structures and Algorithms" has the data structures part added in, especially first. The reason why is data structures should be the first thing you think about even before writing any kind of algorithm.

Lets take the specification of the code that you are given:

  Write some code to find all the common elements in two arrays.

Now first thing is first this isn't specific enough, what do you mean by "common elements", does this include repeating elements so for example [1,1,2] and [1,1,3] would that be [1,1] or just [1]? For this I'm going to assume you mean elements non repeating.

Now after we have established the specification which is

  Given two arrays of numbers find the common unique elements.

I'd say these arrays are sounding a hell of a lot like a set data structure, and this set data structure, because we want to find the intersection between these two sets. In java this would be:

public static void main(String[] args) {
    List<Integer> alist = Arrays.asList(new Integer[] {1,1,2});
    List<Integer> blist = Arrays.asList(new Integer[] {1,1,3});

    HashSet<Integer> aset =  new HashSet<>(alist);
    aset.retainAll(blist);

    System.out.println(aset);
}

It's very important to consider two things when someone gives you a spec, first think through is it clear enough, and secondly, after you have got an idea of what they want you want to strongly consider the data structure as these are structures that have been tried and tested to be efficient at specific tasks.

In the program above, let A be the size of alist and B be the size of blist the time complexity is O(A + B) as it's O(1) to add an element to a hashset which we do for each element in A, then we loop though all elements in B because of the retainAll function needs to do a contains operation on each element, and that is O(1) for a HashSet. This is much more efficient than O(AB) ~ or O(n^2).

edit:

Thanks to nullbyte for pointing out that its more efficient to make one hashset and do ratainAll for just that single set. The code above has been adjusted.


Create a structuringselement in the form of a line with a certain degree and length Within Matlab there exist a function to create a structuringselement in the form of a line.
SE = strel('line',len,deg) creates a linear structuring element that is symmetric with respect to the neighborhood center, with approximate length len and angle deg. For a couple of  morphological operations I need to filter out horizontal-, vertical and other angles of lines. Within Python's CV2 a similiar function does not exist. Therefore, I wanted to recreate this function in Python

My code:

import math
import numpy as np

# bresenham function is the accepted answer of SO's post https://stackoverflow.com/questions/23930274/list-of-coordinates-between-irregular-points-in-python
def bresenham(x0, y0, x1, y1):
   points = []
   dx = abs(x1 - x0)
   dy = abs(y1 - y0)
   x, y = x0, y0
   sx = -1 if x0 > x1 else 1
   sy = -1 if y0 > y1 else 1
   if dx > dy:
      err = dx / 2.0
      while x != x1:
         points.append((x, y))
         err -= dy
         if err < 0:
            y += sy
            err += dx
         x += sx
   else:
      err = dy / 2.0
      while y != y1:
         points.append((x, y))
         err -= dx
         if err < 0:
            x += sx
            err += dy
         y += sy
   points.append((x, y))

   return points

def strel_line(length, degrees):
   if length >= 1:
      theta = degrees * np.pi / 180
      x = round((length - 1) / 2 * np.cos(theta))
      y = -round((length - 1) / 2 * np.sin(theta))
      points = bresenham(-x, -y, x, y)
      points_x = [point[0] for point in points]
      points_y = [point[1] for point in points]
      n_rows = int(2 * max([abs(point_y) for point_y in points_y]) + 1)
      n_columns = int(2 * max([abs(point_x) for point_x in points_x]) + 1)
      strel = np.zeros((n_rows, n_columns))
      rows = ([point_y + max([abs(point_y) for point_y in points_y]) for point_y in points_y])
      columns = ([point_x + max([abs(point_x) for point_x in points_x]) for point_x in points_x])
      idx = []
      for x in zip(rows, columns):
         idx.append(np.ravel_multi_index((int(x[0]), int(x[1])), (n_rows, n_columns)))
      strel.reshape(-1)[idx] = 1

   return strel

if __name__=='__main__':
   strel = strel_line(15, 105)
   print(strel)
   print(strel.shape)

Output:

[[1. 0. 0. 0. 0.]
 [1. 0. 0. 0. 0.]
 [0. 1. 0. 0. 0.]
 [0. 1. 0. 0. 0.]
 [0. 1. 0. 0. 0.]
 [0. 1. 0. 0. 0.]
 [0. 0. 1. 0. 0.]
 [0. 0. 1. 0. 0.]
 [0. 0. 1. 0. 0.]
 [0. 0. 0. 1. 0.]
 [0. 0. 0. 1. 0.]
 [0. 0. 0. 1. 0.]
 [0. 0. 0. 1. 0.]
 [0. 0. 0. 0. 1.]
 [0. 0. 0. 0. 1.]]

Do you see any ways to write my function more efficiently?

Type hints

def bresenham(x0, y0, x1, y1):

should be, at a guess,

def bresenham(x0: float, y0: float, x1: float, y1: float) -> List[float]:

or maybe int, but you get the idea.

Type promotion

You don't need decimals here:

err = dx / 2.0

/2 will accomplish the same thing.

Termination

What happens if you call bresenham(0, 0, 9.5, 9.5)? I suspect that it will iterate infinitely and hang, due to

  while x != x1:

One way to fix this, if your algorithm permits, is - rather than making an sx dependent on the comparison of x0 and x1, simply swap x0 and x1 if they're in the wrong order. Then, rather than !=, use <, and you don't have to calculate sx.

Use a generator

For some use cases this will dramatically decrease memory consumption. Instead of building up points in memory, just yield whenever you have a point. Allow the caller to decide whether they want to cast to a list, a tuple, or just iterate over the points.

Int casting

  n_rows = int(2 * max([abs(point_y) for point_y in points_y]) + 1)

is overambitious in application of the int cast. The multiplication and addition don't need to be done in floating-point math, so you can instead do

  n_rows = 2 * int(max(abs(point_y) for point_y in points_y)) + 1

Also note that you shouldn't pass a list to max - just pass the generator directly.


A deduplicating iterator Implement an iterator (generic) which skips an element if it is equal to the previous element. e.g: AAABBCCCCD, produces ABCD.

Please suggest improvements.

import java.util.Iterator;

public class DeDupIterator implements Iterator {

E next = null;
Iterator<E> itr;

public DeDupIterator(Iterator<E> iter) {
    itr = iter;
    next = itr.next();
}

@Override
public boolean hasNext() {
    if(itr.hasNext())
    if (next != null) {
        return true;
    }
    return false;
}

@Override
public E next() {
    E item=null;
    while (itr.hasNext()) {
        item = (E) itr.next();
        if (!item.equals(next)) {
            E temp = next;
            next = item;
            return temp;
        }

    }
    next = item;
    return next;
}

@Override
public void remove() {
    itr.remove();
}
}

public boolean hasNext() {
    if(itr.hasNext())
    if (next != null) {
        return true;
    }
    return false;
}

First, there's inconsistent usage of braces for your block if statements. Second, if you are already keeping track of what is the next element to be returned by your de-dup iterator, wouldn't it be enough to just check against that?

public boolean hasNext() {
    return next != null;
}

A suggestion regarding the remove() implementation: the Javadoc API suggests that it can be called only once per call to next(). Since your implementation of next() is already quite different, you may want to re-consider whether your implementation can be as simple as calling remove() on the underlying iterator. In your example, is it expected to be removing only one 'C' or all 'C's when remove() is called?


Checking for one- or two-dimensional palindrome strings I am new to Python and I would like some comments to the following code. It is for checking if a string is a palindrome (in one or two dimensions).

string_to_check = str(input("Please enter a phrase to check if it is palindrome: "))
string_to_check = string_to_check.upper() #transform the string to uppercase
list_to_check = string_to_check.split()
print ('The string to check is: \"{}" and has {} words'.format(string_to_check , number_of_words))

#two-dimension palindrome check - with two full lists

reversed_list_to_check = list(map(lambda x:x[::-1], list_to_check[::-1]))

if list_to_check == reversed_list_to_check :
    two_dimension_palindrome = True
    print ("The string %s is a two-dimension palindrome" %string_to_check)
else :
        #one-dimension palindrome check
    string_to_check = "".join(list_to_check) #string without spaces

    if string_to_check == string_to_check[::-1]:
        one_dimension_palindrome = True
        print ('%s is a one-dimension palindrome' % string_to_check)
    else:
        print ("Your string is not a palindrome")

Is this the Pythonic way?

I made some changes, not sure this is better though. It is all a matter of opinion. This is not intended to be "the" answer.

I however find this quite interesting. The goal in my opinion should always be to write "easy-to-read" code.

i = input("Please enter a phrase to check if it is palindrome: ")
s = str(i).upper() #transform the string to uppercase
l = s.split()

num_words = len(l)
print ('The string to check is: \"{}" and has {} words'.format(s, num_words))

# Variables to test
two_d_palindrome = False
one_d_palindrome = False

# reverse lists
rev_l = list(map(lambda x:x[::-1], l[::-1]))

#two-dimension palindrome check - with two full lists
if l == rev_l:
    two_d_palindrome = True
    print ("The string %s is a two-dimension palindrome" % s)

#one-dimension palindrome check
else:
    s = "".join(l) #string without spaces
    if s== s[::-1]:
        one_d_palindrome = True
        print ('%s is a one-dimension palindrome' % s)

#no match
if not (two_d_palindrome or one_d_palindrome):
    print ("Your string is not a palindrome")


Creating a PowerPoint in vba by calling "slide" subroutines I am creating a powerpoint in VBA, but there are a lot of slides in it.

I was thinking I could create a macro that calls a lot of subroutines. Within those subroutines, the actual "slide code" would run. However, I have only created powerpoints where all of the code was in the same subroutine.

I took a shot at it and the below code creates what I want.

My question is, Public and Private variables and the use of "Dim". Would it be bad code if I put all of this in one module and use a bunch of subroutines? Or should I separate over modules? I would prefer to do that so I do not have to define each variable such as ppSlide and ppPres, but open to any suggestions. I am mainly looking for ideas on how to set this up.

Public ppApp As PowerPoint.Application
Public ppPres As PowerPoint.Presentation
Public ppSlide As PowerPoint.Slide

sub CreatePres()
Dim ppTextbox As PowerPoint.Shape
Dim slidesCount As Long
Set ppApp = New PowerPoint.Application
ppApp.Visible = True
ppApp.Activate

Set ppPres = ppApp.Presentations.Add
slidesCount = ppPres.Slides.Count
Set ppSlide = ppPres.Slides.Add(slidesCount + 1, ppLayoutTitle)
ppSlide.Shapes(1).TextFrame.TextRange.Text = "Hello world"
ppSlide.Shapes(2).TextFrame.TextRange.Text = Date
slidesCount = slidesCount + 1

Call slide2(slidesCount)

End Sub

Sub slide2(i As Integer)
'Insert CPR speeds Chart and Table --- slide 2
Set ppSlide = ppPres.Slides.Add(i+ 1, ppLayoutBlank)
ppSlide.Select
Worksheets("Sheet1").Activate
ActiveSheet.Range(Cells(1, 4), Cells(2, 9)).Copy
ppSlide.Shapes.Paste.Select
ppSlide.Shapes(1).Width = _
    (ppPres.PageSetup.SlideWidth / 2)
ppSlide.Shapes(1).Left = _
    (ppPres.PageSetup.SlideWidth / 4)
ppSlide.Shapes(1).Top = _
    4 * (ppPres.PageSetup.SlideHeight / 5)

ActiveSheet.ChartObjects("Chart1").Activate
ActiveChart.ChartArea.Copy
ppSlide.Shapes.Paste.Select
ppSlide.Shapes(2).Height = _
    3 * (ppPres.PageSetup.SlideHeight / 5)
ppSlide.Shapes(2).Width = _
    5 * (ppPres.PageSetup.SlideWidth / 7)
ppSlide.Shapes(2).Left = _
    (ppPres.PageSetup.SlideWidth / 7)
ppSlide.Shapes(2).Top = _
    (ppPres.PageSetup.SlideHeight / 10)

End sub

Public ppApp As PowerPoint.Application
Public ppPres As PowerPoint.Presentation
Public ppSlide As PowerPoint.Slide

Assuming we're looking at a standard module, this is a red flag. Anyone, anywhere, can go and do this:

Set ppPres = Nothing

Or this:

Set ppApp = New PowerPoint.Application

...or otherwise shatter other procedures' assumptions. Variables' scope should be as limited as possible. These 3 public object variables are going to bite you, one day or another.

The ppApp application is never exited, which means more memory is consumed by the system after your macro runs, than before it did. And I wouldn't be surprised that the more you run it, the more memory it consumes. Why is that?

sub CreatePres()
Dim ppTextbox As PowerPoint.Shape
Dim slidesCount As Long
Set ppApp = New PowerPoint.Application

The procedure is doing much more than create pres; it creates a whole PowerPoint application instance, a new one every time it runs.

  About object instantiation

  Whoever creates an object, is responsible for that object's lifetime. When the object is no longer needed, the scope that created the object is also the scope that cleans it up.

Surely that Application class has a Quit method, or something similar - that method should be called before the procedure exits, in all exit points (i.e. even if an error occurs at any point).

My procedure would probably look something like this:

Option Explicit

Public Sub CreatePresentation()
    With New PowerPoint.Application
        Dim pres As PowerPoint.Presentation
        Set pres = .Presentations.Add
        CreateSlides pres
        SavePresentation pres
        pres.Close
        .Quit
    End With
End Sub

The procedure creates a presentation. An implementation detail of that, is that in order to create a presentation it first creates a PowerPoint app, but the outside world couldn't care less. Nor does it care about slides, or for a file name for the presentation (I suspect you're leaving PP open so that the user can tweak as needed and save when they see fit?) - these are separate concerns, there's a separate procedure at a lower abstraction level for that.

Kudos for beginning to split your code into separate procedures! At first it's normal not to be sure where or why split, but the more you familiarise yourself with the concept of abstraction, the easier it will become.

If you can look at a chunk of code and say "this chunk does XYZ", then chances are that it actually belongs in some DoXYZ procedure. Extracting the details of how slide2 is generated was a great first step.. but what about slide1? The part that deals with slide1 is mixing abstraction levels within the procedure: why do we need to know all the details about slide1 but slide2 gets its own dedicated procedure? The number of lines of code involved has nothing to do with the decision, only what's going on does.

Keep up the good work!

Rubberduck would have picked up a few issues:

Option Explicit is not specified. That's dangerous, a mere typo and you have a bug, one that can be pretty tricky to locate.
createPres is implicitly Public. Better make that explicit.
slide2 is implicitly Public, but that one has no business being public. Should be Private
Worksheets implicitly refers to the active workbook. May or may not be intended.
Cells implicitly refers to the active worksheet. Here if you qualify the Range call with a worksheet object other than ActiveSheet, you would get a run-time error because of that.
Call statement is obsolete and can safely be omitted (along with the parentheses around the arguments).
Consider renaming parameter i. Naming is hard, but meaningful names are important. slidePosition would be a better name IMO... But does a procedure named slide2 need to know about where to insert the slide?
Consider renaming procedure slide2 so that the name starts with a verb. Procedures do something: describe it.

Beyond that, I'd say use PascalCase for procedure names and keep camelCase for local variables [and parameters], and avoid Select and Activate where possible (use the object model instead).


Modify parameter data before creating an instance of a class in PHP I have an abstract Badge class, every class extending this class should always correctly set the incoming name (string) and achievedAt (date) fields.
There might be other classes that extend the Badge class, which will get passed different types of values in their constructor (see examples below), the most important thing is that each of these classes extending the badge class should have the correct type of value for name and achievedAt
I currently have the following structure to achieve this:
<?php

namespace App\Domain\Badge;

use Carbon\Carbon;

abstract class Badge
{
    private string $name;
    private Carbon $achievedAt;

    public function __construct(string $name, Carbon $achievedAt)
    {
        $this->name = $name;
        $this->achievedAt = $achievedAt;
    }

    public function getName(): string
    {
        return $this->name;
    }

    public function getAchievedAt(): Carbon
    {
        return $this->achievedAt;
    }
}

This is the KilometerBadge, extending the abstract Badge
<?php

namespace App\Domain\Badge;

use Carbon\Carbon;

class KilometerBadge extends Badge
{
    public function __construct(int $kilometers, $activity)
    {
        $name = 'kilometer-badge-' . $kilometers;
        $achievedAt = Carbon::parse($activity['start_date']);

        parent::__construct($name, $achievedAt);
    }
}

This is the ActivityNumberBadge, which builds the name using a different parameter
<?php

namespace App\Domain\Badge;

use Carbon\Carbon;

class ActivityNumber extends Badge
{
    public function __construct(array $activities, $activity)
    {
        $name = 'kilometer-badge-' . count($activities);
        $achievedAt = Carbon::parse($activity['start_date']);

        parent::__construct($name, $achievedAt);
    }
}

I'm wondering if this is the best way to go about this? Thank you!

I don't think you're taking advantage of the library you're using. The Carbon class extends the standard DateTime class, and as per the documentation, it can handle the instantiation of a Carbon object from a string representation, an integer timestamp, or any object implementing the DateTimeInterface interface. In particular, the Carbon::parse method is really just a wrapper for the constructor, which parses its argument anyways.
Taking advantage of this built-in capability, you could define the Badge class constructor as follows.
public function __construct(string $name, $achievedAt)
{
    $this->name = $name;
    $this->achievedAt = new Carbon($achievedAt);
}

You could also set the default value of the $achievedAt parameter to be 'now', which Carbon would correctly interpret and represent as the current date and time.
public function __construct(string $name, $achievedAt = 'now')
{
    $this->name = $name;
    $this->achievedAt = new Carbon($achievedAt);
}

This has the added benefit of removing the onus of adding the date parsing boilerplate from the caller, and they can now simply pass in the date argument if and when they need to (I'm assuming it's possible for user events to trigger achievements that merit badges, thus circumventing the need to parse a date-time string in those instances).
<?php

namespace App\Domain\Badge;

use Carbon\Carbon;

class KilometerBadge extends Badge
{
    /**
     * Kilometer Badge Constructor
     *
     * @param int $kilometers Kilometers walked or something.
     * @param array $activity Associative-array containing activity info.
     */
    public function __construct(int $kilometers, array $activity)
    {
        parent::__construct('kilometer-badge-' . $kilometers, $activity['start_date']);
    }
}

The creation of the ActivityNumber class is likewise simplified.
<?php

namespace App\Domain\Badge;

use Carbon\Carbon;

class ActivityNumber extends Badge
{
    public function __construct(array $activities, $activity)
    {
        parent::__construct('kilometer-badge-' . count($activities), $activity['start_date']);
    }
}

Note that if the Carbon::parse method fails, it will throw an InvalidArgumentException, which you need to make sure you're checking for.
With that being said, if the "kilometer-badge-x" badge you're creating in both classes is the same idea being created with two different classes, you might want to refactor.
If you had multiple badge types (BlueBadge and GreenBadge, for example), then each would have its own class.
class BlueBadge extends Badge
{
    //
}

class GreenBadge extends Badge
{
    //
}

If you wanted multiple ways of constructing a single badge type, you could look into the Factory pattern. In this case, you could have a class BlueBadgeFactory in which you define two static methods, createFromKilometers and createFromActivities, both of which would return an instance of a BlueBadge object.
class BlueBadgeFactory
{
    public static function createFromKilometers(int $kilometers, string $achievedAt) : BlueBadge
    {
        //
    }

    public static function createFromActivities(array $activities, array $activity) : BlueBadge
    {
        //
    }
}

I'm not familiar with the project you're working on, but it might be helpful to create a base AbstractBadgeFactory class from which each concrete badge factory class inherits some functionality.
Now that you've represented each badge with a single, distinct class with each badge type inheriting from a single Badge superclass, you can now more simply take advantage of polymorphism (you don't have to check whether an object is an instance of the KilometerBadge or the ActivityNumber badge, for instance - pun intended).
Here's a pretty contrived example to write a function that renders an array of badges.
function renderBadges(array $badges) : void
{
    foreach ($badges as $badge)
    {
        renderBadge($badge);
    }
}

The above function simply calls the following renderBadge function, which takes a Badge argument and renders it.
function renderBadge(Badge $badge) : void
{
    $badge->render();
}

This assumes, of course, that you actually define a method render in the Badge class. By defining it as abstract, you would force each badge type to override it, thereby providing a unique implementation per badge type.
The reason this example is contrived is because you don't want a render function on your Badge objects, since it presupposes the output format. Instead, you might define a BadgeWriter superclass, some subclasses like BadgeJSONWriter and BadgeHTMLWriter for API and Browser requests, respectively, and then you would pass in the badge in question to the writer object so that each writer badge object simply contains badge information and each writer object just takes that information and simply writes it to a specific format.
I hope this helps; comments, corrections, questions, and feedback are always welcome.


Pangrams python implementation Given

  Roy wanted to increase his typing speed for programming contests. So, his friend advised him to type the sentence "The quick brown fox jumps over the lazy dog" repeatedly, because it is a pangram. (Pangrams are sentences constructed by using every letter of the alphabet at least once.)
  After typing the sentence several times, Roy became bored with it. So he started to look for other pangrams. Given a sentence s, tell Roy if it is a pangram or not. Input Format Input consists of a line containing s.

Constraints

  Length of s can be at most 103 \$(1≤|s|≤103)\$ and it may contain spaces, lower case and upper case letters. Lower case and upper case instances of a letter are considered the same.

Solution 1

from collections import defaultdict
import string

def is_pangram(astr):
    lookup = defaultdict(int)

    for char in astr:
        lookup[char.lower()] += 1
    for char in string.ascii_lowercase:
        if lookup[char] == 0:
            return False
    return True

print "pangram" if is_pangram(raw_input()) else "not pangram"

Solution 2

from collections import Counter
import string

def is_pangram(astr):
    counter = Counter(astr.lower())
    for char in string.ascii_lowercase:
        if counter[char] == 0:
            return False
    return True

print "pangram" if is_pangram(raw_input()) else "not pangram"

Which is better in terms of running time and space complexity?

Since you do not actually need the count of each character, it would be simpler to use a set and the superset operator >=:

def is_pangram(astr):
    return set(astr.lower()) >= set(string.ascii_lowercase):

Your two solutions are almost identical, though here

for char in astr:
    lookup[char.lower()] += 1

it would be faster to lowercase the whole string at once (like you already do in solution 2):

for char in astr.lower():
    lookup[char] += 1

Other than that, the only difference is defaultdict vs. Counter. The latter makes your code more elegant, but since the standard library implements Counter in pure Python, there may be a speed advantage to defaultdict.


Enum for handling network request errors in Swift I'm working on a new API written in Swift 5 and I wanted to play with the new Result. I wanted to know what you guys think about this syntax:

enum NetworkRequestError: Error {
    case hostNotAvailable
    case accountNotAvailable

    func finish<T>() -> Result<T, NetworkRequestError> {
        return .failure(self)
    }
}

public class NetworkClient: TestableAPI {
    var host: String?

    func fetch(result: @escaping (Result<(any: [Any], any1: [Any]), NetworkRequestError>) -> Void) {
        guard host != nil else {
            result(NetworkRequestError.hostNotAvailable.finish())
            return
        }
    }
}

I would not recommend this pattern. I see no compelling reason to encumber NetworkRequestError (and presumably every other Error enumeration throughout your project) with this finish cruft.

So, instead of:

enum NetworkRequestError: Error {
    case hostNotAvailable
    case accountNotAvailable

    func finish<T>() -> Result<T, NetworkRequestError> {
        return .failure(self)
    }
}

func fetch(result: @escaping (Result<(any: [Any], any1: [Any]), NetworkRequestError>) -> Void) {
    guard host != nil else {
        result(NetworkRequestError.hostNotAvailable.finish())
        return
    }

    ...
}

I’d instead suggest:

enum NetworkRequestError: Error {
    case hostNotAvailable
    case accountNotAvailable
}

func fetch(result: @escaping (Result<(any: [Any], any1: [Any]), NetworkRequestError>) -> Void) {
    guard let host = host else {
        result(.failure(.hostNotAvailable))
        return
    }

    ...
}

This is more concise, conforms to established patterns (making it easier to reason about) and it doesn’t entangle Error types and Result types.

Also you’re undoubtedly checking host because you’re going to use it later in this method, so you might as well do guard let, like above. It saves you from having to do subsequent unwrapping of the optional.

I’m assuming your question was primarily about the Error type and this finish method. I must say that I am equally uneasy about the .success associated type, namely the tuple of [Any] arrays. Perhaps this was just a placeholder in your example, but I generally find references to Any to be code smell. Often specific types or generics would be better than Any to types in one’s code. If you have a question about why you’re using a “success” type which is two arrays of Any, that might warrant its own question, showing why you did that. It, too, could probably be improved.


Keypress function conditional How can I combine these if statements to make this a little cleaner and be less repetitive?

$(document).keydown(function(e){
    if (e.keyCode == 38) {
        if($html.hasClass('client-loading') || canAnim === false) {
            return false;
        }
        var $prevProj = $('.project-current').prev('.project');
        $prevProj.click();
    }

    if (e.keyCode == 40) {
        if($html.hasClass('client-loading') || canAnim === false) {
            return false;
        }
        var $nextProj = $('.project-current').next('.project');
        $nextProj.click();
    }

    // Prevent rapid clicking
    if ( e.keyCode == 38 || e.keycode == 40 ) {
        canAnim = false;

        setTimeout(function(){
            canAnim = true;
        },2000);
    }
});

Here's another one, just for the heck of it

$(document).keydown(function (e) {
  var current;

  // return early
  if( e.keyCode !== 38 && e.keyCode !== 40 ) {
    return false;
  }

  // again, return early
  if( $html.hasClass('client-loading') || canAnim === false ) {
    return false;
  }

  current = $('.project-current');

  if( e.keyCode === 38 ) {
    current.prev('.project').click();
  }

  if( e.keyCode === 40 ) {
    current.next('.project').click();
  }

  canAnim = false;
  setTimeout(function () { canAnim = true; }, 2000);
});


Counting Neighbors (Why scipy.signal.convolve2D so fast?) Here is my Python implementation of counting neighbours of Game of Life with radius as parameter.

def neighbors_count(n2d_array, radii=1):
    assert n2d_array.ndim == 2
    row_len, col_len = n2d_array.shape
    nbrs_count = np.zeros_like(n2d_array)
    for row_idx, row_val in enumerate(n2d_array):
         for col_idx, col_val in enumerate(row_val):
             start_row = 0 if (row_idx-radii) < 0 else (row_idx-radii)
             end_row = row_len if (row_idx+radii+1) > row_len else (row_idx+radii+1)
             start_col = 0 if (col_idx-radii) < 0 else (col_idx-radii)
             end_col = row_len if (col_idx+radii+1) > row_len else (col_idx+radii+1)
             neighbor = 0
             for block_row_idx in np.arange(start_row, end_row):
                 for block_col_idx in np.arange(start_col, end_col):
                     neighbor += n2d_array[block_row_idx, block_col_idx]
             nbrs_count[row_idx, col_idx] = neighbor - n2d_array[row_idx, col_idx]
     return nbrs_count

I found out that my implementation is very slow compared to scipy.signal.convolve2d:

def neighbors_count2(n2d_array, radii=1):
    from scipy.signal import convolve2d
    diameter = 2 * radii + 1
    n2d_array = n2d_array.astype(bool)
    nbrs_count = convolve2d(n2d_array, np.ones((diameter, diameter)),
                            mode='same', boundary='fill') - n2d_array
    return nbrs_count

Here is %timeit result in my computer:

%timeit -n 10 neighbors_count(np.random.randint(2, size=(100,100)))
10 loops, best of 3: 232 ms per loop

%timeit -n 10 neighbors_count2(np.random.randint(2, size=(100,100)))
10 loops, best of 3: 963 µs per loop

How to improve/vectorize my code so it can run faster than scipy.signal.convolve2d?

You can change your algorithmic approach to improve your speed.

Currently:

look at every cell
lookup every neighbor-cell
write the number of neighbors

My proposition:
You start with a zero-ed nbrs_count array and look at every cell. If it's occupied, increase the nbrs_count of all neighboring cells (you will get a huge speedup if the array is mostly empty).

To prevent all your conditional statements, you can simply use a try: ... except: block, as suggested by @JoeWallis

Here is an implementation using my propositions:

import numpy as np

def neighbors_count(n2d_array, radii=1):
    assert n2d_array.ndim == 2

    nbrs_count = np.zeros_like(n2d_array)

    # array of adjacents cells
    adjacents = []
    for i in range(-radii, radii + 1):
        for j in range(-radii, radii + 1):
            if j != 0 or i != 0:
                adjacents.append((i,j))

    for (row_idx, col_idx), value in np.ndenumerate(n2d_array):
        if value:
            for i, j in adjacents:
                try:
                    if row_idx + i >= 0 and col_idx + j >= 0:
                        # because a negative index doesn't fail
                        nbrs_count[row_idx + i, col_idx + j] += 1
                except IndexError:
                    # We are outside the array
                    pass

    return nbrs_count

This solution is about 5 times faster than the original code (which is still way slower than scipy)


How do you name a something as approximately equal? Let's say I following have code:
private bool ErrorLogExists(LogEvent logEvent, DateTime filterStart)
{
    var startTimeFrom = logEvent.StartTime.TruncateMilliseconds();
    var startTimeTo = startTimeFrom.AddSeconds(1);

    return _existingErrorLogs.Any(x =>
        x.Name == logEvent.Name &&
        x.EventDateTime >= filterStart &&
        x.EventDateTime >= startTimeFrom &&
        x.EventDateTime <= startTimeTo);
}

The problem is that I think it is terrible.

I might just rename startTimeFrom and startTimeTo and remove :
x.EventDateTime >= filterStart &&

as a cached view is already returned and it is no longer needed there.

I am tempted to write :
public static class DoubleExtensions
{
    internal const double AcceptableTolerance = .0005;
    public static bool IsApproximately(this double thisValue, double thatValue, double tolerance = AcceptableTolerance)
    {
        return Math.Abs(thisValue - thatValue) <= tolerance;
    }
}
public static class DatetimeExtensions
{
    public static bool IsApproximately(this DateTime thisTime, DateTime thatTime, double toleranceInSeconds = DoubleExtensions.AcceptableTolerance)
    {
        var thisTimeInSeconds = thisTime.AsTotalSeconds();
        var thatTimeInSeconds = thatTime.AsTotalSeconds();

        return thisTimeInSeconds.IsApproximately(thatTimeInSeconds, toleranceInSeconds);
    }

    public static double AsTotalSeconds(this DateTime thisTime)
    {
        return (thisTime - DateTime.MinValue).TotalSeconds;
    }
}

so I could instead use
private bool ErrorLogExists(LogEvent logEvent, DateTime filterStart)
{
    return _existingErrorLogs.Any(x =>
        x.Name == logEvent.Name &&
        x.EventDateTime.IsApproximately(logEvent.StartTime));
}

Reason why I would leave AsTotalSeconds as public, is that I would use it in tests. As in test tests, I already seem to have this partly supported. If I assume that I have:
var thisTimeInSeconds = thisTime.AsTotalSeconds();
var thatTimeInSeconds = thatTime.AsTotalSeconds();
var acceptableTolerance = .0005;

I could write :
Assert.That(thisTimeInSeconds, Is.EqualTo(thatTimeInSeconds).Within(acceptableTolerance);
thisTimeInSeconds.Should().BeApproximately(thatTimeInSeconds, acceptableTolerance);

The problem that I am facing is that
x.EventDateTime.IsApproximately(logEvent.StartTime))

has arguably even worse readability. People doing my code review might say :

"WTF is this?" / "WTF is that?" (I admit, I picked names so I could say that)
Why do we need IsApproximately?
Why is that an extension method and not a class method?
Is that DateTimes responsibility to know about this?
Is the logic change necessary?
Fine, you have DateTimeExtensions, why do you need DoubleExtensions?

My question is: How would you refactor this code?

Edit:
What I have currently:
    private bool ErrorLogExists(LogEvent logEvent)
    {
        var errorLogToleranceInMilliseconds = Properties.Settings.Default.ErrorLogToleranceInMilliseconds;
        var eventDateTimeFrom = logEvent.StartTime.AddMilliseconds(-errorLogToleranceInMilliseconds);
        var eventDateTimeTo = logEvent.StartTime.AddMilliseconds(errorLogToleranceInMilliseconds);

        return _existingErrorLogs.Any(x =>
            x.Name == logEvent.Name &&
            x.EventDateTime >= eventDateTimeFrom &&
            x.EventDateTime <= eventDateTimeTo);
    }

... it's a bit better.

About your current implementation, you write:

  The problem is that I think it is terrible.

But then, you don't explain why.

Then you go ahead and write much longer code, and a long list of objections.

In other words, maybe your original implementation is not so bad after all :)

The original code checks if there is any event that happened within the same second (more or less) as the reference event in the parameter (+ an additional date filter). The alternative implementation changes this logic to ±500 millis, which is different. Regardless of the logic you choose, the first option has some important benefits:

Simple
Efficient: sets constant values before a loop, rather than recalculate in every iteration

So I suggest to stick with that. Also remember Occam's razor.


99 times drunk on Java This is a rags-to-riches attempt from this question, based on the popular 99 Bottles of Beer, by (lightly) using Java 8's stream-based processing.

Any room for improvement in terms of readability? I can only think of two concerns:

It's not immediately obvious what the placeholders for each Line value are expecting.
I need to skip() by HEADER_COUNT to trim away the... header.

public final class Bottles {

    enum Line {
        BOTTLES("%s bottle%s of beer"),
        ON_WALL_PREFIX("%s on the wall, %s."),
        ON_WALL_SUFFIX("%s, %s on the wall.");

        private final String pattern;

        private Line(String pattern) {
            this.pattern = pattern;
        }

        public String with(Object first, Object second) {
            return String.format(pattern, first, second);
        }
    }

    private static final int HEADER_COUNT = 2;
    private static final long ALCOLISM_LEVEL = 99;
    private static final String PASS_AROUND = "Take one down and pass it around";
    private static final String BUY_MORE = "Go to the store and buy some more";

    private Bottles() {
        // empty
    }

    public static void main(String[] args) {
        getLyrics(ALCOLISM_LEVEL).forEach(System.out::println);
    }

    public static List<String> getLyrics(long target) {
        return Stream.concat(LongStream.rangeClosed(0, target)
                        .map(i -> target - i)
                        .mapToObj(Bottles::toWord)
                        .flatMap(Bottles::getVerse)
                        .skip(HEADER_COUNT),
                    Stream.of(Line.ON_WALL_SUFFIX.with(BUY_MORE, toWord(target))))
                .collect(Collectors.toList());
    }

    private static String toWord(long i) {
        return Line.BOTTLES.with(i == 0 ? "no more" : i, i == 1 ? "" : "s");
    }

    private static Stream<String> getVerse(String value) {
        String last = Line.ON_WALL_PREFIX.with(value, value);
        return Stream.of(Line.ON_WALL_SUFFIX.with(PASS_AROUND, value), "",
                Character.toUpperCase(last.charAt(0)) + last.substring(1));
    }
}

In a word, I would say that your program feels… discombobulated.  I see recognizable bits and pieces of "99 Bottles of Beer", but it's not obvious how it all fits together.

Overall, I'm not sure that it is much of an improvement over the question that inspired this one.  Naturally, I'm partial to my answer, which has the advantage of putting all of the conditionals right at the point of use by using the right tools for complex string substitutions.  You can streamify the for-loop in my answer, if you like.

That said, I think there are ways to improve a stream-based implementation.

enum Line isn't doing much, and makes your code incoherent.  "%s bottle%s of beer" is not really a line — it's just a phrase — and you're calling it a "line" just to reuse your wrapper for String.format().  The formatted BOTTLES, in turn, gets substituted into ON_WALL_PREFIX and ON_WALL_SUFFIX, so they are clearly not peers.  Furthermore, you still have PASS_AROUND and BUY_MORE as constants defined elsewhere.
Defining string constants instead of just using string literals isn't necessarily better.  It just makes your eyes jump around to figure out what's going on.
Your getVerse() is weird: it doesn't produce what I would think of as a verse, delimited by the paragraph breaks.  Rather, it produces the end of one verse and the beginning of the next, unified by the fact that they have the same number of bottles.
In getLyrics(), using a stream to generate the sequence 99, 98, …, 1, 0, 99 is awkward.  You have to start with an ascending count, reverse it, skip half of a weirdly delimited "verse", and concatenate a special case at the end that kind of looks like getVerse().
If you're using streams, why bother converting the lyrics to a List?
It's easier (though not necessarily more efficient) to lowercase an entire string than to uppercase just the first letter.
ALCOHOLISM_LEVEL was misspelled.

public final class Bottles {
    private static final long ALCOHOLISM_LEVEL = 99;

    private final long n, replenishment;

    private Bottles(long stock, long replenishment) {
        this.n = stock;
        this.replenishment = replenishment;
    }

    public String toString() {
        return String.format("%s bottle%s of beer",
                             n == 0 ? "No more" : n,
                             n == 1 ? "" : "s");
    }

    private String whatToDo() {
        return n == 0 ? "Go to the store and buy some more"
                      : "Take one down and pass it around";
    }

    private static Bottles next(Bottles b) {
        return new Bottles((b.n == 0) ? b.replenishment : b.n - 1,
                           b.replenishment);
    }

    private static String getVerse(Bottles b) {
        return String.format(
            "%1$s on the wall, %2$s.\n%3$s, %4$s on the wall.",
            b,
            b.toString().toLowerCase(),
            b.whatToDo(),
            next(b).toString().toLowerCase()
        );
    }

    public static Stream<String> getLyrics(long target) {
        return Stream.iterate(new Bottles(target, target), Bottles::next)
                     .limit(target + 1)
                     .map(Bottles::getVerse);
    }

    public static void main(String[] args) {
        System.out.println(getLyrics(ALCOHOLISM_LEVEL)
                           .collect(Collectors.joining("\n\n")));
    }

}


Finding vowels, digits, whitespaces and consonants I am creating a simple app to find vowels, digits, whitespaces and consonants. the basic idea is to practice multi-threading and writing readable code.

import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import javafx.scene.control.TextField;
import javafx.application.Platform;
import javafx.scene.control.Button;
import java.util.stream.IntStream;
import javafx.fxml.Initializable;
import java.util.ResourceBundle;
import javafx.event.ActionEvent;
import java.util.Arrays;
import javafx.fxml.FXML;
import java.util.List;
import java.net.URL;

/**
 *
  * @author Aamir khan
 */
public class FXMLDocumentController implements Initializable {

@FXML
TextField inputField;
@FXML
TextField vowelsField;
@FXML
TextField consField;
@FXML
TextField digitField;
@FXML
TextField spaceField;
@FXML
Button check;
@FXML
Button exit;

private List<Runnable> tasks;

@Override
public void initialize(URL url, ResourceBundle rb) {
    tasks = Arrays.asList(
            () -> findAndUpdateVowels(),
            () -> findAndUpdateCons(),
            () -> findAndUpdateDigits(),
            () -> findAndUpdateWhiteSpace()
    );

    exit.setOnAction(e -> System.exit(0));
}

@FXML
void checkBtnHandle(ActionEvent e) {
    ExecutorService thread = Executors.newFixedThreadPool(tasks.size());
    tasks.forEach(thread::submit);
    thread.shutdown();
}

public void findAndUpdateVowels() {
    long numberOfVowels = find(Find.VOWELS);
    updateField(vowelsField, numberOfVowels);
}

void findAndUpdateWhiteSpace() {
    long numberOfWhiteSpaces = find(Find.WHITE_SPACE);
    updateField(spaceField, numberOfWhiteSpaces);
}

void findAndUpdateCons() {
    long numberOfCons = find(Find.CONSONANTS);
    updateField(consField, numberOfCons);
}

void findAndUpdateDigits() {
    long numberOfDigits = find(Find.DIGITS);
    updateField(digitField, numberOfDigits);

}

public boolean isVowel(int codePoint) {
    //checking for lower case because the input will only in lower case
    return (codePoint == 'a')
            || (codePoint == 'e')
            || (codePoint == 'i')
            || (codePoint == 'o')
            || (codePoint == 'u');

}

private void updateField(TextField field, long val) {
    Platform.runLater(() -> field.setText(String.valueOf(val)));
}

private long find(Find whatToFind) {
    final IntStream chars = inputField.getText().toLowerCase().chars();
    long result = 0L;
    switch (whatToFind) {
        case VOWELS:
            result = chars.filter(this::isVowel).count();
            break;
        case CONSONANTS:
            //Note: we are not preventing any special chars such as œ,Ž,Ÿ
            result =  chars.filter(c -> Character.isLetter(c) && !isVowel(c)).count();
            break;
        case DIGITS:
            result = chars.filter(Character::isDigit).count();
            break;
        case WHITE_SPACE:
            result = chars.filter(Character::isWhitespace).count();
            break;
        default:
            result = 0L;
    }
    return result;
}

enum Find {
    VOWELS,
    WHITE_SPACE,
    DIGITS,
    CONSONANTS;
}
}

Is code readable? How can I optimize it?

Your code contains a lot of duplication. It can be made much shorter by passing a predicate to the find method instead of an enum. When you do that, the whole enum Find will become unused. After that, you should rename the find method to count, so that the name matches what it is actually doing.

Your way of organizing the imports looks unusual and funny. But in the rare case that someone really looks at them (like in this code review) it makes more sense to sort them by topic instead of by length. But if it's your point that these lines are usually not looked at, it's fine.

I would make the code as simple as this:

import javafx.application.Platform;
import javafx.event.ActionEvent;
import javafx.fxml.FXML;
import javafx.fxml.Initializable;
import javafx.scene.control.Button;
import javafx.scene.control.TextField;

import java.net.URL;
import java.util.Arrays;
import java.util.List;
import java.util.ResourceBundle;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.function.IntPredicate;

public class FXMLDocumentController implements Initializable {

    @FXML
    TextField inputField;
    @FXML
    TextField vowelsField;
    @FXML
    TextField consField;
    @FXML
    TextField digitField;
    @FXML
    TextField spaceField;
    @FXML
    Button check;
    @FXML
    Button exit;

    @Override
    public void initialize(URL url, ResourceBundle rb) {
        exit.setOnAction(e -> System.exit(0));
    }

    @FXML
    void checkBtnHandle(ActionEvent e) {
        List<Runnable> tasks = Arrays.asList(
                () -> updateField(vowelsField, this::isVowel),
                () -> updateField(consField, c -> Character.isLetter(c) && !isVowel(c)),
                () -> updateField(digitField, Character::isDigit),
                () -> updateField(spaceField, Character::isWhitespace)
        );

        ExecutorService executor = Executors.newFixedThreadPool(tasks.size());
        tasks.forEach(executor::submit);
        executor.shutdown();
    }

    private void updateField(TextField field, IntPredicate predicate) {
        String lowerCaseText = inputField.getText().toLowerCase();
        long count = lowerCaseText.chars().filter(predicate).count();
        Platform.runLater(() -> field.setText(String.valueOf(count)));
    }

    private boolean isVowel(int codePoint) {
        //checking for lower case because the input will only in lower case
        return codePoint == 'a'
                || codePoint == 'e'
                || codePoint == 'i'
                || codePoint == 'o'
                || codePoint == 'u';
    }
}

I moved the creation of the tasks lists into the checkBtnHandle method since I don't know at which exact instant the initialize method is called, and I wanted to avoid NullPointerExceptions.


Class to fill model objects with data from the database This is a helper class that helps fill models with data obtained from the database.

public class Util {

    public static <M> ArrayList<M> createModel(M model, Cursor cursor) throws IOException {

        try {
            final Class<?> cls = model.getClass();

            final ArrayList<M> result = new ArrayList<>();
            while (cursor.next()) {

                final Class<?> item = Class.forName(cls.getName());
                final M obj = (M) item.newInstance();

                final Field[] fields = item.getDeclaredFields();

                for (Field field : fields) {
                    field.setAccessible(true);

                    final Class<?> type = field.getType();

                    switch (type.toString()) {

                        case "int": {
                            final int value = cursor.getInt(getFieldName(field), 0);
                            field.setInt(obj, value);
                            break;
                        }

                        default:
                            //throw new IOException();
                            continue;
                    }
                }

                result.add(obj);
            }

            return result;
        } catch (IllegalAccessException | InstantiationException | ClassNotFoundException e) {
            e.printStackTrace();
            throw new IOException();
        }
    }

    private static String getFieldName(Field field) {
        final fieldName annotation = field.getDeclaredAnnotation(fieldName.class);
        return annotation == null ? field.getName() : annotation.name();
    }
}

All code here(pastebin.com)

I think my version is very bad, so I want to improve it, but I haven’t yet figured out how to do it.

Usage example:

String query = "SELECT * FROM table_name WHERE;";
Cursor cursor = helper.query(query);
List<Test> test = Util.createModel(new Test(), cursor);

Why do you think that your version in "very bad" ?

There are of course some little improvements that you can do. Like declaring List<M> instead of ArrayList<M> as the return type. Also you can pass Class<M> cls instead of an empty instance of your model.

Another think to improve the readability of your code is to split your main method into many little methods. Like one for the mapping of a class.

Many dislike the switch and specially for a long one like that. If you want you can replace it with a Map<String, BiConsumer<Cursor, Field>> that associate a type and a consumer to set the value to a field. It is usually a wrong parctive to silently ignore an edge case. You may consider to throw an exception or at least log something instead of continuing in the default case.

Instead of testing the name of the type, you can compare the types and add the support for the wrapper types.

Finally for the exception. I am not sure that IOException is the best type, you can also use a logger instead of e.printStackTrace() because this one will pollute your output without any means to control it.

if ( Integer.class.equals(type) || Integer.TYPE.equals(type) ) {
  // ...
} else if ( .. ) {
  // ...
} else {
  throw new UnsupportedTypeException("Type "+type.getName()+" cannot be mapped.");
}