Outputting all possible words which fit a string of letters I got inspired by this C# question. It asks to write a program to output all possible words (included in a dictionary) which fit a string of letters obtained by swiping the finger over the keyboard, as is often done with mobile keyboards.

  Description

  Software like Swype and SwiftKey lets smartphone users enter text by dragging their finger over the on-screen keyboard, rather than
  tapping on each letter.

  You'll be given a string of characters representing the letters the user has dragged their finger over.

  For example, if the user wants "rest", the string of input characters might be "resdft" or "resert".
      Input

  Given the following input strings, find all possible output words 5 characters or longer.

qwertyuytresdftyuioknn
gijakjthoijerjidsdfnokg

  Output

  Your program should find all possible words (5+ characters) that can be derived from the strings supplied.

  Use http://norvig.com/ngrams/enable1.txt as your search dictionary.

  The order of the output words doesn't matter.

queen question
gaeing garring gathering gating geeing gieing going goring

  Notes/Hints

  Assumptions about the input strings:

  QWERTY keyboard
  Lowercase a-z only, no whitespace or punctuation
  The first and last characters of the input string will always match the first and last characters of the desired output word
  Don't assume users take the most efficient path between letters
  Every letter of the output word will appear in the input string

The function read_vocabulary reads the file and saves the words in a nested dictionary structure, where the first key is the first letter of the word and the second key the last letter of the word (this improves running time by about a factor 2 with respect to a simple set).

I saved the linked word list as dictionary.txt on my computer.

The find_words function tries to find all characters of a word (with the right first and last letter) in the pattern string and yields it if it found all.

The call to sorted in the last part is not necessary from the defined interface (any ordering is fine), but I like it better this way.

All comments are welcome, especially about improving readability of the code.

import string
from collections import defaultdict

def read_vocabulary(file_name):
    vocabulary = {letter: defaultdict(set) for letter in string.lowercase}
    with open(file_name) as dict_file:
        for word in dict_file:
            word = word.strip().lower()
            vocabulary[word[0]][word[-1]].add(word)
    return vocabulary

def find_words(vocabulary, pattern, length=5):
    """
    Search `vocabulary` for words matching `pattern` generated by
    swiping the finger over the keyboard.

    Yields all matching words

    >>> vocabulary = {'q': {'n': {'queen'}, 'r': {'qualor'}}}
    >>> list(find_words(vocabulary, 'qwertyuytresdftyuioknn'))
    ['queen']
    """
    for word in vocabulary[pattern[0]][pattern[-1]]:
        if len(word) >= length:
            i = 1
            for character in word[1:-1]:
                try:
                    i = pattern.index(character, i)
                except ValueError:
                    break
            else:
                yield word

if __name__ == "__main__":
    words = ["qwertyuytresdftyuioknn",
             "gijakjthoijerjidsdfnokg",
             "cghhjkkllooiuytrrdfdftgyuiuytrfdsaazzseertyuioppoiuhgfcxxcfvghujiiuytrfddeews"]
    vocabulary = read_vocabulary("dictionary.txt")
    for word in words:
        # print word
        print " ".join(sorted(find_words(vocabulary, word), key=len, reverse=True))

Regarding run-time:

With the 1 million random characters, as linked at the other question, this code runs in about 0.14 seconds (as determined by python -m cProfile script.py), which includes reading the 1M characters from a file (because they are too big to just paste them in...).

I would have used patterns and pattern in main instead of words and word to avoid confusion with the words of vocabulary.


Java and stacks: correct implementation? I created this very simple stack concept's implementation.
Could you tell me if it is correct and clean? Do you see any bad coding habits?

public class MyStack
{
    private static final int MAXELEMENTS = 10;
    private int[] elements;
    private int numElements;

    public MyStack()
    {
        numElements = 0;
        elements = new int[MAXELEMENTS];
    }

    public boolean isEmpty()
    {
        return (numElements == 0);
    }

    public boolean isFull()
    {
        return (numElements == MAXELEMENTS);
    }

    public void push(int e)
    {
        if (!isFull())
            elements[numElements++] = e;
    }

    public int top()
    {
        if (!isEmpty())
            return elements[numElements - 1];
        else
            return -1;
    }

    public void pop()
    {
        if (!isEmpty())
            numElements--;
    }
}

You can use it with the following code:

class MyStackTestDrive
{
    public static void main(String[] args)
    {
        MyStack s1 = new MyStack();
        MyStack s2 = new MyStack();
        s1.push(2);
        s2.push(4);
        System.out.println(s1.top());
        System.out.println(s2.top());
    }
}

Some small notes:

According to the Code Conventions for the Java Programming Language,

  if statements always use braces {}.

Omitting them is error-prone.
top, push and pop methods handle invalid client calls too generously. You shouldn't do that. Crash early. Does it make sense to pop from an empty stack? It is rather a bug in the client code. Instead of returning -1 or swallowing the client error throw an IllegalStateException or an EmptyStackException. See: The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas: Dead Programs Tell No Lies. You could use guard clauses too:

public void push(final int e) {
    if (isFull()) {
        throw new IllegalStateException();
    }
    elements[numElements++] = e;
}

public int top() {
    if (isEmpty()) {
        throw new EmptyStackException();
    }
    return elements[numElements - 1];
}

public void pop() {
    if (isEmpty()) {
        throw new EmptyStackException();
    }
    numElements--;
}

I'd make the maximal number of elements configurable via the constructor:

private static final int DEFAULT_MAX_ELEMENTS = 10;

private final int maxElements;
private final int[] elements;
private int numElements;

public MyStack() {
    this(DEFAULT_MAX_ELEMENTS);
}

public MyStack(final int maxElements) {
    if (maxElements < 1) {
        throw new IllegalArgumentException("maxElements cannot be less than 1, was: " + maxElements);
    }
    this.maxElements = maxElements;
    elements = new int[maxElements];
}

The default value of int fields are 0, therefore numElements = 0; is unnecessary in the constructor.


Properties for Earth class I am creating instance of the same object to populate its properties twice. I need to give this object to another developer who just don't need constructor at all, as he is sending me object through WCF service.

var a = new Earth();
a.ID= 11;
a.UpdateUniverse= true;
a = new Earth(a); // use age and Taxrate to populate rest of Earth's properties
// and call dataAccess as well to get some records
ManagerEarth.Add(a);

Here is the class you want to see:

[DataContract(Name = "Earth")]
public class Earth
{
    [DataMember]
    public int ID;
    [DataMember]
    public int PlanetID;
    [DataMember]
    public string PlanetUrl;
    [DataMember]
    public string UserID;
    [DataMember]
    public string Environment;
    [DataMember]
    public string Status;

    [DataMember(Name = "StartDate")]
    public string StartDateString
    {
        get
        {
            if(this.StartDate.HasValue)
                return this.StartDate.Value.ToString(Utils.C_DateFormat);
            else
                return null;
        }
        set
        {
            this.StartDate = Utils.ToNullableDateTime(value);
        }
    }
    public DateTime? StartDate;

    [DataMember(Name = "EndDate")]
    public string EndDateString
    {
        get
        {
            if(this.EndDate.HasValue)
                return this.EndDate.Value.ToString(Utils.C_DateFormat);
            else
                return null;
        }
        set
        {
            this.EndDate = Utils.ToNullableDateTime(value);
        }
    }
    public DateTime? EndDate;

    [DataMember]
    public bool UpdateUniverse;
    [DataMember]
    public string AuditAddress;

    public Earth()
    {

    }

    public Earth(Earth m)
    {
        if(SPContext.Current == null || SPContext.Current.Web == null)
            return;

        var planetX = new EarthDataAccess().GetByURL(SPContext.Current.Web.Site.Url);
        if(planetX == null || planetX.PlanetID < 1)
            return;

        this.PlanetID = planetX.PlanetID;
        this.PlanetUrl = planetX.PlanetHomeURL;
        this.AuditAddress = planetX.GoHomeUrl;

        this.ID = m.ID;
        this.StartDate = m.StartDate;
        this.EndDate = m.EndDate;

        this.Status = !string.IsNullOrEmpty(m.Status) ? m.Status : "Active";
        if(this.Status != "Destroyed" && this.EndDate != null)
            this.Status = "Ended";

        this.Environment = !string.IsNullOrEmpty(m.Environment) ? m.Environment : "1";
        this.UserID = m.UserID;
        this.UpdateUniverse = m.UpdateUniverse;
    }
}

Some other methods:

public static string C_DateFormat = "dd/MM/yyyy HH:mm:ss";

public static DateTime? ToNullableDateTime(string dateTime)
{
    if(string.IsNullOrEmpty(dateTime))
        return (DateTime?) null;

    var ci = CultureInfo.InvariantCulture;
    var formats = new[] { "yyyy-MM-dd", "yyyy-MM-dd HH:mm:ss", "M-d-yyyy", "dd-MM-yyyy", "dd/MM/yyyy", "dd/MM/yyyy HH:mm:ss", "MM-dd-yyyy", "M.d.yyyy", "dd.MM.yyyy", "MM.dd.yyyy" }
            .Union(ci.DateTimeFormat.GetAllDateTimePatterns()).ToArray();
    var dt = DateTime.ParseExact(dateTime, formats, ci, DateTimeStyles.None);

    return dt;
}

It sounds like Earth cannot exist without an ID and the UpdateUniverse boolean flag. As such, you need a constructor that enforces this:

public Earth(int id, bool updateUniverse)
{
    // the rest of the code that went in Earth(Earth m)
}

If you still need a parameterless constructor because WCF needs it, that's fine. You can extract that constructor into its own method:

public Earth(int id, bool updateUniverse)
{
    SetId(id, updateUniverse);
}

public void SetId(int id, bool updateUniverse)
{
    // the rest of the code that went in Earth(Earth m)
}

I'm also questioning why this class needs a reference to the data access layer at all. It actually looks like you need planetX for an Earth to even exist.

What is planetX, another Earth? I'm getting lost here. Which Earth are we talking about, and how many are there? The real problem is that this class is doing too much. It is doing data access, which needs to happen in the code using the Earth objects, not inside the Earth class.

Judging by your first code block, this seems more appropriate:

Earth earth = new EarthDataAccess().GetById(11);

// now just use a fully populated Earth object

Pulling an Earth object from the database should not involve more lines of code than that.


legacy code and exception handling After some 'digging' into the dark corners of legacy code I've found class, which handles INI files. It does reading and writing to the file, but I haven't found any exception handling logic. What kind of potential problems, if any, can you detect and what is your suggestion/advise for improving the code, based on these three C++ code snippets:

Constructor

IniValue::IniValue(const char* FileName, const char* Section, const char* Entry, int sz, PersistMediumEnum medium_, int iniprofileId_)
:filename(NULL)
,section(new char[strlen(Section)+1])
,entry(new char[strlen(Entry)+1])
,buffer(new char[sz])
,bufsiz(sz)
{
  if (FileName == NULL) {
    FileName = "somefile.ini";
  }

  // 'GetFullInifileName' calls windows functions 'GetEnvironmentVariable', 'CreateDirectory', 'CopyFile', 'DeleteFile'
  std::string fullname = GetFullInifileName(FileName);

  filename = new char[fullname.length()+1]; strcpy(filename, fullname.c_str());

    strcpy(section, Section);
    strcpy(entry, Entry);
    buffer[0] = 0;
}

Some function, which reads values from the file:

void IniValue::TheGetProfileString() {
    char* defstr = new char[bufsiz];
    strncpy(defstr, buffer, bufsiz);

  IniPersistIF* inf = IniPersistIF::get(medium);

  //calls windows 'GetPrivateProfileString'
  inf->Read(section, entry, defstr, buffer, bufsiz, filename, iniprofileId);
  delete [] defstr;
}

Destructor

IniValue::~IniValue() {
  IniValue* thys = this;

  assert(section[0] != '\0');
  assert(filename[0] != '\0');

  IniPersistIF* inf = IniPersistIF::get(medium);
  assert(inf != NULL);

  assert(section[0] != '\0');
  assert(filename[0] != '\0');

  // Calls Windows 'WritePrivateProfileString' function
  inf->Write(section, entry, buffer, filename, iniprofileId, defaultval, bModified);

  delete [] buffer;
  delete [] entry;
  delete [] section;
  delete [] filename;
}

This was posted just a short while ago on StackOverflow.com

Firstly, if this is legacy code that has existed in production for a long time, then even if you can make it semantically better, you have to really weigh up the gains against the losses. Changing code is a cost, can introduce bugs even when the new code appears to be a lot better, and has to be justified. That the existing code uses a lot of arrays does not mean it has any bugs, nor would the "improvements" give you better performance in any way.

However, on the basis of giving this code a review I will do so.

Basically, start off by getting rid of all the arrays and calls to new[] and delete[] and use std::string (which is used there in one place so it is obviously known to the programmer).

Ensure your IniPersistInf class is const-correct so takes if it takes a string that it only reads, it uses const char * or const std::string & rather than char *.

buffer in IniValue appears to be for writing into, so for this one you might use std::vector<char>. In such a case, to get the pointer out of it, use &buffer[0] when you pass it to the IniPersistIf methods.


A method that generates a Comparator using lambda expression Given below is an exercise from the book *Java SE 8 for the Really Impatient( by Cay S. Horstmann:

  Write a method that generates a Comparator that can be normal or
  reversed, case-sensitive or case-insensitive, space-sensitive or
  space-insensitive, or any combination thereof. Your method should
  return a lambda expression.

Is there a better (efficient, elegant) way to do it than below?

public static final int COMPARE_REVERSED = 0x01;
public static final int COMPARE_CASE_INSENSITIVE = 0x02;
public static final int COMPARE_SPACE_INSENSITIVE = 0x04;

public static Comparator<String> getComparator(int flags) {

    return (s1, s2) -> {
        if (has(flags, COMPARE_REVERSED)) {
            String temp = s1;
            s1 = s2;
            s2 = temp;
        }
        if (has(flags, COMPARE_SPACE_INSENSITIVE)) {
            s1 = s1.replaceAll("\\s", "");
            s2 = s2.replaceAll("\\s", "");
        }
        if (has(flags, COMPARE_CASE_INSENSITIVE)) {
            return s1.compareToIgnoreCase(s2);
        }
        return s1.compareTo(s2);
    };
}

public static boolean has(int flags, int FLAG) {
    return (flags & FLAG) != 0;
}

This seems as efficient as it gets, to me.

In terms of elegance, Joshua Bloch (in Effective Java) recommends to stop using bit fields, use EnumSet instead.

The first step is to change your bit fields to an enum:

public enum Mode {
    COMPARE_REVERSED,
    COMPARE_CASE_INSENSITIVE,
    COMPARE_SPACE_INSENSITIVE
}

Next, change your method to take a Set instead of an int:

public static Comparator<String> getComparator(Set<Mode> flags) {
    return (s1, s2) -> {
        if (flags.contains(Mode.COMPARE_REVERSED)) {
            String temp = s1;
            s1 = s2;
            s2 = temp;
        }
        if (flags.contains(Mode.COMPARE_SPACE_INSENSITIVE)) {
            s1 = s1.replaceAll("\\s", "");
            s2 = s2.replaceAll("\\s", "");
        }
        if (flags.contains(Mode.COMPARE_CASE_INSENSITIVE)) {
            return s1.compareToIgnoreCase(s2);
        }
        return s1.compareTo(s2);
    };
}

You could call this method with any Set, but it will be best to call with an EnumSet, for example:

getComparator(EnumSet.of(Mode.COMPARE_CASE_INSENSITIVE, Mode.COMPARE_SPACE_INSENSITIVE));

Why is this better?

You don't need to worry about setting the bit fields correctly to be mutually exclusive
You don't need the has method anymore, now you can use simple Set.contains
You don't need the ... | ... bit operations anymore, EnumSet.of(...) is simple and clean

In short (quoted from the book):

  No more bit twiddling, EnumSet does all the hard work for you.

I would also add a convenience method for the case of no flags:

public static Comparator<String> getComparator() {
    return getComparator(Collections.<Mode>emptySet());
}

And of course, a bunch of unit tests, for example:

@Test
public void testNormalComparator() {
    Comparator<String> comparator = ComparatorGenerator.getComparator();
    assertNotEquals(0, comparator.compare("hello", "Hello"));
    assertEquals(0, comparator.compare("hello", "hello"));
}

@Test
public void testCaseInsensitiveComparator() {
    Comparator<String> comparator = ComparatorGenerator.getComparator(EnumSet.of(ComparatorGenerator.Mode.COMPARE_CASE_INSENSITIVE));
    assertNotEquals(0, comparator.compare("hello", "xHello"));
    assertEquals(0, comparator.compare("hello", "Hello"));
    assertEquals(0, comparator.compare("hello", "hello"));
}

@Test
public void testReversedComparator() {
    Comparator<String> comparator1 = ComparatorGenerator.getComparator();
    assertEquals(-1, comparator1.compare("hello1", "hello2"));
    Comparator<String> comparator2 = ComparatorGenerator.getComparator(EnumSet.of(ComparatorGenerator.Mode.COMPARE_REVERSED));
    assertEquals(1, comparator2.compare("hello1", "hello2"));
}

@Test
public void testCaseAndSpaceInsensitiveComparator() {
    Comparator<String> comparator = ComparatorGenerator.getComparator(EnumSet.of(ComparatorGenerator.Mode.COMPARE_CASE_INSENSITIVE, ComparatorGenerator.Mode.COMPARE_SPACE_INSENSITIVE));
    assertNotEquals(0, comparator.compare("hello there", "x hello there"));
    assertEquals(0, comparator.compare("HELLO there", "h e llo there"));
}


Child/Parent relationship by adding an element to list I have to implement a class with has parent and child fields. The problem is that by adding a child it must immediately keep reference to its' parent. I implemented it and it seems to work but I am not quite sure if I did it right. Any advice will be useful!

namespace DocumentToDocument
{
  public class MyEventArgs<T>: EventArgs
  {
    public T D;
  }

  public class MyList<T> : List<T>
  {
    public event EventHandler OnAdd;

    public new void Add(T item)
    {
      if (null != OnAdd)
      {
        var m = new MyEventArgs<Document> {D = item as Document};

        OnAdd(this, m);
      }
      base.Add(item);
    }
   }

   public class Document
   {
     public Document()
     {
       Children = new MyList<Document>();
       Children.OnAdd += new EventHandler(Children_OnAdd);
     }

     public string Name;

     public Document Parent;

     public MyList<Document> Children;

     public void Children_OnAdd(object sender, EventArgs e)
     {
      ((MyEventArgs<Document>) e).D.Parent = this;
     }
   }

   class Program
   {
     private static void Main(string[] args)
     {
       var d = new Document {Name = "I am a parent"};

       var dd = new Document {Name = "I am a child"};

       d.Children.Add(dd);
     }
   }
 }

Here's how I might restructure it (note, I've removed many of the generics since it seems to be tied to Document - if you do need those classes to go for other types, go back to the generics, otherwise, YAGNI):

namespace DocumentToDocument
{
    using System;
    using System.Collections.Generic;

    public sealed class MyEventArgs : EventArgs
    {
        private readonly Document document;

        public MyEventArgs(Document document)
        {
            this.document = document;
        }

        public Document Document
        {
            get
            {
                return this.document;
            }
        }
    }

    public sealed class MyList : List<Document>
    {
        public event EventHandler<MyEventArgs> OnAdd;

        public new void Add(Document item)
        {
            var onAdd = this.OnAdd;

            if (onAdd != null)
            {
                onAdd(this, new MyEventArgs(item));
            }

            base.Add(item);
        }
    }

    public sealed class Document
    {
        private readonly MyList children = new MyList();

        private readonly string name;

        private Document parent;

        public Document(string name, Document parent = null)
        {
            this.name = name;
            this.parent = parent;
            if (this.parent != null)
            {
                this.parent.Children.Add(this);
            }

            this.children.OnAdd += (sender, e) => e.Document.Parent = this;
        }

        public string Name
        {
            get
            {
                return this.name;
            }
        }

        public Document Parent
        {
            get
            {
                return this.parent;
            }

            set
            {
                this.parent = value;
            }
        }

        public MyList Children
        {
            get
            {
                return this.children;
            }
        }
    }

    internal static class Program
    {
        private static void Main()
        {
            var d = new Document("I am a parent");
            var dd = new Document("I am a child", d);
        }
    }
}


Generic predicate in Java 8 Use Case

Creating a generic and reusable predicate with closures that supports Float, Double, Integer, float, double, and integer.

Problem

Extending "Number" doesn't allow for the modulo operator.

Code

Below is the form I'd like the code to be. However, Number doesn't support the modulo function.

public <T extends Number> Predicate<T> isDivisibleBy(T divisor) {
    return (T dividend) -> dividend % divisor == 0;
}

Current Solution

The current solution I have uses method overloading, but just doesn't seem DRY. How can I improve this code?

public Predicate<Integer> isDivisibleBy(Integer divisor) {
    return dividend -> dividend % divisor == 0;
}

public Predicate<Float> isDivisibleBy(Float divisor) {
    return dividend -> dividend % divisor == 0;
}

public Predicate<Double> isDivisibleBy(Double divisor) {
    return dividend -> dividend % divisor == 0;
}

Even though Number doesn't provide any methods to check for divisibility directly, it does provide a method to make the Number into a double, and as we all know, doubles do support the % operator.

Other slight modifications:

Your method can be static
You don't need to specify the (T dividend) ->, simply using dividend -> is enough.

Final:

public static <T extends Number> Predicate<T> isDivisibleBy(T divisor) {
    return dividend -> dividend.doubleValue() % divisor.doubleValue() == 0;
}

I have tested this code for many cases using a for-loop, and it works mostly fine.

I had one problem with using Long though as the Long value = -195798407404780179L was converted into -1.95798407404780192E17, which loses some accuracy. So it seems like you need a special case for Long:

public static Predicate<Long> isDivisibleBy(Long divisor) {
    return dividend -> dividend % divisor == 0;
}

However, be aware that there are some primitive variants available for Predicate:

IntPredicate
LongPredicate
DoublePredicate


Bracket Pairs Validation Purpose

Validate whether the open brackets within a String have matching closed brackets, in the correct order.

The three types of valid brackets are

[ and ]
{ and }
( and )

Examples:

A[B]{C}(D) returns true
A[B{C}(D) returns false

Feedback

Creating a new Map of open brackets to closed brackets seems a bit expensive to do every single time - what's the best way of making a static Map (or some equivalent)?
Is the switch statement the best way to go about evaluating the cases? Is it better to use if statements instead?

Implementation

public class BracketPairsValidator {

  public static final char OPEN_SQUARE_BRACKET = '[';
  public static final char CLOSED_SQUARE_BRACKET = ']';

  public static final char OPEN_CURLY_BRACKET = '{';
  public static final char CLOSED_CURLY_BRACKET = '}';

  public static final char OPEN_PARENTHESIS = '(';
  public static final char CLOSED_PARENTHESIS = ')';

  public static boolean validBracketPairs(final String candidate) {
    final char[] chars = candidate.toCharArray();
    final Stack<Character> bracketStack = new Stack<>();
    final Map<Character, Character> bracketMap = new HashMap<>();
    bracketMap.put(OPEN_SQUARE_BRACKET, CLOSED_SQUARE_BRACKET);
    bracketMap.put(OPEN_CURLY_BRACKET, CLOSED_CURLY_BRACKET);
    bracketMap.put(OPEN_PARENTHESIS, CLOSED_PARENTHESIS);
    for (final char ch : chars) {
      switch (ch) {
        case OPEN_SQUARE_BRACKET:case OPEN_CURLY_BRACKET:case OPEN_PARENTHESIS: {
          bracketStack.add(ch);
          break;
        }

        case CLOSED_SQUARE_BRACKET:case CLOSED_CURLY_BRACKET:case CLOSED_PARENTHESIS: {
          if (bracketStack.isEmpty()) {
            return false;
          }

          if (ch != bracketMap.get(bracketStack.pop())) {
            return false;
          }
          break;
        }

        default: {
          break;
        }
      }
    }

    return bracketStack.isEmpty();
    }
}

Object scope

You should introduce object scope. Your source code gains flexibility through the possibility to to apply abstraction, polymorphy, inheritance (Open Closed Principle). Your code will be testable much more easy. Furthermore in JAVA 8 you can make use of functional interfaces. That would not be possible in a static context.

Constants

As "bracketMap" is a constant you should declare it "static final". Then you will get rid of instantiating the HashMap every time you call "validateBracketPairs".

break, multiple return (and continue)

Avoid those statements. Using them you hinder yourself to make use of applying refactorings like "extract method".

Within a loop you spread your breaking conditions all over the place. That makes the code less readable an maintainable. All breaking conditions for a loop should be at one place, the loop header/footer.

For methods that have to have a return value try to have only ONE return statement at the end.

for-loop

The for-each-loop is not the appropriate loop for this case. Consider a loop type that allows breaking conditions to have a proper place to put your go-on (breaking) conditions: As long as there are characters to process OR you not found a validation error.

switch-case

The switch case statement can be simplified through the standard if-then-else. Use the keys() and values()-method of the bracketMap to check if they contain a character. Now you are able to extends bracket pairs through putting them into the bracketMap without touching the if statement anymore. (Single Responsibility Principle).

Naming

Because bracketStack only contains open brackets it should be mentioned in the name: openBracketStack.

Code

So I would end up with something like this:

public class BracketPairsValidator implements Function<String, Boolean> {

    public static void main(String[] args) {
        System.out.println(new BracketPairsValidator().apply(""));
    }

    public static final char OPEN_SQUARE_BRACKET = '[';
    public static final char CLOSED_SQUARE_BRACKET = ']';

    public static final char OPEN_CURLY_BRACKET = '{';
    public static final char CLOSED_CURLY_BRACKET = '}';

    public static final char OPEN_PARENTHESIS = '(';
    public static final char CLOSED_PARENTHESIS = ')';

    private final static Map<Character, Character> BRACKET_MAP = createBracketMap();

    private static Map<Character, Character> createBracketMap() {
        HashMap<Character, Character> map = new HashMap<>();
        map.put(OPEN_SQUARE_BRACKET, CLOSED_SQUARE_BRACKET);
        map.put(OPEN_CURLY_BRACKET, CLOSED_CURLY_BRACKET);
        map.put(OPEN_PARENTHESIS, CLOSED_PARENTHESIS);
        return map;
    }

    @Override
    public Boolean apply(final String candidate) {

        StringCharacterIterator iterator = new StringCharacterIterator(candidate);

        final Stack<Character> openBracketStack = new Stack<>();

        boolean hasMatchingBracket = true;

        for (char ch = iterator.current(); ch != StringCharacterIterator.DONE && hasMatchingBracket; ch = iterator.next()) {

            if (isOpenBracket(ch)) {
                openBracketStack.add(ch);
            } else if (isClosedBracket(ch)) {
                hasMatchingBracket = !openBracketStack.isEmpty() && ch == BRACKET_MAP.get(openBracketStack.pop());
            }

        }

        return hasMatchingBracket && openBracketStack.isEmpty();
    }

    private boolean isClosedBracket(final char ch) {
        return BRACKET_MAP.values().contains(ch);
    }

    private boolean isOpenBracket(final char ch) {
        return BRACKET_MAP.keySet().contains(ch);
    }

}


Finding the smartest set from an array of numbers Is there any way to make this program run in better time ? While running, it is taking 1 second for the sample test case to pass and 5-10 seconds for the rest of the test cases.

Problem statement

  A smart-set is a set of distinct numbers in which all the elements have the same number of 1s in their binary form. The set of all smallest elements from each smart-set
  that can be formed from a given array of distinct positive numbers is known as the smartest-set.

  So given an array of distinct numbers, outline the elements of the smartest-set in ascending sorted order.

Example

  Let the array be {6 , 2 , 11 , 1 , 9 , 14 , 13 , 4 , 18}.

  In binary form the set is {110, 010, 1011, 0001, 1001, 1110, 1101, 0100, 10010}.

  The smart-sets are {1, 2, 4}, {6, 9, 18}, {11, 13, 14}.

  The smartest-set is {1,6,11} as each element is the smallest element from each smart-set.

Input Format

The first line of input consists of an integer t. This is the number of test cases. For each test case,
the first line of input contains an integer n. Here n is the number of elements in the array. The next line contains n space separated distinct integers which are the elements
of the array.

Output Format

The output will space separated integer elements of the smartest-set in ascending order.

Constraints

0 < t < 1000 (This is the number of test cases )
2 < n < 10000 (This is the number of integer elements of the array)
1 < Xi < 100000 (This is the size of each element of the array)

SAMPLE STDIN 1
3
9
6 2 11 1 9 14 13 4 18
3
7 3 1
3
1 2 4
SAMPLE STDOUT
1 6 11
1 3 7
1

Code

test_case=input()
for case_num in range(int(test_case)):
    num_of_elements=input()
    arr=input()
    dictn={}
    #print (num_of_elements,arr)
    for bin_values in list(map(int,arr.split())):
        count=0
        for occurence in [int(x) for x in list('{0:0b}'.format(bin_values))]:
               if occurence==1:
                 count=count+1
        dictn[bin_values]=count
    v = {}

    for key, value in sorted(dictn.items()):
        v.setdefault(value, []).append(key)

    lst=[]
    for key,value in (v.items()):
        x=min(value)
        lst.append(str(x))
    s= ' '
    s = s.join(lst)
    print (s)

Review

Don't print() but return variables, so other functions can reuse the outcome.
Split up your code into functions, for re usability and to easier test parts of your code.
Write test cases using the unittest module rather then doing it from the CLI every time
There are some code improvements as well,

You could benefit from the collections.defaultdict module
An easier way to count the "1" in the binaray format is: str(bin(ele)[2:]).count("1")
You can benefit from list or dict comprehension, see PEP202

Alternative

import unittest
from collections import defaultdict

def smartest_set(array):
    smart_sets = defaultdict(list)

    for element in array:
        num_of_ones = str(bin(element)[2:]).count("1")
        smart_sets[num_of_ones].append(element)

    # If you'd really want the output be printed, you can add a print statement in the function
    result = [min(e) for e in smart_sets.values()]
    print(result)
    return result

class Test(unittest.TestCase):
    def test_smartest_set(self):
        array = [6, 2, 11, 1, 9, 14, 13, 4, 18]
        expected = [1, 6, 11]
        self.assertEqual(expected, smartest_set(array))

        # More tests here...

if __name__ == "__main__":
    unittest.main()

Or a slightly faster approach storing the minimum value of the counts only

def smartest_set_2(array):
    smarts = {}
    for ele in array:
        num_of_ones = str(bin(ele)[2:]).count("1")
        smarts[num_of_ones] = min(ele, smarts.get(num_of_ones, float('inf'))
    return smarts.values()


DataTable to given type T simple mapper I threw this together and would love any suggestions on how to improve on this simple DataTable to given Type T mapper. Anything from coding conventions to speed optimizations or "you're doing something stupid".

public class test {

  static ISXLog log = new XLog();

  public void runTest() {
    var data = RetrieveDataSet(/*parameters not important*/);
    AddressType[] addr = mapObjects<AddressType>(data.Tables[0]);
  }

  // overload for optional function parameter
  public static T[] mapObjects<T>(DataTable dt ) where T : new(){
    return mapObjects<T>(dt, (p => p.IsCollection()));
  }

  // mapObjects from DataTable to Type T
  public static T[] mapObjects<T>(DataTable dt, Func<PropertyInfo,bool>propRestriction) where T : new() {
  T[] newobjs;
  var Rows = dt.Rows;
  newobjs = new T[Rows.Count];
  var MField = typeof(DataRowExtensions)
    .GetMethod(@"Field", new[] { typeof(DataRow), typeof(string) });

  for (int i = 0; i < Rows.Count; i++) {
    DataRow dbobj = Rows[i];
    var obj = newobjs[i] = new T();
    var objProps = obj.GetType().GetProperties();

    foreach (var prop in objProps) {
      try {
        if (!dbobj.Table.Columns.Contains(prop.Name) || propRestriction(prop)) {
          log.Debug("Nothing to set for property: {0}", prop.Name);
        }else{
          MField = MField.GetGenericMethodDefinition()
            .MakeGenericMethod(prop.PropertyType);

          var objval = MField.Invoke(null, new object[] { dbobj, prop.Name });
          prop.SetValue(obj, objval, null);
          log.Debug("Set property '{0}' to value '{1}'", prop.Name, objval);

        }
      }
      catch (Exception e) {
        log.DebugException("Error occured while trying to set prop: " + prop.Name, e);
      }
    }
  }
  return newobjs;
}

  public static class PropertyInfoExtensions
  {
    public static bool IsCollection(this PropertyInfo property) {
      return (!typeof(String).Equals(property.PropertyType) &&
        typeof(IEnumerable).IsAssignableFrom(property.PropertyType));
  }
}

You can minimize you code taking out the filtering out side of loop.
Avoid catching exception inside a loop if it needed ,(it seems like it cannot be avoided here)
No need to invoke datarow extension method to get value , you can get it directly.
Naming of variable is really bad.

This is first round of re factoring.

public static T[] MapObjects<T>(DataTable dataTable, Func<PropertyInfo, bool>
 propRestriction) where T : new()
{
var mappedObjectProperties = typeof(T).GetProperties()
                                       .Where(elem => !propRestriction(elem)).ToList();
var mappedObjectCollection = new List<T>();
foreach (var dataRow in dataTable.AsEnumerable())
{
    var mappedObject = new T();
    foreach (var mappedObjectProperty in mappedObjectProperties)
    {
        if (dataTable.Columns.Contains(mappedObjectProperty.Name))
        {
            var value = dataRow[mappedObjectProperty.Name];
            mappedObjectProperty.SetValue(mappedObject, value, null);
        }
        else
        {
            log.Debug("Nothing to set for property: {0}", prop.Name);
        }
    }
    mappedObjectCollection.Add(mappedObject);
  }
  return mappedObjectCollection.ToArray();
}


Topological sort of a graph Please refer following graph.

Please review following code for topological sort.

I can sense one odd thing in my code, TS() is calling TSUtil(), still I had to write  Visited.emplace(n.first); and Sorted.emplace(n.first); in TS().

Please suggest if I could handle it better.

test() method is called from main to test the code.

I tried to follow C++ 11 standard.

.h file

#pragma once
#include <set>
#include <stack>
#include <unordered_map>
#include <vector>

using std::set;
using std::stack;
using std::unordered_map;
using std::vector;

class TopolocialSort
{
private:
    set<char> Visited;
    stack<char> Sorted;
    unordered_map<char, vector<char>> G;
    void createGraph();
    void TS();
    void TSUtil(vector<char> childNode);
    void PrintTS();

public:
    void test();
};

.cpp

#include "TopolocialSort.h"
#include <iostream>

using std::cout;
using std::endl;

void TopolocialSort::createGraph()
{
    G['A'].push_back('E');
    G['A'].push_back('C');

    G['B'].push_back('D');
    G['B'].push_back('C');

    G['C'].push_back('\0');

    G['D'].push_back('F');

    G['E'].push_back('F');
    G['E'].push_back('H');

    G['F'].push_back('G');

    G['G'].push_back('\0');

    G['H'].push_back('\0');

}

void TopolocialSort::test()
{
    createGraph();
    TS();
    PrintTS();
}

void TopolocialSort::PrintTS()
{
    while (!Sorted.empty())
    {

        cout << Sorted.top() <<" ";
        Sorted.pop();
    }
}

void TopolocialSort::TSUtil(vector<char> childNode)
{
    for (auto n : childNode)
    {
        //will visit node only once
        if (Visited.find(n) != Visited.end())
        {
            continue;
        }

        Visited.emplace(n);
        TSUtil(G[n]);

        Sorted.emplace(n);
    }

}

void TopolocialSort::TS()
{
    for (auto n : G)
    {
        //will visit node only once
        if (Visited.find(n.first) != Visited.end())
        {
            continue;
        }

        Visited.emplace(n.first);
        TSUtil(n.second);

        Sorted.emplace(n.first);
    }
}

Small nits from my side.

You are always using for (auto foo : bar). This involves a copy of foo. If you do not alter foo then you should use for (const auto& foo : bar)
As far as i know it is more idiomatic to check for existense of an element in a set via count. So rather than Visited.find(n) != Visited.end() you can check Visited.count(n) or the in my opinion way better Visited.count(n) != 1
Your code assumes every node is unique. You might want to adopt to a more robust approach utilizing a struct node that holds the data and then wr on pointers to that node


Polyalphabetic cipher I am trying to write the simplest to undestand code possible, each function has documentation and examples and I tried to use the best style possible.

""" This programme implements a polyalphabetic cipher. """

import string

ALPHABET = string.ascii_lowercase
CHARACTERS_THAT_MUST_REMAIN_THE_SAME = string.digits + string.punctuation + string.whitespace

def cycle_get(lst,index):
    """
    If the list ends go back to the start.
    >>> cycle_get(["lorem","ipsum","dolor","sit"],8)
    "lorem"
    """
    new_index = index % len(lst)
    return(lst[new_index])

def cycle_increment_index(index,lst):
    """
    If at the end: go back to the start
    else: increment.
    >>> cycle_increment_index(0,["a","b","c"])
    1
    >>> cycle_increment_index(2,["a","b","c"])
    0
    """
    if index == len(lst) - 1:
        index = 0
    else:
        index += 1
    return(index)

def shift(letter,value):
    """
    Shifts a letter in the alphabet by the value,
    if the alphabet ends go back to the start.
    >>> shift('a',5)
    f
    >>> "".join([shift(i,20) for i in "hello"])
    'byffi'
    """
    current_letter_value = ALPHABET.find(letter)
    end_value = current_letter_value + value
    return(cycle_get(ALPHABET,end_value))

def convert_key_to_numbers(key):
    """
    Uses the alphabetic value of letters to convert a word
    to a list of numbers.
    >>> convert_key_to_numbers("abcde")
    [0,1,2,3,4]
    >>> convert_key_to_numbers("example")
    [4, 23, 0, 12, 15, 11, 4]
    """
    return([ALPHABET.find(i) for i in key])

def encrypt(text,key,reverse_operation=False):
    """
    Encrypts the text with a polyalphabetic cipher.
    >>> encrypt("lorem ipsum dolor sit amet, consectetur adipiscing elit","latine")
    'wokmz masnu qswok avx lmxb, psysxkgieuk iqmailkvrr eeqg'
    >>> encrypt("the quick brown fox jumps over the lazy dog","gvufigfwiufw")
    'zcy vcohg jltst aic rarla iaax obj tgeu lil'
    """
    text = text.lower()
    key = convert_key_to_numbers(key)
    index_of_key = 0
    result = ""
    for char in text:
        if char in CHARACTERS_THAT_MUST_REMAIN_THE_SAME:
            result += char
        else:
            if not reverse_operation:
                result += shift(char,key[index_of_key])
            else:
                result += shift(char,- key[index_of_key])
            index_of_key = cycle_increment_index(index_of_key,key)
    return(result)

def decrypt(text,key):
    """
    Decrypts the text previously encrypted with a polyalphabetic cipher.
    >>> decript('wokmz masnu qswok avx lmxb, psysxkgieuk iqmailkvrr eeqg',"latine")
    'lorem ipsum dolor sit amet, consectetur adipiscing elit'
    >>> decrypt("zcy vcohg jltst aic rarla iaax obj tgeu lil","gvufigfwiufw")
    'the quick brown fox jumps over the lazy dog'
    """
    return(encrypt(text,key,reverse_operation=True))

I am trying to write the simplest to understand code possible, each function has documentation and examples and I tried to use the best style possible.

The individual methods are simple and easy to understand.
The docstrings are especially great, they are fantastic help for the reader.

To improve further, try to zoom out. Take a step back and look at the outline of the code, like some advanced editors fold the function bodies and show only the method signatures:

def cycle_get(lst,index): ...

def cycle_increment_index(index,lst): ...

def shift(letter,value): ...

def convert_key_to_numbers(key): ...

def encrypt(text,key,reverse_operation=False): ...

def decrypt(text,key): ...

I don't know if you see it, but things look less clear at this level:

It's hard to guess what cycle_get(lst,index) and cycle_increment_index(index,lst) would do. Also, curiously, although both seem to take a list and a number parameter, the order of parameters is reversed. This will be difficult to remember for users of the class.
shift(letter,value) could be clearer: the method shifts letter by some number, but value is too generic, so it doesn't help guessing that.
convert_key_to_numbers(key) comes quite close, but it would help to be more specific and call "numbers" to "indexes" instead.
encrypt(text,key,reverse_operation=False) is clear, but the reverse_operation parameter is a bit too long. But the biggest problem is that the parameter doesn't really make sense: what is a reverse operation of encrypt? The reverse of "encrypt" should be "decrypt", which shouldn't belong in this method
decrypt(text,key) is clear :-)

I would recommend this alternative outline:

def get_circular_index(lst, index):
    return index % len(lst)

def get_circular_item(lst, index):
    return lst[get_circular_index(lst, index)]

def get_next_index(lst, index):
    return get_circular_index(lst, index + 1)

def shift_letter_by(letter, num):
    # ...

def convert_key_to_indexes(key):
    return [ALPHABET.find(i) for i in key]

def cycle_text(text, key, reverse=False):
    # ...

def encrypt(text, key):
    return cycle_text(text, key)

def decrypt(text, key):
    return cycle_text(text, key, reverse=True)

For the shorter ones I included the implementation too.
I made some other changes too:

The original cycle_get and cycle_increment_index were sharing the "index cycling logic". I moved that part of the logic in one place, a new get_circular_index function.
encrypt is just as clear now as decrypt, and these methods shouldn't call each other, but the common purpose method cycle_text.
As suggested above, the methods and method parameters are renamed to what I hope would be more natural, and the method parameters are consistently ordered.

The CHARACTERS_THAT_MUST_REMAIN_THE_SAME variable is long.
I tried to find a better name for it,
but it was too hard.
When it's too hard to name something,
it's often the sign that there's a better way.
In this case, is this variable really necessary?
It seems the intention is to put characters in it that you don't want to encrypt / decrypt.
Its purpose seems sort of the opposite of the ALPHABET variable.
But it's not really the opposite of that.
And it could be, and it would be simpler.
So instead of the condition if char in CHARACTERS_THAT_MUST_REMAIN_THE_SAME: ...,
how about using if char not in ALPHABET: ... ?

With this in mind, I would recommend this implementation for cycle_text:

def cycle_text(text, key, reverse=False):
    text = text.lower()
    indexes = convert_key_to_indexes(key)
    index_of_key = 0
    result = ""
    for char in text:
        if char not in ALPHABET:
            result += char
        else:
            if not reverse:
                result += shift_letter_by(char, indexes[index_of_key])
            else:
                result += shift_letter_by(char, - indexes[index_of_key])
            index_of_key = get_next_index(indexes, index_of_key)
    return result

Another improvement I slipped in here is not reassigning to the key parameter inside the method body, as it was in the original code.

Why all the redundant parentheses in the return statements?

return(lst[new_index])
return(index)
# ...

All these could be simply like this without parentheses:

return lst[new_index]
return index
# ...

You should follow PEP8, the official coding style guide of Python.

For example, instead of this:

return encrypt(text,key,reverse_operation=True)

You should put a space after every comma separating a list of parameters, like this:

return encrypt(text, key, reverse_operation=True)

And put 2 empty lines before function declarations (before every def).
This rule is only for functions in the global namespace,
for functions inside classes 1 empty line is enough.


Instance variables and do loops in ruby on rails I find my self often doing things like this in Ruby on Rails:

@count = 0

LineItem.all.each do |item|
     if (item.cart_id == @cart.id)
       @count += item.quantity
     end
end

So many things look wrong with this, but I want to initialize my variable (@count) before the loop, and then be able to use it after I'm out of the loop.

Is there a better way to do this?

If you work with database you can do this more efficiently with sql request:

LineItem.where(:card_id => @card.id).sum('quantity')

This is equivalent sql query

SELECT SUM(items.identity) FROM items WHERE items.card_id = ?;


Vue.js dynamically passing in functions I have a jsonArray.js file
const jsonArray = [
  {"click": "clearValue", "alt": "clearValueIcon",
  "src" : "https://icons.iconarchive.com/icons/oxygen-icons.org/oxygen/128/Actions-edit-clear-icon.png",
  "style": "grid-column:1/2;grid-row: 2/3;"},
  {"click": "negativePositive", "alt": "negativePositiveIcon",
  "src" : "https://image.flaticon.com/icons/svg/1777/1777560.svg",
  "style":"grid-column:2/3;grid-row: 2/3"},
  {"click": "percentage", "alt": "percentageIcon",
  "src" : "https://image.flaticon.com/icons/svg/69/69839.svg",
  "style":"grid-column:3/4;grid-row: 2/3;"}
]

In my App.vue file I import it and give the value to imagesData
 beforeMount() {
    this.imagesData = JsonArray;
  }

Then I use a for loop to populate my img tags
<img
          v-for="img in imagesData"
          :key="img.alt"
          @click="getFunction(img.click)"
          :alt="img.alt"
          :src="img.src"
          :style="img.style"
        />

The only way I have been able to get the click to work is with a giant switch statement that evaluates the string and calls the method.
 getFunction: function(fn) {
      switch (fn) {
        case "clearValue":
          this.clearValue();
          break;
        case "negativePositive":
          this.negativePositive();
          break;
}}

I am thinking there is a better way, but cannot think of one.

First off, the file jsonArray.js doesn't contain "JSON". It's a JavaScript source code file containing an JavaScript array literal which in turn contains JavaScript object literals. And since it's not JSON, you could theoretically put functions into the objects:
{
  alt: "clearValueIcon", // Quotes around key are not needed, since it's not JSON
  src : "...", // shortened
  style: "grid-column:1/2;grid-row: 2/3;",
  handler() {
     this.value = ""; // For example
  }
},

And with a helping method:
methods: {
  callHandler(handler) {
     handler.call(this);
  }
}

<img
     v-for="img in imagesData"
     :key="img.alt"
     @click="callHandler(img.handler)"
     :alt="img.alt"
     :src="img.src"
     :style="img.style"
/>

However I don't really see the point of outsourcing the buttons into a data array like this. Unless you are using the array in some other way in the project, you should just straight up have a list of imgs (or custom "button" components) in your template.
Two more things:

The styles belong in the style sheet.
The chosen texts for the alt attributes are bad. They are supposed to be fallback texts that displayed to the user in the case the images can't be displayed to (or seen by) the user. For the shown buttons something like "Clear", "+/-" and "%" would be better choices.

EDIT: One more thing: You shouldn't be using a plain image with a click handler, but for accessability wrap them in a <button> (or <a>) and put the handler on them.


Efficiently read in 2nd column of CSV into List of Lists in Python 3 I created a function to read in arbitrary numbers of .csv files, their 2nd column only, into a list of lists (or if there is a more efficient data structure, then that). I want it to be efficient and scalable/customizable.
Is there anything in it that can be tweaked and improved?

import os
FILE_NAMES=["DOCS/1.csv","DOCS/2.csv"]

def FUNC_READ_FILES(file_names):
    nr_files=len(file_names)
    filedata=[[] for x in range(nr_files)] # efficient list of lists

    for i in range(nr_files):   # read in the files
        if(os.path.isfile(file_names[i])):
           with open(file_names[i],'r') as f:
              filedata[i]=f.readlines()
        else:
             print("ERROR: FILES ARE MISSING!!!!")
             exit()

    for i in range(nr_files):  # iterate through the files and only keep the 2nd column, remove \n if it's end of line
       for k in range(len(filedata[0])):
        filedata[i][k]=filedata[i][k].strip().split(',')[1]

    return (filedata)

FUNC_READ_FILES(FILE_NAMES)

General remarks

pep 8

for your names and code style, try to follow pep-8

lower_case for variable and function names
spaces around operators

main guard

put the calling of your functions after a if __name__ == '__main__':, so you can load the script from somewhere else without it executing the code immediately

looping

Don't loop over indices. Code like for i in range(nr_files):  is a lot cleaner using enumerate: for i, filename in enumerate(file_names).

I suggest you check out the excellent 'Looping like a Pro' talk by David Baumgold

functions

Instead of having 1 function to load the files, loop over them and pick the correct element, seasiest would be to split if into different functions:

takes the list of files, and passes them on one by one to the parse
parse a single file

Generators

The most pythonic an efficient approach to do this would be to use generators, pathlib.Path and the built-in csv module

My solution

parse one file

This function takes a filehandle, and parses the requested element from the line

import csv
from pathlib import Path

def parse_file(filehandle, field_name):
    kwargs = {  # https://docs.python.org/3/library/csv.html#csv-fmt-params
        'delimiter': ',',
        'skipinitialspace': True,
        # ...
    }
#     field_name = b  # or the column name
    reader = csv.DictReader(filehandle, **kwargs)  # or csv.reader if there is no header, and it might be faster
    for line in reader:
        yield line[field_name]

This can be easily tested like this:

from io import StringIO
csv_str = '''a, b, c
0, 1, 2
3, 4, 5'''
with StringIO(csv_str, newline='') as file:
    print(list(parse_file(file, 'b')))

['1', '4']

parse multiple file

def parse_files(files):
    for file in files:
        try:
            with filename.open('r', newline='', ) as csv_file:
                yield list(parse_file(csv_file))
        except FileNotFoundError:
            print("ERROR: FILES ARE MISSING!!!!")
            raise

Now we have a good method to parse the information, we just need to call it with the subsequent files

main

def main(files):
    results = list(parse_files(files))
    return results

if __name__ == '__main__':

    files= [Path("DOCS/1.csv"),Path("DOCS/2.csv")]
    main(files)


Python program to translate sentences from "bird language" Given a sentence in 'bird language', where there are two rules:

After each consonant there is a random vowel added. For example if the consonant is "l" it will end as "la" or "le"...
After each vowel there are two extra vowels which are the same as the first. For example if there is a "u" it will end as "uuu"

Vowels are "aeiouy"

Examples:

hieeelalaooo → hello
hoooowe yyyooouuu duoooiiine → how you doin
aaa bo cy da eee fe → a b c d e f
sooooso aaaaaaaaa → sos aaa

The code is:

VOWELS = "aeiouy"

def translate(phrase):

    result = ""
    num_of_letters_to_ignore = 0

    for i, letter in enumerate(phrase):
        if num_of_letters_to_ignore > 0:
            num_of_letters_to_ignore -= 1
            continue

        if letter in VOWELS:
            num_of_letters_to_ignore += 2
        elif letter.isalpha():
            num_of_letters_to_ignore += 1
        result += letter

    return result

How can it be improved?

EDIT: I have added the first sentence which was missing where it is correctly specified that y is in the vowels!

Considering that you have four concrete test cases, it would be a good idea to put them in a docstring as doctests.

Your iteration is awkward.  First of all, you never use i, so there is no need to call enumerate().  A better way to conditionally skip ahead is to call next() on an iterator over the phrase.

def translate(phrase):
    """
    Translate the phrase from bird language.

    >>> translate('hieeelalaooo')
    'hello'
    >>> translate('hoooowe yyyooouuu duoooiiine')
    'how you doin'
    >>> translate('aaa bo cy da eee fe')
    'a b c d e f'
    >>> translate('sooooso aaaaaaaaa')
    'sos aaa'
    """
    phrase = iter(phrase)
    result = ""
    for letter in phrase:
        result += letter
        if letter in VOWELS:   # Vowel. Skip the next two characters.
            next(phrase)
            next(phrase)
        elif letter.isalpha(): # Consonant. Skip the next character.
            next(phrase)
    return result

A much more compact solution, though, would be to perform a regular expression substitution.

import re

def translate(phrase):
    return re.sub(r'([aeiouy])\1\1|([a-z])[aeiouy]', r'\1\2', phrase, flags=re.I)


c++ Pseudo-random generator wrapper I wrote a simple wrapper for the <random> header, as follows:

#include <random>

namespace Random
{

    static int getInt(int min, int max)
    {
        std::random_device rd;
        std::mt19937 gen(rd());
        std::uniform_int_distribution<> distribution(min, max);

        int rnd = distribution(gen);

        return rnd;
    }

    static float getFloat(float min, float max)
    {
        std::random_device rd;
        std::mt19937 gen(rd());
        std::uniform_real_distribution<> distribution(min, max);

        float rnd = distribution(gen);

        return rnd;
    }

}

I basically just want to know: is there a better / more efficient way to do it?

I know for a fact that it works quite well, but I am no expert. I thank you in advance for your answers.

You might be able to save the overhead of constructing a new generator on each call, with something like:

static std::mt19937 default_generator(std::random_device{}());

static int getInt(int min, int max)
{
    return std::uniform_int_distribution{min, max}(default_generator);
}

But, given that most uses require several values from a single distribution and there's rarely a need to use more than one generator, is this header really worth its cognitive load?  Each function is a one-liner, and even less if the distribution can be re-used over several calls.

We'd also expect to have versions for longer or unsigned integer types, and for double and other floating-point types.  It probably makes more sense for it to be a template:

#include <random>
#include <type_traits>

template<typename T>
inline T getUniformRandom(T min, T max)
{
    static std::mt19937 default_generator(std::random_device{}());
    if constexpr (std::is_integral_v<T>) {
        return std::uniform_int_distribution{min, max}(default_generator);
    } else if constexpr (std::is_floating_point_v<T>) {
        return std::uniform_real_distribution{min, max}(default_generator);
    }

    static_assert(0, "getUniformRandom requires an arithmetic type");
    return min+max;             // just to eliminate compiler warnings
}

Alternatively, use std::enable_if instead of if constexpr:

#include <random>
#include <type_traits>

template<typename T>
inline std::enable_if_t<std::is_integral_v<T>, T> getUniformRandom(T min, T max)
{
    static std::mt19937 default_generator(std::random_device{}());
    return std::uniform_int_distribution{min, max}(default_generator);
}

template<typename T>
inline std::enable_if_t<std::is_floating_point_v<T>, T> getUniformRandom(T min, T max)
{
    static std::mt19937 default_generator(std::random_device{}());
    return std::uniform_real_distribution{min, max}(default_generator);
}


Python softmatcher using difflib impracticably slow I have a softmatch function (below) that takes a donor list and a new entry and looks to see if the given donor already exists.

The data is not exact, so I have to use a softmatch to determine whether a given record exists (ex: Jon Doe at 123 Sesame St. is the same as John P. Doe at 123 Sesame Street).  I have this implemented with difflib, and it works; it's just slow as Christmas.

The program currently takes about two days to process 10mb of data.  The profiler indicates it's the difflib operations in the softmatch function that is causing the slowness.

Is there a way to optimize my matching function to work better?

def softMatch(self, new, donors):
    #Takes new, a contribution record from a DG report, and attempts to soft match it with a record in donors
    #Returns the position of the matched record or -1 if no match
    #Methodology
        #Name
        #Address
        #Affiliation,Occupation,Employer not analyzed
    #Dependences cleanAddr(str), re(from cleanAddr), difflib

    match = -1

    name = new[0]
    address = self.cleanAddr(new[1] + " " + new[2] + " " + new[3] + " " + new[4] + " " + new[5][:5])

    while address.find("  ") != -1:
        address = address.replace("  "," ")

    diff = 0.6

    for x in range(0, len(donors)):
        ratio = difflib.SequenceMatcher(None, name, donors[x].getBestName()[0]).ratio() * difflib.SequenceMatcher(None, address, donors[x].getBestAddr()).ratio()**2
        if ratio > diff:
            diff = ratio
            match = x

    return match

And the profiler output:

   Ordered by: internal time

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
  3091394   53.737    0.000   66.798    0.000 difflib.py:353(find_longest_match)
   368770   10.030    0.000   80.702    0.000 difflib.py:463(get_matching_blocks)
 59454412    7.683    0.000    7.683    0.000 {method 'get' of 'dict' objects}
   368770    6.906    0.000   10.521    0.000 difflib.py:300(__chain_b)
  5210450    4.009    0.000    4.009    0.000 {built-in method __new__ of type object at 0x1001e14a0}
 16895496    3.056    0.000    3.056    0.000 {method 'append' of 'list' objects}
      756    2.863    0.004  104.674    0.138 june12.py:74(softMatch)
  3091394    1.875    0.000    1.875    0.000 {method 'pop' of 'list' objects}
 10299176    1.850    0.000    1.850    0.000 {method 'setdefault' of 'dict' objects}
  2487807    1.808    0.000    4.634    0.000 difflib.py:661(<genexpr>)
  3091394    1.620    0.000    4.345    0.000 <string>:12(__new__)
   368770    1.461    0.000   88.032    0.000 difflib.py:639(ratio)
  2119037    1.258    0.000    2.826    0.000 <string>:16(_make)
  8127502    1.032    0.000    1.032    0.000 {method '__contains__' of 'set' objects}
   184385    0.919    0.000    1.500    0.000 Donor.py:63(getBestAddr)
   368770    0.859    0.000    5.493    0.000 {built-in method sum}
   368770    0.554    0.000   12.131    0.000 difflib.py:154(__init__)
   368817    0.552    0.000    0.552    0.000 {method 'sort' of 'list' objects}
3965153/3965134    0.538    0.000    0.538    0.000 {built-in method len}
   368770    0.421    0.000   11.577    0.000 difflib.py:218(set_seqs)
   368770    0.403    0.000   10.924    0.000 difflib.py:256(set_seq2)
   337141    0.371    0.000    0.371    0.000 {method 'find' of 'str' objects}
   368770    0.281    0.000    0.281    0.000 difflib.py:41(_calculate_ratio)
   368770    0.232    0.000    0.232    0.000 difflib.py:230(set_seq1)
   159562    0.216    0.000    0.216    0.000 {method 'replace' of 'str' objects}
   184385    0.134    0.000    0.134    0.000 Donor.py:59(getBestName)
   1    0.017    0.017  104.718  104.718 june12.py:56(loadDonors)
    ...

(softmatch is called from loadDonors)

I recommend you try different approaches:

Use quick_ratio instead of ratio
Apply .lower().split(' ') to the data before sending it to the SequenceMatcher
Add a line somewhere that checks if the two strings are equal. In those cases don't call SequenceMatcher

The first approach will most likely reduce the overall run time, but decrease precision. The second approach should work pretty well, since SequenceMatcher knows how to handle lists and will match only two items: ['john', 'doe'] and ['john', 'doe'] instead of all the letters in the name.


Prime sieve: improve efficiency while keeping it reasonably simple? public static void listPrimesThree(int maxNum){
long startTime = System.currentTimeMillis();

boolean[] booleanArray = new boolean[maxNum+1];
int root = (int)Math.sqrt(maxNum);

for (int m = 2; m <= root; m++){
  for (int k = m*m; k <= maxNum; k+=m){
    booleanArray[k] = true;
  }
}
for (int m = 3; m <= maxNum; m+=2){
  if(!booleanArray[m]){
    System.out.println(m);
  }
}
long endTime = System.currentTimeMillis();
System.err.println(endTime - startTime+"ms");
}

This is a great example of memory bound problem. If you manage to reduce the number of cache misses you can have quite a speedup.
I am providing almost identical code, only instead of boolean array I am using BitSet. Running on Server machinve, on my quite dated hardware I observe about 30% speedup without any other optimizations. Hand-optimizing the bit set functionality may speed things a little more, although I am not too much an optimist.

I am providing this example only to show how important memory (micro) management can be. I do not suggest that the original algorithm can not be optimized.

public void listPrimesThree(int maxNum) {
    long startTime = System.currentTimeMillis();

    int root = (int) Math.sqrt(maxNum);

    BitSet bs = new BitSet(maxNum+1);
    for (int m = 2; m <= root; m++) {
        for (int k = m * m; k <= maxNum; k += m) {
            bs.set(k);
        }
    }
    long endTime = System.currentTimeMillis();
    System.err.println((endTime - startTime) / 1000 + " s");
    for (int m = 3; m <= maxNum; m += 2) {
        if (!bs.get(m)) {
            System.out.println(m);
        }
    }
}


Too many for-loops and conditional operators Not please with this outcome, function works as is, but would love to simplify and prettify it. Use of one for-loop would be great and limit the use of the conditional operator.

private void checkForOverage(List<GoogleUsageMapping> records) {
    Long sumOfAllocatedSize = 0L;
    Long sumOfLogicalSize = 0L;
    for (GoogleUsageMapping record : records) {
        // initially all records are set to GoogleTrackingState.UNPROCESSED
        if (record.getUsage().getMeasuredType() == MeasuredType.ALLOCATED_SIZE) {
            sumOfAllocatedSize += record.getUsage().getQuantity().longValue();
        } else if (record.getUsage().getMeasuredType() == MeasuredType.LOGICAL_SIZE) {
            sumOfLogicalSize += record.getUsage().getQuantity().longValue();
        }
    }

    for (GoogleUsageMapping record : records) {
        if (sumOfAllocatedSize >= sumOfLogicalSize && record.getUsage().getMeasuredType() == MeasuredType.LOGICAL_SIZE) {
            record.setState(GoogleTrackingState.IGNORED_OVERAGE);
        } else if (sumOfLogicalSize > sumOfAllocatedSize && record.getUsage().getMeasuredType() == MeasuredType.ALLOCATED_SIZE) {
            record.setState(GoogleTrackingState.IGNORED_OVERAGE);
        }
    }
}

If you have not done so. I recommend picking up the book Refactoring: Improving the Design of Existing Code by Martin Fowler. It is an excellent book on exactly this topic.

One thing you can start with is to split the method by extracting each operation into a separate method. So that instead of doing everything at once you end up with a method per operation.

Doing so will let you examine what your code is actually doing and helps you see the patterns. Something like this:

private void checkForOverage(List<GoogleUsageMapping> records) {
    long allocatedSize = sumAllocated(records);
    long logicalSize = sumLogical(records);
    checkAllocated(records,allocatedSize, logicalSize);
    checkLogical(records,allocatedSize, logicalSize);
}

private long sumAllocated(List<GoogleUsageMapping> records) {
    long sumOfAllocatedSize = 0L;
    for (GoogleUsageMapping record : records) {
        if (record.getUsage().getMeasuredType() == MeasuredType.ALLOCATED_SIZE) {
            sumOfAllocatedSize += record.getUsage().getQuantity().longValue();
        }
    }
    return sumOfAllocatedSize;
}

private long sumLogical(List<GoogleUsageMapping> records) {
    long sumOfLogicalSize = 0L;
    for (GoogleUsageMapping record : records) {
        if (record.getUsage().getMeasuredType() == MeasuredType.LOGICAL_SIZE) {
            sumOfLogicalSize += record.getUsage().getQuantity().longValue();
        }
    }
    return sumOfLogicalSize;
}

private void checkAllocated(List<GoogleUsageMapping> records, long allocatedSize, long logicalSize) {
    for (GoogleUsageMapping record : records) {
        if (allocatedSize >= logicalSize && record.getUsage().getMeasuredType() == MeasuredType.LOGICAL_SIZE) {
            record.setState(GoogleTrackingState.IGNORED_OVERAGE);
        }
    }
}

private void checkLogical(List<GoogleUsageMapping> records, long allocatedSize, long logicalSize) {
    for (GoogleUsageMapping record : records) {
        if (logicalSize > allocatedSize && record.getUsage().getMeasuredType() == MeasuredType.ALLOCATED_SIZE) {
            record.setState(GoogleTrackingState.IGNORED_OVERAGE);
        }
    }
}

As you can see both pairs of methods are nearly identical. All you have to do to unify them is to extract the difference as parameter.

private void checkForOverage(List<GoogleUsageMapping> records) {
    long allocatedSize = sumByType(records, MeasuredType.ALLOCATED_SIZE);
    long logicalSize = sumByType(records, MeasuredType.LOGICAL_SIZE);
    check(records, allocatedSize >= logicalSize, MeasuredType.ALLOCATED_SIZE);
    check(records, logicalSize > allocatedSize, MeasuredType.LOGICAL_SIZE);
}

private long sumByType(List<GoogleUsageMapping> records, MeasuredType type) {
    long sumOfAllocatedSize = 0L;
    for (GoogleUsageMapping record : records) {
        if (record.getUsage().getMeasuredType() == type) {
            sumOfAllocatedSize += record.getUsage().getQuantity().longValue();
        }
    }
    return sumOfAllocatedSize;
}

private void check(List<GoogleUsageMapping> records, boolean condition, MeasuredType type) {
    for (GoogleUsageMapping record : records) {
        if (condition && record.getUsage().getMeasuredType() == type) {
            record.setState(GoogleTrackingState.IGNORED_OVERAGE);
        }
    }
}


Tracking the stability of a website using ping monitor I'm new to coding and made a small Node script that checks when a website is down because of high traffic. It's a website that sells tickets and I don't want to press F5 every 10 seconds to check if the website works again.

Because I send a ping so often I sometimes receive something back from the website. But when I check it again a couple of seconds later I get a request timeout again.

So I made a counter to track how stable the website is. If I receive a request timeout subtract from the counter.

It works, but I was wondering if there is a better way or solution to do this. Because I'm still new I don't have much experience with other solutions to this sort of problem.

This is my code:

const Monitor = require('ping-monitor');
const sendEmail = require('./Email.js')

const myMonitor = new Monitor({
  website: 'https://www.pathe.nl/',
  title: 'Pathe',
  interval: 0.1 // minutes, so 0.1 is 6 seconds
});

let counterUp = 0;

myMonitor.on('up', function (res, state) {
  counterUp++
  console.log('counter is ' + counterUp)
  console.log('Yay!! ' + res.website + ' is up.');

  // send email when script made sure the website is really up and stable. Otherwise we receive an email when the website isn't fully up and running. Because we send a ping every 6 seconds.
  if (counterUp == 20) {
    sendEmail(myMonitor.title)
    console.log('sending email')
  }
});

myMonitor.on('down', function (res) {
  counterUp--
  console.log('counter is ' + counterUp)
  console.log('Oh Snap!! ' + res.website + ' is down! ' + res.statusMessage);
});

myMonitor.on('stop', function (website) {
  console.log(website + ' monitor has stopped.');
});

myMonitor.on('error', function (error) {
  console.log(error);
});

In reality, people just use third-party services for that. For instance, uptime website monitoring can be done StatusCake. If you own the target site, you install infrastructure tools like New Relic. Cloud services these days also provide uptime data and tools out of the box.

The only reason I would write my own uptime monitor is if  you're doing this for a small project or doing it for fun.

Now the problem with the uptime monitor you wrote is that it contributes to the site traffic. To the website, you're just another client trying to connect to the site. Together with your traffic are other sources of traffic, like legitimate users, web crawlers, malicious site scanners, DDOS bots, etc.

The reason you get the Request Timeout (HTTP 504) is because the site is already overloaded. It can be worse though. The site may choose to ignore you, block you temporarily, or even permanently. If you've seen a captcha appear when doing a Google search, that's one example.

So, what I recommend is to:

Reduce the rate you ping. Every 5, 15, or 60 minutes is reasonable. You're probably not going to need it every 6 seconds.
Hit a static asset on the domain instead, like the favicon. Hitting actual pages will cause 504s since it requires logic and probably database calls to generate the page. Just make sure it's not cached or served from a CDN.

This only tells you if the static server/caching proxy is up, not the actual server with the logic.

If the website has an API for uptime or a status page that you can scrape, use that instead. Those are usually served by a separate server so they can still tell you the status even if the main site is down.
If you own the website, monitor the infrastructure instead.


Toy Expression Interpreter I've just started learning Rust (coming from Haskell) and decided to test a toy expression interpreter.

Code:

use std::rc::Rc;

pub type RAST<T> = Rc<AST<T>>;

#[derive(Debug)]
pub enum AST<T> {
    ConstInt(isize, Rc<T>),
    Add(RAST<T>, RAST<T>, Rc<T>),
}

pub fn eval<T>(x: &RAST<T>) -> RAST<T> {
    match &**x {
        AST::Add::<T>(l, r, t) => {
            let el = eval(&l);
            let er = eval(&r);
            match (&*el, &*er) {
                (AST::ConstInt::<T>(li,_q),
                 AST::ConstInt::<T>(ri,_r)) =>
                    Rc::new(AST::ConstInt::<T>(li+ri, Rc::clone(t))),
                _ => mk_add(&el, &er, &t),
            }
        },
        _ => Rc::clone(x),
    }
}

pub fn mk_int<T>(i: isize, t: &Rc<T>) -> RAST<T> {
    Rc::new(AST::ConstInt::<T>(i, Rc::clone(t)))
}

pub fn mk_add<T>(l: &RAST<T>, r: &RAST<T>, t: &Rc<T>) -> RAST<T> {
    Rc::new(AST::Add::<T>(Rc::clone(l), Rc::clone(r), Rc::clone(t)))
}

pub fn main() {
    let a = Rc::new(());

    let x = mk_int(3, &a);
    let y = mk_int(6, &a);
    let z = mk_add(&x, &y, &a);
    let c = mk_add(&x, &z, &a);

    let r = eval(&c);
    println!("Raw: {:?}", c);
    println!("Evaluated: {:?}", r);
}

With this output:

Raw: Add(ConstInt(3, ()), Add(ConstInt(3, ()), ConstInt(6, ()), ()), ())
Evaluated: ConstInt(12, ())

I'd really appreciate any feedback on:

Am I using the "right" type for the Add subnodes (i.e. Rc)?
Is it possible to create eval as an AST method instead of a function? I kept fighting the borrow checking if x in eval was self : &AST<T> (within the impl block, of course)
Is there any way to make this program more concise/cleaner? I think I understand why all the additional wrapping and sprinkling of Rc::clone/new and lots of reference taking is needed, but was wondering if I was perhaps missing something that might make this code cleaner to read.
Any other feedback/helpful pointers would also be appreciated. Thank you!

Am I using the "right" type for the Add subnodes (i.e. Rc)?

No. You probably don't want to do that. Just use AST. Good Rust code rarely actually needs Rc. Instead of using Rc<> in the definition of AST, use Box.

#[derive(Debug)]
pub enum AST<T> {
    ConstInt(isize, Rc<T>),
    Add(Box<AST<T>>, Box<AST<T>>, Rc<T>),
}

Now Rc does give you the ability to reuse branches of the AST. For example, you use x twice. However, I've not seen a real case of ASTs where that needs to happen. As it stands you are paying the costs of RC (both in programmer effort, execution time, and memory overahead) for no good reason.

You should also get rid of the Rc<> for the T. Just store a T. In cases where you need to be able to copy the T for some reason (which is probably never), take a <T: Clone> so you can call the clone method. The client code can then put in Rc<> in place if it wants, or if the data is trivial just allow it to be copied.

I would also question the usefulness of a generic type here at all. Are you really going to annotate the same AST with different additional data in different places? I rather doubt it.

  Is it possible to create eval as an AST method instead of a function? I kept fighting the borrow checking if x in eval was self : &AST (within the impl block, of course)

You'll find that if you don't try to pass ASTs in Rc<> that this will work much better. The problem is that you can't take self to be an Rc<>, so you don't get access to the reference counter itself which makes it difficult to do anything useful.

Even if you use RC, you can define a static method in the impl (just don't have a self parameter. Then you can use syntax like AST::eval(ast).

pub fn eval<T>(x: &RAST<T>) -> RAST<T> {

This function is pretty strange. Why does an evaluation function return an AST. Shouldn't it return a value? If instead this function were to return isize it would be a much simpler function.

However, the logic of the function makes more sense if it is a optimization function. That is, it seeks to produce a more optimized version of the AST rather than evaluate the AST. But if that's the idea, your function takes the wrong type. It might make sense for the optimization function to take ownership of the AST, consuming the old AST reusing peices of it for the new AST. Or it might make sense to take a mutable reference to the AST, modifying the AST to make a more optimized version.

  Is there any way to make this program more concise/cleaner? I think I understand why all the additional wrapping and sprinkling of Rc::clone/new and lots of reference taking is needed, but was wondering if I was perhaps missing something that might make this code cleaner to read.

Yes, if you follow my advice aboev you'll find the code a lot more concise and cleaner.


Is this an acceptable way of writing PHP code(in scriptlets) Is this the way PHP developers write PHP or is echoing the html the preferred method?

<?php

require_once 'db_connect.php';
session_start();

$result = mysql_query("SELECT * FROM products");
?>

    Product ID

    Name

    Description

    Price

    Image

    Action

<?php
while($row = mysql_fetch_array($result))
{
    ?>

            <?php
                echo $row['p_id'];
            ?>

            <?php
                echo $row['p_name'];
            ?>

            <?php
                echo $row['p_description'];
            ?>

            <?php
                echo $row['p_price'];
            ?>

            <?php
                echo $row['p_image_path'];
            ?>

            edit

            delete

<?php
}
?>

A few points:

Put the session_start() and as much of processing as possible on top of the file, before any output. This will be useful for emitting http-headers.

When using a database: don't forget to handle exceptions.

When using a database: don't forget to handle  charsets.

When selecting from the database: always limit your results. Add pagination of some kind.

Do not use mysql_*, use a db library that offers prepared statements and is as db-independent as possible. I would recommend PDO.  I would also recommend getting the data from the db as objects. If necessary you can define a class to hold some methods you need for these objects.

I cuncur with Cygal: using php as the templating language is quite ok.  See Wordpress for an example where this is used extensively in themes.

Try to write your HTML in a compact, readable way. No matter what you use for templating: HTML is an important part of your code, and should be just as readable as any other code you write.

Output valid HTML. Don't forget question marks around attribute values, don't forget -Tags.

   <?php

      session_start();
      include "config.php";
      if( ! $DB_NAME ) die('please create config.php, define $DB_NAME, $DB_USER, $DB_PASS there');

      try {

          $dbh = new PDO("mysql:dbname=$DB_NAME", $DB_USER, $DB_PASS);
          $dbh->exec('SET CHARACTER SET utf8') ;

          $result = $dbh->query("SELECT * FROM products LIMIT 0,20");
          $products = $result->fetchAll(PDO::FETCH_OBJ);

      } catch( Exception $e ) {

          error_log( $e->getMessage() . " ".  $e->getTraceAsString() );
          include("500-failwhale.php");
          exit;
      }
    ?>

    <!DOCTYPE html>

      <title>Products</title>
      <meta charset="utf-8">

            Product ID
            Name
            Description
            Price
            Image
             Action

    <?php foreach($products as $p) : ?>

            <?= $p->p_id          ?>
            <?= $p->p_name        ?>
            <?= $p->p_description ?>
            <?= $p->p_price       ?>
            <?php if ( $p->p_image_path ) : ?>
                   <img src="<?= $p->p_image_path ?>" alt="<?= $p->p_name ?>">
            <?php endif; ?>
            edit
            delete

    <?php endforeach; ?>


Properly Structured Angular Application I've been writing a somewhat complicated service that involves a lot of API function calls in Angular. After a somewhat tortured first attempt I have tried to take more of an MVC approach rewritten the Angular in the following ways.

All data, once it's retrieved from the API, is stored in a Service with a setter or getter, i. e. (MODEL):

.service('userFirstName', function() {
  var userFirstNameProp;
  return {
    getter: function() {
      return userFirstNameProp;
    },
    setter: function(value) {
        userFirstNameProp = value;
      }
    };
  })

That way the data can be retrived from any controller via the setter or getter with a simple function call to determine whether it's already been properly set.
All API calls are pushed into factories using Angular's $resource module.  A sample call might be (CONTROLLER):

.factory("someQueryFactory", function($resource, userToken) {
  var resource = $resource(serverPath + apiPath, {}, {
    get: {
      method: "GET",
      headers: {
        'uid': function() {
          var uid= curUid.getter();
          return token;
        }
      }
    }
  });
  return resource;
})

Each "page" is given its own route, partial, and controller (VIEW):

$routeProvider
  .when('/', {
    templateUrl: '/partials/login1.html',
    controller: 'homePageController'
  })

Each controller handles the logic of each page.  I avoided using directives because the logic of the pages is not too complex and almost entirely consists of seeing if the user's last step has been completed to the satisfaction of the user's next step (i. e. has the user logged in?  Then take them to the home page.)

So far so good, but two issues:

I notice my controller headers seem fairly bloated, mostly because of the need to include the the modules, services, and factories in them.  So maybe something like:

 LSPortal.controller('portalLoginController', ['$scope', '$log', '$location', 'thingOne', 'thingTwo', 'thingThree', 'userStatus', 'userFirstName', 'userLastName', 'userPetName', 'userColor', 'userGender', 'apiQueryOne', 'apiQueryTwo', 'apiQueryThree', function($scope, $log, $location, thingOne, thingTwo, thingThree, userStatus, userFirstName, userLastName, userPetName, userColor, userGender, apiQueryOne, apiQueryTwo, apiQueryThree) { //logic }]);

I understand I could use something like RestMod or Restangular to reduce multiple query calls, but the API is complex and I want something that's going to be supported going forward in Angular that I don't have to throw out in three months.
It's also a lot of getter and setter calls, many times involving doing a bunch of setting at the end of one page only to have to do a lot of getting at the beginning of the next.  This seems like an improvement over my formerly somewhat tortured use of $scope, but is it?

I've been feeling free to mix AngularJS with regular JS but the more I read the more it seems I should try to keep things in Angular as much as possible.  Not sure if this is illustrated above but if someone needs a better example I'd be glad to provide one.

And is there anything else I'm missing?  A lot of tutorials go over certain facets of angular but very few seek to put everything together.  All help appreciated.

I notice my controller headers seem fairly bloated, mostly because of the need to include the the modules, services, and factories in them. So maybe something like

You can use the implicit method of dependency injection, where as long as the variable names match the dependency name, Angular will figure it out for you. However, for production, you'll need ngAnnotate to convert it back to the explicit form before minification.

  I understand I could use something like RestMod or Restangular to reduce multiple query calls.

If this means wiring up multiple API calls to generate cohesive data (merging data from multiple calls), then I believe this calls for a service which hides stuff away from the controller. Multiple calls can be done in parallel.

// SomeMergedDataService
getSomethingFromMultipleResources: function(){
  return $q.all($http.get(), $http.get(), ...).then(function(values){
    // `values` is an array containing the results of each ajax call
    // Do the merge logic here
    return mergedData;
  });
};

// meanwhile... in your controller
App.controller('MyController', function(SomeMergedDataService){
  SomeMergedDataService.getSomethingFromMultipleResources().then(function(data){
    // you got merged data!
  });
});

Another way to do this is via the route's resolve property. It's essentially the same thing, except that instead of injecting the service and calling a method to get the data, the injected resolve is the data. A resolved property takes a function that can return a promise. You can feed the result of your $q.all().then() as the return.

  a bunch of setting at the end of one page only to have to do a lot of getting at the beginning of the next

One way to do it is by way of $rootScope. You can treat $rootScope as the global space of the application.

If you don't want to pollute $rootScope, you can also do it like how React+Flux, by using services as storage locations. This does entail getters and setters though, which I think you're already doing now.

If you want the state pushed down to the controller level, you can nest controllers. Descendant controllers have access to the $scope properties of the enclosing controller. This means that you can put stuff on the parent controller and have access to it from any descendant.

  I've been feeling free to mix AngularJS with regular JS but the more I read the more it seems I should try to keep things in Angular as much as possible.

It's perfectly fine to mix. They're all JS anyways. It's really up to you. Angular is pretty flexible despite its crazy conventions.

Angular does provide some very handy modules which are usually optimized to how Angular internals work. I especially use $timeout instead of setTimeout, or $log instead of console.

However, I certainly dislike certain built-ins such as $q as it doesn't behave like the native promises. This makes $http and resources weird as it uses $q internally.

  A lot of tutorials go over certain facets of angular but very few seek to put everything together.

I suggest you go over and look at JHipster. It's a stack that uses Angular+Spring. I'm not suggesting you go over to Java, but it has a good way of structuring an Angular app front-to-back. I suggest you go try to run it's setup and see how it provides the default services, controllers, templates etc. and how it's made to talk to the server.


C++ class template for notification management Recently, I found myself frequently writing notification managing facilities. That is, one can register as a receiver by providing a callback, and another can send a notification to all registrants by invoking their corresponding callbacks. I had been bothered by re-writing this similar code again and again. Finally, I decided to write a generic one once and for all. Following is the version for cases where the maximum number of registrants is known and limited. So I don't bother using advanced data structures for managing the registered callbacks. Comments pertaining to any aspect of the code are welcome :)

#include <array>
#include <functional>
#include <mutex>
#include <stdexcept>
#include <utility>

template <typename Sig, int max_reg = 16, typename Func = std::function<Sig>>
class NotifyRegistry;

template <int max_reg, typename Func, typename R, typename... Args>
class NotifyRegistry<R(Args...), max_reg, Func>
{
  static_assert(max_reg > 0, "");
public:
  template <typename F>
  int Register(F&& f) {
    std::lock_guard<std::mutex> lock(mut_);
    int i = -1;
    while (++i < max_reg && reg_[i]);
    if (i == max_reg) throw std::length_error("");
    reg_[i] = std::forward<F>(f);
    return i;
  }
  void Unregister(int id) {
    if (id < 0 || id >= max_reg) throw std::invalid_argument("");
    std::lock_guard<std::mutex> lock(mut_);
    reg_[id] = nullptr;
  }
  void Notify(Args... args) const {
    std::lock_guard<std::mutex> lock(mut_);
    for (auto& f : reg_) {
      if (f) (void)f(std::forward<Args>(args)...);
    }
  }
private:
  std::array<Func, max_reg> reg_;
  mutable std::mutex mut_;
};

Your class seems generally well done and easy to use.  I have only a few suggested improvements.

Add text to throws

It would be useful to have something other than a null string when the object throws an error.

Add text to the static_assert

The same issue exists for the static_assert in that it would be useful to provide a meaningful error message instead of an empty string.

Provide some comments

Because the class is compact and generally well written, not much explanation is required, but a brief explanation, especially noting which exceptions might be thrown, would be useful.


Counting the occurrences of a string in each file I am trying to find a phrase or a string in file and count the number of occurrences of that string:

 private static HashMap<String, Integer> bruteForceSearch(
      String stringToLookFor, List<File> list) throws IOException {

    HashMap<String, Integer> result = new HashMap<String, Integer>();
    for (File file : list) {
      String fileName = file.getName();
      // System.out.println(fileName);
      FileInputStream fstream = new FileInputStream(file);
      BufferedReader in = new BufferedReader(new InputStreamReader(fstream));
      String readLine = "";
      int count = 0;
      while ((readLine = in.readLine()) != null) {
        String[] words = readLine.split(" ");
        for (String text : words) {
          if (text.equalsIgnoreCase(stringToLookFor)) {
            count++;
          }
        }
      }
      if (count != 0) {
        result.put(fileName, count);
      }

      in.close();
    }

    return result;

  }

Are there any improvements to do in my code? Please let me know.

The first comment is that your code doesn't handle exceptions at all. You open a FileInputStream but you do not close it in case of exceptions. This can lead to a resource leak. Beginning with Java 7, you can use the try-with-resources statement to make sure that all open streams are closed properly. Instead of having

FileInputStream fstream = new FileInputStream(file);
BufferedReader in = new BufferedReader(new InputStreamReader(fstream));

you would have

try (FileInputStream fstream = new FileInputStream(file);
     BufferedReader in = new BufferedReader(new InputStreamReader(fstream));
) {
    // do the rest here
}

When the try block will finish, whether normally or exceptionally, all the streams that were opened in the try initialization block will be closed.
You don't need to wrap your stream into a InputStreamReader and a FileInputStream. You can just use a FileReader that does this already, turning your code into

try (BufferedReader in = new BufferedReader(new FileReader(file))) {
    // do the rest here
}

When the BufferedReader will be closed, it will also close the underlying FileReader.
You should prefer to program against interfaces and not implementation. Consider changing this

HashMap<String, Integer> result = new HashMap<String, Integer>();

to this

Map<String, Integer> result = new HashMap<String, Integer>();

or even

Map<String, Integer> result = new HashMap<>();

since you don't need to reiterate the type parameters since Java 7. In the same way, your method could return a Map<String, Integer> instead of a HashMap<String, Integer>.
Try to remove commented out code like // System.out.println(fileName);.