Oracle query to split 1 row into two where conditions are applicable The scenario question:

"I'm having difficulties with splitting a single row into two individual ones."

My test schema is:

CREATE TABLE T
    (id int,
     old varchar2(24),
     new varchar2(24))
;

where old and new are strings that need to be split, an example row:

  INTO T (id, old, new)
     VALUES (14, 'L-D / T-E', 'L-E / T-E')
  INTO T (id, old, new)
     VALUES (14, 'L-D / T-E', 'L-D / T-D')
  INTO T (id, old, new)
     VALUES (12, 'L-D / T-E', 'L-E / T-D')

In this example 'L' has changed in row 1, 'T' has changed in row 2 and both have changed in row 3.

The table is:

ID    OLD         NEW
14    L-D / T-E   L-E / T-E
14    L-D / T-E   L-D / T-D
12    L-D / T-E   L-E / T-D

I'm trying to split these so that the resulting table will have individual rows for each significant change such that the result will look like:

ID  OLD     NEW
1   L-D     L-E  //for existing row 1
2   T-E     T-D  //for existing row 2
//then two rows for existing row 3
3   L-D     L-E
4   T-E     T-D

Once these rows have been inserted, I'd then like to delete the existing rows.

My solution looks like:

    drop table t;
CREATE TABLE T
    (id int,
   key int,
  old varchar2(24),
  new varchar2(24))
;
INSERT ALL
      INTO T (id, key, old, new)
         VALUES (12, 638, 'L-D / T-E', 'L-E / T-E')
      INTO T (id, key, old, new)
         VALUES (13, 638, 'L-D / T-E', 'L-D / T-D')
      INTO T (id, key ,old, new)
         VALUES (14, 638, 'L-D / T-E', 'L-E / T-D')
SELECT * FROM dual;
SELECT * FROM T;
--insert into t (id, old, new) values (1,'dasdsad', 'asdasd');

BEGIN
  INSERT INTO t (id, key, old, new)
      WITH DATA AS
      (SELECT id, old, new
      FROM t
      WHERE
      --1 change in t
      ((SUBSTR(OLD,3,1) = SUBSTR(NEW, 3,1)
        AND SUBSTR(OLD, 9) <> SUBSTR(NEW, 9)
        )) )--end select,
    SELECT id, 646, substr(old, 7, 9) old,
               substr(new, 7, 9) new

    FROM DATA
      CONNECT BY  level = 1;

    INSERT INTO t (id, key, old, new)
      WITH DATA AS
      (SELECT id,OLD, new
      FROM t
      WHERE
      --1 change in t
      ((SUBSTR(OLD,3,1) <> SUBSTR(NEW, 3,1)
        AND SUBSTR(OLD, 9) = SUBSTR(NEW, 9)
        )) )--end select,
    SELECT id, 645, substr(old, 1, 3) old,
               substr(new, 1, 3) new

    FROM DATA
      CONNECT BY  level = 1;

    INSERT INTO t (id,key,old, new)
      WITH DATA AS
      (SELECT id,OLD, new
      FROM t
      WHERE
      --BOLLOX, need two keys, one for each row.
      ((SUBSTR(OLD,3,1) <> SUBSTR(NEW, 3,1)
        AND SUBSTR(OLD, 9) <> SUBSTR(NEW, 9)
        )) )--end select,
    SELECT id,
          (644+level), --will result in 645 and 646 --needs testing
          trim(regexp_substr(OLD, '[^/]+', 1, LEVEL)) OLD,
          trim(regexp_substr(NEW, '[^/]+', 1, LEVEL)) NEW
    FROM DATA
      CONNECT BY (LEVEL <= regexp_count(new, '/')+1);

      delete from t where key = 638;
END;
 /

It's not very elegant, so any advice, critique and suggestions are welcome.

I'd solve the problem by providing a PL/SQL procedure that knows the "split"-logic.

In the example below I assume that non-splitted rows are not inserted into the table at all but all inserts should be done with the procedure. This is a rather common pattern when there's data manipulations but it is not clear from your problem statement if this is an acceptable solution in your case.

I used regular expressions as it's a very handy tool for pattern matching with superior expressiveness and clarity compared to substr(instr()) spaghetti.

I didn't used triggers as it's not possible to modify the table trigger is attached to.

The solution can be easily applied even if you need to read the input data (i.e. non-splitted) from the same table.

Example

create table cr1_t(
 id number
,orig_id number
,old varchar2(24)
,new varchar2(24)
);

create sequence cr1_s;

create or replace procedure cr1_insert(
  p_id in number
 ,p_old in varchar2
 ,p_new in varchar2
) is
  -- the p_*-variables are visible in the nested subprogram
  procedure insert_if(p_pattern in varchar2) is
    v_old constant varchar2(32767) := regexp_substr(p_old, p_pattern);
    v_new constant varchar2(32767) := regexp_substr(p_new, p_pattern);
  begin
    -- insert only if the values are different
    if v_old <> v_new
    then
      insert into cr1_t(id, orig_id, old, new) values (cr1_s.nextval, p_id, v_old, v_new);
    end if;
  end;
begin
  -- https://en.wikipedia.org/wiki/Tuple
  -- note the separator doesn't matter

  -- the pattern will match the first member of a "2-tuple"
  -- regexp explained:
  -- ^           anchor the pattern to the beginning of a string
  -- [[:upper:]] any uppercase character
  -- -           character '-'
  -- [[:upper:]] any uppercase character
  --
  insert_if(p_pattern => '^[[:upper:]]-[[:upper:]]');

  -- the pattern will match the second member of a "2-tuple"
  -- (T-[[:upper:]])$
  -- regexp is similar than above but now the search is anchored to the end of the string ($):
  insert_if(p_pattern => '[[:upper:]]-[[:upper:]]$');
end;
/

Test run

begin
  for i in (with data(id, old, new) as (
              select 10, 'L-D / T-E', 'L-E / T-E' from dual union all
              select 20, 'L-D / T-E', 'L-D / T-D' from dual union all
              select 30, 'L-D / T-E', 'L-E / T-D' from dual union all
              select 40, 'A-B | C-D', 'A-E | C-D' from dual union all
              select 50, 'L-D / T-E', 'L-D / T-E' from dual
            ) select * from data)
  loop
    cr1_insert(i.id, i.old, i.new);
  end loop;
end;
/

select * from cr1_t;

ID ORIG_ID OLD NEW
-- ------- --- ---
 1      10 L-D L-E
 2      20 T-E T-D
 3      30 L-D L-E
 4      30 T-E T-D
 5      40 A-B A-E


group almost duplicate strings I am using the following script to fuzzy search almost similar book names for duplicate:
import re
from nltk.util import ngrams

OriginalBooksList = list()
booksAfterRemovingStopWords = list()
booksWithNGrams = list()
duplicatesSorted = list()

stopWords = ['I', 'a', 'about', 'an', 'are', 'as', 'at', 'be', 'by', 'com', 'for', 'from', 'how', 'in', 'is', 'it', 'of', 'on', 'or', 'that', 'the', 'this', 'to', 'was', 'the',
             'and', 'A', 'About', 'An', 'Are', 'As', 'At', 'Be', 'By', 'Com', 'For', 'From', 'How', 'In', 'Is', 'It', 'Of', 'On', 'Or', 'That', 'The', 'This', 'To', 'Was', 'The', 'And']

with open('UnifiedBookList.txt') as fin:
    for line_no, line in enumerate(fin):
        OriginalBooksList.append(line)
        line = re.sub(r'[^\w\s]', ' ', line)  # replace punctuation with space
        line = re.sub(' +', ' ', line)  # replace multiple space with one
        line = line.lower()  # to lower case
        if line.strip() and len(line.split()) > 2:  # line can not be empty and line must have more than 2 words
            booksAfterRemovingStopWords.append(' '.join([i for i in line.split(
            ) if i not in stopWords]))  # Remove Stop Words And Make Sentence

for line_no, line in enumerate(booksAfterRemovingStopWords):
    tokens = line.split(" ")
    output = list(ngrams(tokens, 3))
    temp = list()

    temp.append(OriginalBooksList[line_no])  # Adding original line
    for x in output:  # Adding n-grams
        temp.append(' '.join(x))
    booksWithNGrams.append(temp)

while booksWithNGrams:
    first_element = booksWithNGrams.pop(0)
    x = 0
    for mylist in booksWithNGrams:
        if set(first_element) & set(mylist):
            if x == 0:
                duplicatesSorted.append(first_element[0])
                x = 1
            duplicatesSorted.append(mylist[0])
            booksWithNGrams.remove(mylist)
    x = 0

with open('DuplicatesSorted.txt', 'w') as f:
    for item in duplicatesSorted:
        f.write("%s\n" % item)

The input is:
A Course of Pure Mathematics by G. H. Hardy
Agile Software Development, Principles, Patterns, and Practices by Robert C. Martin
Advanced Programming in the UNIX Environment, 3rd Edition
Advanced Selling Strategies: Brian Tracy
Advanced Programming in the UNIX(R) Environment
Alex's Adventures in Numberland: Dispatches from the Wonderful World of Mathematics by Alex Bellos, Andy Riley
Advertising Secrets of the Written Word: The Ultimate Resource on How to Write Powerful Advertising
Agile Software Development, Principles, Patterns, and Practices
A Course of Pure Mathematics (Cambridge Mathematical Library) 10th Edition by G. H. Hardy
Alex’s Adventures in Numberland
Advertising Secrets of the Written Word
Alex's Adventures in Numberland Paperback by Alex Bellos

The output is:
A Course of Pure Mathematics by G. H. Hardy

A Course of Pure Mathematics (Cambridge Mathematical Library) 10th Edition by G. H. Hardy

Agile Software Development, Principles, Patterns, and Practices by Robert C. Martin

Agile Software Development, Principles, Patterns, and Practices

Advanced Programming in the UNIX Environment, 3rd Edition

Advanced Programming in the UNIX(R) Environment

Alex's Adventures in Numberland: Dispatches from the Wonderful World of Mathematics by Alex Bellos, Andy Riley

Alex’s Adventures in Numberland

Alex's Adventures in Numberland Paperback by Alex Bellos

Advertising Secrets of the Written Word: The Ultimate Resource on How to Write Powerful Advertising

Advertising Secrets of the Written Word

By looking at the script, it seems to me that I have over complicated things. Please give some suggestion on how I can make this code better.

Ok, I tried to rearrange it a bit:

Corrected stop words (Should only contain lowercase words)
Used Jaccard method to calculate distances
Rearranged code structure
Rewrote it in Python3 with type annotations

You should now add an argument parser and that's basically it.
As far as I understood the task, the final goal was to remove same books.
Now you can play with a threshold argument to find out what strings should be treated the same.
import re
from typing import List, Callable, Set

from nltk.metrics.distance import jaccard_distance
from nltk.util import ngrams

def canonize(data: str) -> str:
    data = re.sub(r'[^\w\s]', ' ', data)  # replace punctuation with space
    data = re.sub(' +', ' ', data)  # replace multiple space with one
    return data.lower().strip()

def jaccard(book_a: str, book_b: str, n: int = 3) -> float:
    return 1 - jaccard_distance(set(ngrams(book_a, n)), set(ngrams(book_b, n)))

def filter_books(books: List[str],
                 book_filter_fun: Callable,
                 cmp_filter_func: Callable,
                 threshold: float = 0.3) -> Set[int]:
    excluded_indices = set()
    for one_book_offset, one_book in enumerate(books):
        if book_filter_fun(one_book):
            excluded_indices.add(one_book_offset)
        for another_book_offset, another_book in enumerate(books[one_book_offset + 1:], start=one_book_offset + 1):
            if {one_book_offset, another_book_offset} & excluded_indices:
                continue
            if cmp_filter_func(one_book, another_book) > threshold:
                excluded_indices.add(one_book_offset)
    return excluded_indices

if __name__ == '__main__':
    stopWords = {'i', 'a', 'about', 'an', 'are', 'as', 'at', 'be', 'by', 'com', 'for', 'from', 'how', 'in', 'is', 'it',
                 'of', 'on', 'or', 'that', 'the', 'this', 'to', 'was', 'the'}

    with open('UnifiedBookList.txt') as fin:
        original_books = fin.readlines()

    canonized_books = list(map(canonize, original_books))

    excluded_indices = filter_books(
        canonized_books,
        lambda book: len(book.split()) < 2,  # book name should contain not less than 2 words
        jaccard,
    )

    with open('DuplicatesSorted.txt', 'w') as fout:
        for i, book in enumerate(original_books):
            if i in excluded_indices:
                continue
            fout.write(book)


Data to FTP upload by stream copy I'd like to check the following process relating to the uploading of data by FTP, after the data is read from a database.

I have a stored procedure that returns data in the following pipe delimited format in a single column. It typically returns around 300 rows, each row has about 300 bytes.

1234|||||PROPERTY|||MARKET INDEX|||||ADDRESS|||||1||||GBP|||||||20110930||||||||C|OFFICE|F||

The application then submits that data by FTP, typically <100k overall.

Here's the mechanism used, restricted to .NET 3.5.

// database method returns dataset/datareader, converted and passed to method as
// IEnumerable<string> data

string ftpServerIP = "5.4.3.2:21";
string targetFileName = "mann.txt";
string username = "manfred";
string password = "*******";

Uri uri = new Uri(String.Format("ftp://{0}/{1}", ftpServerIP, targetFileName));
FtpWebRequest reqFTP = (FtpWebRequest)FtpWebRequest.Create(uri);
reqFTP.Credentials = new NetworkCredential(username, password);
reqFTP.Method = WebRequestMethods.Ftp.UploadFile;
reqFTP.KeepAlive = false;
reqFTP.UsePassive = false;

MemoryStream stIn = new MemoryStream();
using (StreamWriter sw = new StreamWriter(stIn))
{
    foreach (string item in data)
    {
        sw.WriteLine(item);
    }

    sw.Flush();

    using (Stream stOut = reqFTP.GetRequestStream())
    {
        stOut.Write(stIn.GetBuffer(), 0, (int)stIn.Length);
    }
}

FtpWebResponse response = (FtpWebResponse)reqFTP.GetResponse();
response.Close();

I'd like to check whether the stream and ftp processing looks ok, or whether there are other stream types to consider (BufferedStream?), or closure/disposal steps, etc...

In general it looks fine to me. BufferedStream would not help anything here, your data is already in memory by the time any streams get created.

I would prefer to see the MemoryStream in a using block, although in this case it is being disposed when the StreamWriter is disposed.

It may be a good idea to check the response and handle any error status. If you do ever add any code that uses the FtpWebResponse, I would change to a using block, instead of the call to close.

Not really related to the code, but avoid using FTP if you can.


Improving speed of word search algorithm I've been working on this word search algorithm and had some issues about its speed. It kinda looks like it takes a little too much time.

If anyone could offer suggestions on making this better and improving my skills, that would be great.

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

    using namespace std;

    bool Check(string Letter, string Matrix)
    {
        return (Letter == Matrix) ? true : false;

    }

    int main()
    {
        int Height = 0, Widht = 0, Numb = 0, Longer = 0, Look = 0, Look1 = 0, Look2 = 0, Look3 = 0, Poz = 0, Poz1 = 0;

        int Loop = 0, Loop1 = 0, Loop2 = 0, Loop3 = 0, Loop4 = 0, Loop5 = 0, Loop6 = 0;

        bool Found, Found1, Found2, Found3;

        ifstream Data("Files/Info.txt");

        Data >> Widht >> Height;

        string Matrix[Height][Widht];

        while(Loop < Height)
        {
            Loop1 = 0;

            while(Loop1 < Widht)
            {
                Data >> Matrix[Loop][Loop1];

                Loop1++;
            }

            Loop++;
        }

        Data >> Numb;

        string Words[Numb];

        while(Loop2 < Numb)
        {
            Data >> Words[Loop2];

            Loop2++;
        }

        Data.close();

        while(Loop3 < Numb)
        {
            Longer = Words[Loop3].size();
            Loop4 = 0;
            Loop6 = 0;

            cout << Words[Loop3] << endl;

            while(Loop4 < Height) // Looks horizantaly
            {
                Loop5 = 0;
                Look = 0;
                Look1 = 0;

                while(Loop5 < Widht - Longer + 1)
                {
                    Found = Check(Words[Loop3].substr(Look, 1), Matrix[Loop4][Loop5]);

                    if(Found)
                    {
                        Loop6 = 1;
                        Look = Loop5;
                        Poz = Loop4;
                        Poz1 = Loop5;

                        while(Loop6 < Longer)
                        {
                            Look++;

                            Found1 = Check(Words[Loop3].substr(Loop6, 1), Matrix[Loop4][Look]);

                            if(Found1)
                            {
                                Loop6++;
                            }
                            else
                            {
                                Loop5 += Loop6;
                                Loop6 = Longer + 1;
                            }
                        }
                    }

                    if(Loop6 == Longer)
                    {
                        cout << "Word " << Words[Loop3] << " was found at: " << Poz << " collumn " << Poz1 + 1 << " symbol. Horizontaly from right to left" << endl;
                        Loop5 = Widht;
                        Loop4 = Height;
                        Loop3++;
                    }

                    Loop5++;
                }

                Loop4++;
            }

            Loop3++;
        }

        return 0;
    }

The infos file looks like this:

10 10
a s d f g h j k l o
e t a k t r e e a d
t e d a w e f g d a
r e s a g a f a q w
d c s q a z x v n b
q w e t a s f w p s
w a r y i w t o o a
s b b d s s w q o k
w e r a d g d a p a
q w r c b a x l s k
3
tree
poop
test

Of course, I will later add other directions in which words might be written.

widht is misspelled; it should be width. :-)
Some generalities regarding naming convention:

Variables and functions should start with a lowercase letter:

// "camel case"
int camelCaseVariable;
void camelCaseFunction() {}

// "snake case"
int snake_case_variable;
void snake_case_function() {}

Custom types should start with a capital letter:

class Class;

Macros should be uppercase:

#define MACRO 0

Check()'s body can be simplified as such:

return Letter == Matrix; // already returns true or false

Its parameters should also be passed by constant reference since they're not being modified:

bool Check(string const& Letter, string const& Matrix) {}

You should make sure the input file is open before proceeding.  If not, terminate from main() by returning 1 or EXIT_FAILURE.
You could vastly improve readability by using more functions.  Putting everything in main() is not always a good idea, especially with a lot of nesting.  Define the functions to perform one important task, and call them in main() or wherever else as needed.  Function call overhead shouldn't hurt you here, making the readability aspect more important.

In its current state, more comments may help others follow the code.  That, in turn, could help flesh out any optimization issues.  However, this isn't a substitute for writing readable code.


Calculator-like application I'm attempting to implement a calculator-like application in C#. But the way I've currently implemented it seems a little messy and inefficient. It works, but it's just I'm pretty sure it can be achieved in a more elegant solution.

I looked to see if I could factor out any repeating code, such as the switch statement - but without having numerous parameters in the method, I don't think it'll make things much better.

Here's the main portion of the code:

private double Calculate()
{

    double answer = 0.0;
    foreach (double number in this.numbers)
    {
        int index = this.numbers.IndexOf(number);

        if (index != this.numbers.Count() - 1)
        {
            if (answer != 0.0)
            {
                switch ((int)operators[index])
                {
                    case (int)Operator.Plus:
                        answer = (double)(answer + numbers[index + 1]);
                        break;

                    case (int)Operator.Minus:
                        answer = (double)answer - numbers[index + 1];
                        break;

                    case (int)Operator.Divide:
                        answer = (double)answer / numbers[index + 1];
                        break;

                    case (int)Operator.Multiply:
                        answer = (double)answer * numbers[index + 1];
                        break;
                }
            }
            else
            {
                switch ((int)operators[index])
                {
                    case (int)Operator.Plus:
                        answer += (double)(number + numbers[index + 1]);
                        break;

                    case (int)Operator.Minus:
                        answer += (double)number - numbers[index + 1];
                        break;

                    case (int)Operator.Divide:
                        answer += (double)number / numbers[index + 1];
                        break;

                    case (int)Operator.Multiply:
                        answer += (double)number * numbers[index + 1];
                        break;
                }
            }
        }

    }

    return answer;
}

Just for reference, Operators is an ENUM that's defined as:

private enum Operator
{
    Plus = 1,
    Minus = 2,
    Multiply = 3,
    Divide = 4,
}

and the two lists:

private List<double> numbers;
private List<Operator> operators;

Any ideas on how I could improve this?

Well, first of all there is a bug in the code. You are getting the index of a number by looking for it in the list, but if the number occured earlier in the list you will get the wrong index. You should use the index in the loop instead of using a foreach.

You are using the Count() extension method to get the number of items in the list, but you should use the Count property instead.

You are casting all enum values to integers, but you can just use the enum values directly.

You are casting a lot of values to double, but they are already double values.

You are using the value of answer in the code where you have determined that it's always zero.

You are comparing the answer value to zero to determine if it's the first iteration, but that could occur later in the loop too.

Instead of handling the first iteration differently, you can just put the first number in the answer variable and start the loop from 1.

The code suddenly got a lot simpler. :)

private double Calculate() {
  double answer = numbers[0];
  for (int index = 1; index < numbers.Count; index++) {
    switch (operators[index - 1]) {
      case Operator.Plus:
        answer += numbers[index];
        break;
      case Operator.Minus:
        answer -= numbers[index];
        break;
      case Operator.Divide:
        answer /= numbers[index];
        break;
      case Operator.Multiply:
        answer *= numbers[index];
        break;
    }
  }
  return answer;
}


Name of language of translation I am unhappy with my entity-relation-namings.

The underliyng tables are named TRANSLATION and LANGUAGE.

To contains translations like

German is German in English
Deutsch is German in German
English is English in English
Englisch is English in German

I have the table

LANGUAGE
  ID (int pk)               <-- 1,2,3, ...
  LANGTAG (varchar)         <-- en / de / fr / ...
TRANSLATION
  LOCAL_NAME (varchar 64)   <-- German, Allemand, Deutsch, ...

Now I need relations between them. In the Entity i have both sides, the owning side and the non-owning-side.

I started using this relations:

You see two FKs:

Translates (getter in Translations) -> Translations (getter in Language)
Into language (getter in Translations) -> Language Translations (getter in Language)

But I inspect my Language-Entity now and see this:

private Set<Translation> translations = new LinkedHashSet<>();

@OneToMany(mappedBy = Translation.PROP_TRANSLATES)
public Set<Translation> getTranslations() {
    return translations;
}

public Language setTranslations(final Set<Translation> translations) {
    this.translations = translations;
    return this;
}

private Set<Translation> languageTranslations = new LinkedHashSet<>();

@OneToMany(mappedBy = Translation.PROP_INTO_LANGUAGE)
public Set<Translation> getLanguageTranslations() {
    return languageTranslations;
}

public Language setLanguageTranslations(final Set<Translation> languageTranslations) {
    this.languageTranslations = languageTranslations;
    return this;
}

I think the naming is problematic because I do not know what getter to use if I need the translations.

Intuitivly, to get the Languages that has been translated from German I use

Language german = entityManager.createCriteria("FROM Language WHERE langtag='de'");
Set<Translations> languagesInGerman = german.getLanguageTranslations();

The getter looks absolutly strange but I have no idea for a renaming!

(Proper answer from my comment in OP's question)

My proposal:

getLanguageNameInNativeLanguage

Other than that, there's a term for that: Endonym. See wiki: https://en.wikipedia.org/wiki/Exonym_and_endonym

Quote:
An endonym or autonym is an internal name for a geographical place, or a group of people, or a language or dialect.

For instance, "Germany" is the English language exonym, "Allemagne" is the French language exonym, and "Deutschland" is the endonym for the same country in Europe.

getLanguageEndonym() or something similar would look prettier, but a layman of the app doesn't understand it. If language is something important in the app, I would use the "academic" term.


Replacing thrown exception with more descriptive exception during file I/O My code needs to open a specific file. Under certain (expected, but rare) conditions, the code will get handed a file path to a file that does not exist -- the path is expected to be otherwise valid though.

If I get handed a path to a file that does not exist, this is an explicit failure scenario that could be reported to the logfile differently, with a more explicit exception message, thereby probably increasing the chance of analysing the error appropriately.

I have solved this by catching the thrown IOException and checking its type. If it is a FileNotFoundException or DirectoryNotFoundException I throw an InvalidOperationException with a message relating to how my code runs.

Here are the relevant parts from my code:

    public MyUpdater(...)
    {
        _reader = ...;
        try {
            // Open immediately to ensure we can read the file:
            _reader.AccessForReading();
        } catch (IOException ioex) {
            var extype = ioex.GetType();
            if (extype == typeof(FileNotFoundException) || extype == typeof(DirectoryNotFoundException)) {
                // This is an explicit failure scenario in which case we do not want the raw IOException to escape
                throw new InvalidOperationException("File to update does not exist! It may have been moved because of foobar reason xyz ...", ioex);
            } else {
                // re-throw all other
                throw;
            }
        }

Essentially, I'd like to replace the exception message with the likely reason for the error, so that support personnel looking at the logfile can quickly identify the problem.

Does this solution make sense?

Note:

This is not example code. It's actually copied verbatim out of the source, that's the very reason for the ellipsis, because that stuff there isn't relevant for the question.
By "injecting" I do mean what you see. The exception message (the one above is made up for posting it here) just gives a hint as to what may be the likely cause. This message should end up being the first related entry in the log file, thereby giving support personnel a first hint as to what may have gone wrong.)
One point that I'm not sure about specifically is using InvalidOperationException -- Since it's only for logging, the exception type is really not all that important, but should one create a separate exception type for such a scenario?

Give your code some room to breathe with a few new lines, they're free.

Before:

public MyUpdater(...)
{
    _reader = ...;
    try {
        // Open immediately to ensure we can read the file:
        _reader.AccessForReading();
    } catch (IOException ioex) {
        var extype = ioex.GetType();
        if (extype == typeof(FileNotFoundException) || extype == typeof(DirectoryNotFoundException)) {
            // This is an explicit failure scenario in which case we do not want the raw IOException to escape
            throw new InvalidOperationException("File to update does not exist! It may have been moved because of foobar reason xyz ...", ioex);
        } else {
            // re-throw all other
            throw;
        }
    }
}

After:

public MyUpdater(...)
{
    _reader = ...;

    try
    {
        // Open immediately to ensure we can read the file:
        _reader.AccessForReading();
    }
    catch (IOException ioex)
    {
        var extype = ioex.GetType();

        if (extype == typeof(FileNotFoundException) || extype == typeof(DirectoryNotFoundException))
       {
            // This is an explicit failure scenario in which case we do not want the raw IOException to escape
            throw new InvalidOperationException("File to update does not exist! It may have been moved because of foobar reason xyz ...", ioex);
        }
        else
        {
            // re-throw all other
            throw;
        }
    }
}

Secondly, catching, filtering and then rethrowing seems wasteful here, why not just catch the explicit exceptions you hope to catch?

public MyUpdater(...)
{
    _reader = ...;

    try
    {
        // Open immediately to ensure we can read the file:
        _reader.AccessForReading();
    }
    catch (FileNotFoundException ex)
    {
            // This is an explicit failure scenario in which case we do not want the raw IOException to escape
            throw new InvalidOperationException("File to update does not exist! It may have been moved because of foobar reason xyz ...", ex);
    }
    catch (DirectoryNotFoundException ex)
    {
            // This is an explicit failure scenario in which case we do not want the raw IOException to escape
            throw new InvalidOperationException("File to update does not exist! It may have been moved because of foobar reason xyz ...", ex);
    }
}

Granted, this breaks DRY a little, but it is significantly easier to read. You can reduce duplication further by extraction your exception creation to another method.

public MyUpdater(...)
{
    _reader = ...;

    try
    {
        // Open immediately to ensure we can read the file:
        _reader.AccessForReading();
    }
    catch (FileNotFoundException ex)
    {
            // This is an explicit failure scenario in which case we do not want the raw IOException to escape
            throw CreateUpdateFileMissingException(ex);
    }
    catch (DirectoryNotFoundException ex)
    {
            // This is an explicit failure scenario in which case we do not want the raw IOException to escape
            throw CreateUpdateFileMissingException(ex);
    }
}

public InvalidOperationException CreateUpdateFileMissingException(Exception ex)
{
    return new InvalidOperationException("File to update does not exist! It may have been moved because of foobar reason xyz ...", ex)
}

On the other hand this bonus is negated if there are a lot of exception types you want to catch, so if that list grows, then sticking with your filtering method may be the better course of action.

As for what exception you should be throwing here, it appears that the error is that the file cannot be found, so what's wrong with a FileNotFoundException with a different message?

Lastly, the comment // re-throw all other is simply describing what your code is doing. That is your code's job. Comments should explain why you have made certain choices. If the code is too complex to understand it should be refactored and comments explaining what code does should only be considered in extreme circumstances where refactoring is a poor choice.


Generic ObjectCache with Expiration I sometimes need an object that caches some data but only for the specified amount of time. So I created a class that should handle this. The main goal was to make it generic unlike the MemoryCache and be a single object cache and not a general all-pupose cache.
ObjectCache
public class ObjectCache<T>
{
    private readonly object _valueExpiredMutex = new object();

    private T _value;

    public ObjectCache(TimeSpan expiration)
    {
        Expiration = expiration;
    }

    public EventHandler<ObjectCacheEventArgs<T>> ValueExpired { get; set; }

    public TimeSpan Expiration { get; set; }

    public DateTime? LastUpdate { get; internal set; }

    public bool IsExpired
    {
        get { return !LastUpdate.HasValue || DateTime.Now - LastUpdate.Value > Expiration; }
    }

    public T GetValue()
    {
        if (!IsExpired)
        {
            return _value;
        }

        lock (_valueExpiredMutex)
        {
            _value = OnValueExpired();
            LastUpdate = DateTime.Now;
        }

        return _value;
    }

    private T OnValueExpired()
    {
        if (ValueExpired == null)
        {
            throw new InvalidOperationException("ValueExpired event handler not set.");
        }

        var e = new ObjectCacheEventArgs<T>();
        ValueExpired(this, e);
        return e.Value;
    }
}

LastUpdate has an internal setter so that I can set it in tests
I tried to implement it to be thread-safe but I'm not sure if I did it correctly with only one mutex for getting a new value.
Any kind of feedback is welcome ;-)

EventArgs
public class ObjectCacheEventArgs<T> : EventArgs
{
    public T Value { get; set; }
}

Test
[TestMethod]
public void TestObjectCache()
{
    var number = 1;

    var objectCache = new ObjectCache<int>(
        new TimeSpan(hours: 0, minutes: 21, seconds: 0));
    objectCache.ValueExpired += (sender, e) =>
    {
        e.Value = number++;
    };

    Assert.IsTrue(objectCache.IsExpired);
    Assert.AreEqual(1, objectCache.GetValue());
    Assert.IsFalse(objectCache.IsExpired);

    objectCache.LastUpdate = DateTime.Now.AddMinutes(-30);
    Assert.IsTrue(objectCache.IsExpired);
    Assert.AreEqual(2, objectCache.GetValue());
}

IMO this a strange aproach. One needs to first call GetValue() to be able to set a value by using an eventhandler. I would expect from such an ObjectCache to provide a constructor which takes a value as parameter too.

In addition I would add a property to ObjectCacheEventArgs<T> signaling a state, for instance an enum having the members ChangeValue, KeepAlive and SkipValue to be set by the handler of the event.
If this value is e.g set to ChangeValue then the _value should be changed. If it is set to KeepAlive the LastUpdate should be set to DateTime.Now. And for SkipValue you would need to add another var indicating that the value is skipped and set _value to default(T).

It isn't obvious from a caller side of view that you have to add an eventhandler for that event. IMO this is a bad sign which one would maybe notice first at running the code. Such behaviour shouldn't be wanted and isn't expected. If one does not add an eventhandler he expects that he just don't get notified if this event happens.


Detecting available List operations I'm writing some code which needs to take a generic java.util.List from another source and adapt to the available functions; eg. does it support add(), set(), etc.

I'm currently just calling the functions on the list and creating a set of available operations, but I'm wondering if there's a better way than try-catch.

Current code:

import java.util.*;

import javax.swing.JPanel;

public class JListPanel<T> extends JPanel {
    private static final long serialVersionUID = 832057842976869638L;

    private static enum JLPFlag {
        ADD, REMOVE, SET;
    }

    private List<T> list;
    private Set<JLPFlag> flags = EnumSet.allOf(JLPFlag.class);

    public JListPanel(List<T> backinglist) {
        list = backinglist;
        setFlags();
    }

    private void setFlags() {
        // store the last state
        List<T> prev = Collections.unmodifiableList(list);
        boolean empty = list.isEmpty();
        try {
            if (!empty) {
                list.add(list.get(0));
            } else {
                // hope that list allows null
                list.add(null);
            }
        } catch (Exception e) {
            flags.remove(JLPFlag.ADD);
        }
        try {
            list.set(0, list.get(0));
        } catch (Exception e) {
            flags.remove(JLPFlag.SET);
        }
        try {
            list.remove(0);
        } catch (Exception e) {
            flags.remove(JLPFlag.REMOVE);
        }
        if (flags.contains(JLPFlag.REMOVE))
            list.removeAll(list);
        if (flags.contains(JLPFlag.ADD))
            list.addAll(prev);
    }
}

Edit: This is supposed to be a JPanel that contains a list that can be modified by a program and this should reflect those changes on the list display. It's not complete yet, but the ultimate goal is to have a list that can be modified by the user or programmer. It will have some buttons that will be disabled if the list doesn't support the operations, so it might not have add or remove but it might have set.

I've compiled most of the suggestions into this new function: http://pastebin.com/JPWXdFfi

After edit original question:

I understand what you are trying to do, and very nice if you can do that.
That's why I'm trying to think together with you for an solution.
The only thing is that I seems not capable enough to reach to an solution.
That's why I'm still posting mine thoughts.

We have some problems(with solutions) that prevents us to check it at creation of the class :

We init with a list, and after constructing the "wrapper" it must be the same list.
The original list may support one or more methods, add may be supported and an addAll or remove maybe not,
Adding/removing a null can be supported or not. So if we want to test add we need a instance of T.
We can make the wrapper abstract and create an abstract T getInstance (); where we give a real instance of T or we disallow a wrapper with an empty List.This solves issue 3 but still not issue 2 when only add is supported we can't remove our T from the list. (reversed is not a problem cause it shall return false if T isn't in the list)
Maybe we could use the Object clone() what gives a shallow copy but that is in this case no problem as the copy is used to test the methods.Still a problem if it throws the CloneNotSupportedException.
We can at construction check if the CloneNotSupportedException is thrown and do the test at runtime => buttons are disabled when user press the button once or buttons are always disabled when cloning isn't possible.

Other bad solution :

Create an enum with all known implementations of List.
Check for instanceOf of the class and get the flags from the enum.

Why possible bad :

ArrayList implements everything.
When I create a subclass of ArrayList and throw at the add an UnsupportedOperationException your check for ArrayList is good and set the flags for an ArrayList.
Still the add operation shall not work.


Case statement in multiple methods My page contains: GridView1, GridView2, Button1, Button2, DropDownList1 I bind Gridviews to the table selected in dropdown like this:

Dim results as DataTable
Select Case ddl1.SelectedValue
    Case 0
        Dim cl as ClassZero = new ClassZero()
        results = cl.GetClassZeroNames()
    Case 1
           Dim cl as ClassOne = new ClassOne()
        results = cl.GetClassOneNames()
    Case 2
         Dim cl as ClassTwo = new ClassTwo()
        results = cl.GetClassTwoNames()
End Select
GridView1.DataSource = results
GridView1.DataBind()

Then I have two buttons with the following code:

Protected Sub btn1_Click(sender As Object, e As EventArgs) Handles btn2.Click
    Select Case ddl1.SelectedValue
        Case 0
             Dim cl as ClassZero = new ClassZero()
        results = cl.RunInsert()
        Case 1
             Dim cl as ClassOne = new ClassOne()
        results = cl.RunInsert()
        Case 2
             Dim cl as ClassTwo = new ClassTwo()
        results = cl.RunInsert()
    End Select
End Sub

And

Protected Sub btn2_Click(sender As Object, e As EventArgs) Handles btn2.Click
    Select Case ddl1.SelectedValue
        Case 0
             Dim cl as ClassZero = new ClassZero()
        results = cl.RemoveFromZero()
        Case 1
            Dim cl as ClassOne = new ClassOne()
        results = cl.RemoveFromOne()
        Case 2
            Dim cl as ClassTwo = new ClassTwo()
        results = cl.RemoveFromTwo()
    End Select
End Sub

For me it looks like a lot of overhead. How can I improve then design and only specify that I'm working on the following record in dropdown list without specifying the Case condition every time? Should I change my design or leave it like it is?

RunZero, RunOne, RunTwo, RemoveZero, RemoveOne, RemoveTwo, RemoveThree - Execute six different stored procedures.

You could use delegates. Set delegates in the Change event of ddl1, then you can always call the same delegates in your button handlers. This way you only need a switch in ddl1 Change event, and your button handlers will become much cleaner. However, even though this would make the code much more maintainable/readable, it'd probably be less efficient, though you still wouldn't be able to feel the difference in UI handlers.

Update: example added

Something along these lines (not copy&paste):

Public Delegate Function CreateClassDelegate() As NumberClass

Public Class Form
    Public CreateClass As CreateClassDelegate

    Private Sub ddl1_SelectedValueChanged(sender As Object, e As EventArgs)
        Select Case Integer.Parse(TryCast(ddl1.SelectedItem, String))
            Case 0
                CreateClass = New CreateClassDelegate(AddressOf ClassZero.CreateClassZero)
                Exit Select
            Case 1
                CreateClass = new CreateClassDelegate(AddressOf ClassOne.CreateClassOne);
                Exit Select
            Case 2
                CreateClass = new CreateClassDelegate(AddressOf ClassTwo.CreateClassTwo);
                Exit Select
        End Select
    End Sub

    Private Sub btn1_Click(sender As Object, e As EventArgs)
        Dim cl As NumberClass = CreateClass()
        cl.GetNames()
    End Sub

    Private Sub btn2_Click(sender As Object, e As EventArgs)
        Dim cl As NumberClass = CreateClass()
        cl.RunInsert()
    End Sub
End Class

Public MustInherit Class NumberClass
    Public MustOverride Function GetNames() As List(Of String)
    Public MustOverride Sub RunInsert()
    Public MustOverride Sub Remove()
End Class

Public Class ClassZero
    Inherits NumberClass
    Public Overrides Function GetNames() As List(Of String)
         ...
        Return Nothing
    End Function
    Public Overrides Sub RunInsert()
             ...
    End Sub
    Public Overrides Sub Remove()
         ...
    End Sub

    Public Shared Function CreateClassZero() As ClassZero
        Return New ClassZero()
    End Function
End Class

' ... same as above for ClassOne, ClassTwo


Adding/changing ID3 tags to media files with "artist - title" format This code puts or changes ID3 tags to media files based on their file names. It assumes the files are in this format:

morbid angel - maze of torment.mp3

The resulting tag would be:

Artist: morbid angel

Title: maze of torment

I use boost::filesystem to iterate through the directory and taglib to manipulate the tags.
#include <boost/filesystem.hpp>
#define TAGLIB_STATIC
#include <taglib/fileref.h>
#include <iostream>
#include <string>
#include <vector>

using namespace boost::filesystem;
using namespace TagLib;

void changeTag(const path& p) {
    std::string filename = p.filename().string();
    FileRef file(p.string().c_str());
    std::string artist = filename.substr(0,filename.find(" -"));
    Tag* t = file.tag();
    t->setArtist(artist);
    t->setTitle(filename.substr(artist.size()+3, filename.size()-artist.size()-7));
    file.save();
    std::cout << t->artist() << " - " << t->title() << " saved" << '\n';
}

void extractFiles(const std::string& p) {
    std::vector<path> files;
    std::vector<std::string> errors; // for files that might cause errors
    try {
        path filepath = p;
        for (auto& i : directory_iterator(filepath)) {
            changeTag(i.path());
        }
    } catch(const filesystem_error& e) {
        errors.push_back(e.what()); // output later after all files have been processed
    }
    if (errors.size() > 0) {
        std::cout << "Some operations failed with errors:" << '\n';
        for (auto& i : errors) {
            std::cout << i << '\n';
        }
    }
}

int main() {
    std::string dir;
    while (true) {
        std::cout << "Directory: Enter 'x' to close: ";
        std::cin >> dir;
        if (dir == "x") {
            break;
        }
        extractFiles(dir);
    }
}

How can I improve this code? How can I make this faster, cleaner and better?

Here are some things I see that may help you improve your code.

Add error checking

In your changeTag routine, a segfault can occur if there is no file. Add this just after the FileRef line:

if(!file.isNull() && file.tag()) {

Separate concerns

The code would be a little more clear if the changeTag routine was split into two parts. In particular, one could extract artist and title, and the other could apply those. It may also be reasonable to make extractFiles return the vector of errors rather than having it emit those errors to std::cout.  The calling program might be able to do something different with those, such as if it were a GUI.

Use command line parameters

Instead of prompting for a directory name, it would be better to have that as a command line parameter.  That would also simplify the control structure of your program as mentioned in the next comment.

Try to consolidate loop exit conditions at the top

Prefer a for loop with the actual exit condition to a while(true) loop.  Because the reader is forced to search for the loop terminating condition, it's more effort to understand the latter construct.  This code, for example could be structured like this:

int main() {
    std::string dir;
    std::cout << "Directory: Enter 'x' to close: ";
    for (std::cin >> dir; dir != "x"; std::cin >> dir) {
        extractFiles(dir);
    }
}

This also eliminates a break statement.  A break can also contribute to difficulties understanding a loop because again, the programmer reading the code has to search for them.

Consider the user

It might be nice to have an option for a "dry run" of the program.  That is, have it print what it would have done without actually altering any files.  This would help the user have confidence that the program was doing only and exactly what was intended.


Design considerations for classes/methods using JDBC to ensure resources closed correctly In reviewing code in our application, I see a tension between what makes a convenient design for methods accessing database data versus coding to ensure that the JDBC resource objects are closed properly.

An example problem:

// Application.java: Singleton class for objects which want to execute SQL queries

/**
 * Execute a query and return the Result. Provide a common point of
 * connection/statement handling
 *
 * @param sql
 *            SQL to execute
 * @return a ResultSet
 */
public ResultSet executeQuery(String sql) throws Exception {
    DBConnectionObj connectionObj = dbPoolMgr.getConnection();
    try {
        if (LOG.isDebugEnabled()) {
            LOG.debug("StateServer.executeQuery(): executing query=" + sql);
        }
        return connectionObj.executeQuery(sql);
    } catch (Exception e) {
        LOG.error("executeQuery(): error executing SQL=" + sql);
        throw e;
    } finally {
        dbPoolMgr.putConnection(connectionObj);
    }
}

// DBConnectionObj.java: JDBC Connection wrapper used in a connection pool

public ResultSet executeQuery(String sql, Connection connection) {
  try {
    if (Log4jUtils.isDebugLevel(LOG)) {
            LOG.debug("executeQuery(): " + sql);
        }
        return connection.createStatement().executeQuery(sql);
  } catch (SQLException e) {
        UnsuccessfulSQLExecutionResult result = new UnsuccessfulSQLExecutionResult(e, sql);
        LOG.error("executeQuery(): exception executing query. SQL info=" +        result.getContents());
        e.printStackTrace();
        return null;
  }
}

With above helper functions, other application classes can just pass the specific SQL and get back a reference to the ResultSet:

    ResultSet rs = Application.getInstance().executeQuery(getSql());
    while (rs.next()) {
            // process data
    }
    rs.close();

The problem with the DBConnectionObj.executeQuery() method, is that it creates a temporary Statement to execute the query, and this temporary object is never closed.  But also, because the method returns the ResultSet to the caller, it can't close the Statement until the caller is ready to close the ResultSet.  (The code doesn't need to close the Connection object, because it stays open for the lifetime of the application, and is part of a pool of open database connections.)

One solution would be to make sure every caller to the executeQuery() method, always does this:

rs.close();
rs.getStatement().close();

So the question is this: for usefulness, having a method which takes a SQL string for input and returns a ResultSet feels like a good design.  However, it imposes requirement that callers make sure to close both the ResultSet and the Statement, and it can't take advantage of the safer Java 7 try-close feature:

try(ResultSet rs = Application.getInstance().executeQuery(getSql())) {
    while (rs.next()) {
        // process data
    }
}  // rs is automatically closed, but not the Statement object

Any comments or suggestions on a better design are appreciated.

JDBC has always been a PITA when it comes to handling/closing resources.

One of the big advancements in the past while has been the advent of ConnectionPools and abstraction layers.... If you are inside a framwork like tomcat, or WebSphere, these are built in. Otherwise you can use things like Apache DBCP or C3PO. For both of these frameworks, when you close the Connections it closes any created Statements.

Using a pool like this allows you to trust in the pooling layer... but, I would still recommend that you use a 'clean' implementation for your code. What I have used in the past, and I think is the neatest method, is to have an abstract class like:

public abstract class DBCallable<T> implements Callable<T> {
    private final ConnectionPool pool;

    public DBCallable(ConnectionPool pool) {
        this.pool = pool;
    }

    public <T> call() throws SQLException {
        try (Connection con = pool.getConnection()) {
            return execute(con);
        }
    }

    protected abstract T execute(Connection con);
}

Then, you can do all sorts of things....

For example, you need to populate some widget:

final Widget widget = new Widget();
final ConnectionPool pool = .......

new DBCallable<Integer>(pool) {
    Integer execute(Connection con) {
        try (Statement stmt = con.createStatement()) {
            int count = 0;
            try (ResultSet rs = con.executeQuery("...")) {
                while (rs.next()) {
                    count++;
                    widget.doSomethingWith(rs.get(1));
                }
            }
            return count;
        }
    }
}

You can even supply one of those to an ExecutorService, and get a  Future from it, and then then run these things in alternate threads.

Very versatile.

Edit: If you would prefer to have (easy) access to the Statement instead of the Connection, create an abstract sub-class of the DBCallable:

public abstract class StmtCallable<T> extends DBCallable<T> {

    public DBCallable(ConnectionPool pool) {
        super(pool);
    }

    protected T execute(Connection con) {
        try (Statement stmt = con.createStatement()) {
            return query(stmt);
        }
    }

    protected abstract T query(Statement stmt);
}

Then, if you want a clean statement each time for your code, you can have

new StmtCallable<Integer>(pool) {
    Integer query(Statement stmt) {
        int count = 0;
        try (ResultSet rs = stmt.executeQuery("...")) {
            while (rs.next()) {
                count++;
                widget.doSomethingWith(rs.get(1));
            }
        }
        return count;

    }
}


Eliminate axis-aligned bounding box (AABB) variables I have an axis-aligned bounding box (AABB) that I've coded, but at the moment it is very redundant. I feel like there are variables that I can get rid of.  I need the AABB to be very fast, and I'd like it to be more lightweight than it is.

#pragma once

#include "Engine.h"
// Collision
#include "Vec2D.h"
// End Collision

#include "Renderer.h"

struct ENGINE_API AABB {
private:
    Vec2D min;
    Vec2D max;

    double x;
    double y;
    double width;
    double height;
    double xwidth; // x + width
    double yheight; // y + height

    double vertMid; // vertical mid, cached for quadtree
    double horzMid; // horizontal mid, cached for quadtree

    // Normalize
    void calcMid() {
        vertMid = x + (width / 2);
        horzMid = y + (height / 2);
        xwidth = x + width;
        yheight = y + height;
    }

    void init(Vec2D, Vec2D);
    // End Normalize
public:
    // Constructor
    AABB(Vec2D min, Vec2D max) {
        init(min, max);
    }

    AABB(double x, double y, double width, double height){
        init(Vec2D(x, y), Vec2D(x + width, y + height));
    }
    // End Constructor

    // Getters
    double getX()const {
        return x;
    }

    double getY()const {
        return y;
    }

    double getWidth()const {
        return width;
    }

    double getHeight()const {
        return height;
    }

    double getHorzMid()const {
        return horzMid;
    }

    double getVertMid()const {
        return vertMid;
    }

    double getXWidth()const {
        return xwidth;
    }

    double getYHeight()const {
        return yheight;
    }
    // End Getters

    // Base
    bool collides(AABB other) {
        if (xwidth < other.x || x > other.xwidth) { return false; }
        if (yheight < other.y || y > other.yheight) { return false; }

        return true;
    }

    bool contains(Vec2D point) {
        if (x > point.getX() || xwidth < point.getX()) { return false; }
        if (y > point.getY() || yheight < point.getY()) { return false; }

        return true;
    }

    void move(double x, double y) {
        this->x = x;
        this->y = y;
        calcMid();
    }
    // End Base
    // Testing
    void render(Renderer& renderer) {
        sf::RectangleShape shape;
        shape.setPosition(x, y);
        shape.setOutlineColor(sf::Color::Red);
        shape.setOutlineThickness(3);
        shape.setSize(sf::Vector2f(width, height));
        renderer.render(shape);
    }
    // End Testing
};

And the messy part:

void AABB::init(Vec2D one, Vec2D two) {
    double x1 = one.getX();
    double y1 = one.getY();
    double xx = two.getX();
    double yy = two.getY();

    if (x1 > xx) {
        if (y1 > yy) {
            min = Vec2D(xx, yy);
            max = Vec2D(x1, y1);
        }
        else {
            min = Vec2D(xx, y1);
            max = Vec2D(x1, yy);
        }
    }
    else {
        if (y1 > yy) {
            min = Vec2D(x1, yy);
            max = Vec2D(xx, y1);
        }
        else {
            min = Vec2D(x1, y1);
            max = Vec2D(xx, yy);
        }
    }

    x = min.getX();
    y = min.getY();
    width = max.getX() - x;
    height = max.getY() - y;

    calcMid();
}

In a comment in your other question you claimed,

  Thank you, Caching vertMid and horzMid really helped.

Whether caching is helpful depends on:

How often you used the cached data
How tight your performance requirements are
Whether your cache implementation is reliable

One of your comments says,

// vertical mid, cached for quadtree

Only your testing can show whether that optimization is helpful.

From a performance point of view, in general:

If it's not used it's harmful to cache
If it's used once then it's harmless/useless to cache
If it's used several times then it's useful to cache

However making something bigger (by caching extra values) can also have some negative effect on performance, e.g. because you can't fit so many objects in the CPU cache (which is faster than memory): which is why you should profile/test to see whether caching makes it faster in practice as well as in theory.

If you want to eliminate duplicate variables, you only need 4 doubles:

min and max
or, x and either width or xwidth, and y and either height and yheight

I found your xwidth and yheight names confusing to read; I would have preferred 'right' and 'bottom'.

If you have large structs, you may improve performance by curing your habit of passing objects by value instead of by reference. For example, this ...

bool collides(AABB other)

... should be ...

bool collides(const AABB& other) const

You can also use different classes for different purposes. For example, the AABB bounds member of your Quadtree class needs getHorzMid() and getVertMid() members, but those methods aren't needed/used by the AABB which you return using Locatable::getAABB(). So you could use two classes, e.g. AABB (without getHorzMid and getVertMid members), and a SplittableAABB class (used as a member of Quadtree) which adds those members.

I don't much like AABB as a class name: partly because all-upper-case looks like a macro to me: consider Bounds or Rect or Rectangle instead.

Your current code creates but doesn't read the initialize the min and max data members.

Your move method doesn't update the data in these members (rendering that data untrue).

Maybe Vec2D min and Vec2D max should be (temporary) local variables of your init function, not (permanent) member data of the class.

The parameters passed to the AABB(Vec2D min, Vec2D max) constructor should probably be named one and two not min and max (and should possibly be passed by reference not by value).


Haskell basic typeclass primer (quadratic equations) I have written Haskell code to my spec to help me grasp types and syntax, because I am learning.
I hope to generalize it further to accommodate more kinds of equations. I am (perhaps prematurely) worried that repeated usage of delta in multiple places is going to slow down my code, but I don't see how I can avoid it.
I am a bit confused about whether I should use Floating class or just use Float concrete type everywhere.
I appreciate any and all tips.
Here's the expected output
Equation -10.0x^2 +4.0x +1.0 = 0 has following 2 solutions:
0.28708285
-8.708288e-2

And the code
-- MathPrimer

delta :: Floating a => a -> a -> a -> a
x1 :: Floating a => a -> a -> a -> a
x2 :: Floating a => a -> a -> a -> a

delta a b c = b * b - 4 * a * c

x1 a b c = ( - b - sqrt(delta a b c)) / ( 4 * a * c )
x2 a b c = ( - b + sqrt(delta a b c)) / ( 4 * a * c )

class Algebraic equ where
    algebraicForm :: equ -> String

class (ZeroEquation equ) where
    solve :: equ -> [Float]
    solutionCount :: equ -> Int

data Equation = Quadratic {a :: Float, b :: Float, c :: Float}
    deriving (Show, Read, Eq)

instance (ZeroEquation Equation) where
    solve (Quadratic a b c) = solutions
        where
            both = [x1 a b c, x2 a b c]
            one = [x1 a b c]
            zero = []
            solutions = if _delta < 0 then zero else if _delta == 0 then one else both
            _delta = (delta a b c)
    solutionCount (Quadratic a b c) = count
        where
            count = if _delta < 0 then 0 else if _delta == 0 then 1 else 2
            _delta = (delta a b c)

instance (Algebraic Equation) where
    algebraicForm (Quadratic a b c) = show a ++ "x^2 +" ++ show b ++ "x +" ++ show c

showEquation :: Equation -> String
showEquation equ = "Equation " ++ description ++ " = 0 has following " ++ count ++ " solutions:\n" ++ solutions
    where
        count = show $ solutionCount equ
        description = algebraicForm equ
        solutions = unlines $ map show $ solve equ

main = do
    let equation = Quadratic (-10) 4 1
    putStrLn $ showEquation equation

It's hard to tell if the compiler would be able to eliminate duplicate calls to delta. It may do that, by inlining the x1 and x2 functions and then eliminating common subexpressions. Personally though, I prefer to do this myself. The following modification is guaranteed to call delta only once:
solve (Quadratic a b c) = solutions
    where
        both = [x1, x2]
        one = [x1]
        zero = []
        solutions = if _delta < 0 then zero else if _delta == 0 then one else both
        _delta = (delta a b c)
        x1 = ( - b - sqrt _delta) / ( 4 * a * c )
        x2 = ( - b + sqrt _delta) / ( 4 * a * c )


Generalize code that populates MFC list control? I'm using the Model View Presenter (MVP) pattern in MFC that's not hugely relevant to my question but I mention it for context.  I have some code that is designed to populate a list control with data from a list.

I have two list boxes which have almost the same code except the type of list is different and the function used to actually put items in the list control is different.  So my questions are:

How can I genericize this member function so that I could pass it an arbitrary list of objects, and a population function (i.e., what AddCustomerToListControl is)?
Besides genericizing it are there other improvements to be made?

(Note the calls to SetTopIndex are so that the scroll position is remembered.)

void MyDialog::SetCustomerList(const std::list<Customer*>& customers)
{
    customersListCtrl_.SetRedraw(FALSE);

    int selectedItemIndex = customersListCtrl_.GetNextItem(-1, LVNI_SELECTED);
    int topIndex = customersListCtrl_.GetTopIndex();

    customersListCtrl_.DeleteAllItems();

    if(customers.size() > 0)
    {
        for(std::list<Customer*>::const_iterator it = customers.begin();
                it != customers.end() ; it ++)
        {
            AddCustomerToListControl(*it);
        }
        customersListCtrl_.SetItemState(selectedItemIndex,LVIS_SELECTED, LVIS_SELECTED);
    }

    customersListCtrl_.SetTopIndex(topIndex);
    customersListCtrl_.SetRedraw(TRUE);
}

UPDATE

Another detail is the the function AddCustomerToListControl above needs a reference to the listControl in order to add items to it (e.g., call InsertItem and SetItemText)

To Generalize it:

class MyDialog
{
    template<typename T, typename A>
    void SetCustomerList(T const& customers, A action);
    ....
};

template<typename T, typename A>
void MyDialog::SetCustomerList(T const& customers, A action)
{
    .....
    // AddCustomerToListControl(*it);
    action(*it);
    .....
}

// At call point:
SetCustomerList(customers, &AddCustomerToListControl);

Other Changes:
You can use the standard algorithms:

    for(std::list<Customer*>::const_iterator it = customers.begin();
            it != customers.end() ; it ++)
    {
        AddCustomerToListControl(*it);
    }

Can be replaced with:

    std::for_each(customer.begin(), customer.end(), AddCustomerToListControl);
    // or in the new code:
    std::for_each(customer.begin(), customer.end(), action);


C# Security Module for generating secure hashes Any pointers on the level of security this hashing function perform at?
I have never had to really write my own modules for security before.

public static class Hashing
{

    public static int SaltSize = 32;

    public static string HashPassword(string passwordToHash, byte[] salt, int numberOfRounds)
    {
        var hashedPassword = HashPassword(Encoding.UTF8.GetBytes(passwordToHash), salt, numberOfRounds);

        return Convert.ToBase64String(salt.Concat(hashedPassword).ToArray());
    }

    public static string HashPassword(string passwordToHash, int numberOfRounds)
    {
        var salt = GenerateSalt();
        var hash =  HashPassword(passwordToHash, salt, numberOfRounds);

        return hash;
    }
    public static byte[] GenerateSalt()
    {
        using (var randomNumberGenerator = new RNGCryptoServiceProvider())
        {
            var randomNumber = new byte[SaltSize];
            randomNumberGenerator.GetBytes(randomNumber);

            return randomNumber;
        }
    }
    public static byte[] HashPassword(byte[] toBeHashed, byte[] salt, int numberOfRounds)
    {
        using (var rfc2898DeriveBytes = new Rfc2898DeriveBytes(toBeHashed, salt, numberOfRounds))
        {
            return rfc2898DeriveBytes.GetBytes(SaltSize);
        }
    }
}

public abstract class HashVerification
{
    public abstract string GetDatabaseHash(string username);

    public bool HashMatch(string username, string password)
    {
        var combinedHash = GetDatabaseHash(username);

        var salt = Convert.FromBase64String(combinedHash).Take(Hashing.SaltSize).ToArray();

        var serverHash = Hashing.HashPassword(password, salt, username.Length * 100);

        return combinedHash.Equals(serverHash);
    }

}

public static class Hashing
{

    public static int SaltSize = 32;

This is public, static, and not const or readonly. That means:

"Anyone" can change it.
It's not thread-safe.
Hash validation is undermined.

I see no reason for it to be public, and every reason for it to be readonly or const. Personally I'd favour readonly and initialised from ConfigurationManager.AppSettings so that an upgrade path can be built in which doesn't require recompiling the code.

    public static string HashPassword(string passwordToHash, byte[] salt, int numberOfRounds)
    ...
    public static string HashPassword(string passwordToHash, int numberOfRounds)
    ...
    public static byte[] GenerateSalt()
    ...
    public static byte[] HashPassword(byte[] toBeHashed, byte[] salt, int numberOfRounds)

Why expose everything? I would expect the only public method to be HashPassword(string password) (or maybe HashPassword(SecureString password), although SecureString is such a nuisance to do anything with that I can understand not supporting it).

        return Convert.ToBase64String(salt.Concat(hashedPassword).ToArray());

Cryptography is an arms war: it's always worth thinking about the upgrade path. That means recording the parameters (name of hash, number of iterations, length of salt) in such a way that they can be extracted when it's time to validate.

        using (var randomNumberGenerator = new RNGCryptoServiceProvider())

I think it's better practice to use RandomNumberGenerator.Create() and leave the selection of the provider to the system-wide configuration.

        using (var rfc2898DeriveBytes = new Rfc2898DeriveBytes(toBeHashed, salt, numberOfRounds))

RFC 2898 was intended as a key derivation function, not a password validation mechanism. It's probably safe, but I'm leery of using it for a different purpose without including a reference to a respected cryptographer who says it's safe.

public abstract class HashVerification
{
    public abstract string GetDatabaseHash(string username);

    public bool HashMatch(string username, string password)

The guideline that one should favour composition over inheritance suggests that hash verification (HashMatch) should probably be in Hashing, and the database fetching should be independent of it.

        var salt = Convert.FromBase64String(combinedHash).Take(Hashing.SaltSize).ToArray();

        var serverHash = Hashing.HashPassword(password, salt, username.Length * 100);

Yikes! Why should people with shorter usernames get less protection? The number of rounds should be another thing that comes from a configuration file so that it can be increased over time, and it should be constant for everyone.

        return combinedHash.Equals(serverHash);

Already addressed in an earlier answer.


Naming a not-quite-flatMap method I recently created Argyle, a small library for parsing command-line arguments in Scala. I'd be happy for anyone to take a look and provide general feedback on it. But there is also one specific issue that sparked some discussion when I posted on reddit, so I would be interested to hear the thoughts of folks here also. Basically, I have the following trait:

trait Arg[+A] {
  def parse(xs: List[String], mode: ArgMode): Try[A]
  def flatMap[B](f: A => Try[B]): Arg[B]
}

The idea is that an Arg[A] handles parsing (and potentially failing to parse) a list of strings into an A. The issue is over the name of the flatMap method. It's a useful method, for instance, converting an Arg[String] to Arg[Int] by .flatMap(s => Try(s.toInt)). But it's not a valid FP Monad flatMap since the parameter type is A => Try[B] and not A => Arg[B].

If you view Arg as a function List[String] => Try[A], then it is actually pretty identical to .andThen(_.flatMap(f)). But I don't know if there's a canonical FP name for an operation like this.

In my mind, pros of naming it flatMap:

Similar enough to other flatMap methods that it gives some immediate understanding of how it should function
Enables for-comprehension with chains of Trys and such, that work as expected

Cons:

Confusing for those who expect flatMap always implies monad, since Arg is not a monad.
Potentially confusing that for-comprehensions can't use chains of Args, but that seems nonsensical anyway, and probably shouldn't be allowed.

Any other thoughts, or suggestions for a better name? There is also a map method to deal with.

I would consider creating an Arrow type from Arg[A] to Arg[B], something like:

import scalaz.Arrow // Also available in Cats

case class ArgArrow[A, B](run: Arg[A] => Arg[B])

object ArgArrow {
  implicit val arrowInstance: Arrow[ArgArrow] = new Arrow[ArgArrow] {
    def arrTry[A, B](f: A => Try[B]): ArgArrow[A, B] = ???
    def arrOption[A, B](f: A => Option[B]): ArgArrow[A, B] = ???
    // implement other required functions
  }
}

With Arrow, it will be very easy to assemble different things together, for example, you can easily combine an A => B, an A => Try[C], and an A => Option[D] into an ArgArrow[A, (B, C, D)].


Duplicating Rails model for a form with meta-programming Can someone please review my code:

# dup/ish -> //github.com/rails/rails/issues/5449

class Form
  include Mongoid::Document

  attr_accessor :source

  def self.ish(prefix, suffix, &block)
    klass_name = prefix.classify + suffix

    dup.tap do |klass|
      klass.class_eval do
        define_singleton_method(:name) do
          prefix.classify
        end
      end

      klass.class_eval(&block)

      Object.const_set(klass_name, klass)
    end
  end

  def self.compose(klass_name, id, source)

    mongoid_translate = {:models => {}, :attributes => {}}

    form = ish(klass_name, id) do

      translate = {}

      source.flatten.each do |widget|
        if widget.include?('validate')
          validates *widget.values_at('name', 'validate')
          translate.store *widget.values_at('name', 'translate')
        end

        if widget.include?('name')
          if 'upload' == widget['type']
            has_many(:attachments)
            accepts_nested_attributes_for(:attachments)
          elsif 'repeat' == widget['type']
            part_klass = Form.compose(widget['name'], id, widget['parts'])
            embeds_many(widget['name'].pluralize, :class_name => part_klass.class.to_s)
            accepts_nested_attributes_for(widget['name'].pluralize)
          end
        end
      end

      mongoid_translate[:attributes][self.name.downcase.to_sym] = translate
    end

    I18n.backend.store_translations(:pt_BR, {:mongoid => mongoid_translate})

    form.new.tap do |form|
      form.source = source
    end
  end

  def prepare
    self.source.flatten.each do |widget|
      if widget.include?('name')
        if 'repeat' == widget['type']
          parts = send("#{widget['name'].pluralize}")

          if parts.empty?
            parts.build

            widget['parts'].each do |part|
              parts[0].write_attribute(part['name'], nil)
            end
          end
        elsif 'upload' == widget['type']
          attachments.build if attachments.empty?
        else
          begin
            send(widget['name'].to_sym)
          rescue NoMethodError
            write_attribute(widget['name'], nil)
          end
        end
      end
    end
  end
end

Few things I have noted in your design are listed below. Beware all of this is purely from a design perspective rather than application.

Source is defined as attr_accessor which essentially violates encapsulation. Inject it from outside in constructor call.(I am not able to understand why you have not gone for attr_reader instead of accessor.
Ideally self.compose can be split into different method calls. It is violating SRP by doing too many things at one place. That is same for other methods as well.
Speaking from a puritan view, the branched if condition is also a smell.

In general, metaprogramming is considered to be something which must be avoided unless you have enough justification to keep it.


Format the output of Color.ToString() to RGB values The method that I wrote below functions properly; however, I believe that the code I wrote is not clean nor "good".

I know the basics of regex but I do not know more advanced techniques as I am still an amateur at programming.

static string SimpleColorOutput(Color color)
{
    //If input = Color[A=255, R=8, G=14, B=21]

    string colorText = color.ToString();

    Match R = Regex.Match(colorText, @"R=(\d+)[,\]]");
    Match G = Regex.Match(colorText, @"G=(\d+)[,\]]");
    Match B = Regex.Match(colorText, @"B=(\d+)[,\]]");

    return $"R = {R.Groups[1].Value}, G = {G.Groups[1].Value}, B = {B.Groups[1].Value}";

    //The method returns "R = 7, G = 13, B = 20"
}

Also, is it better to just pass in the Color object

The code in question is using the wrong tool (Regex) to achieve what you want. Using the string representation of a Color to get a string back is not a good way.

If one would pass a predefined color like Color.Red into that method it would fail by returning R = , G = , B = because the Color.ToString() returns

  A string that is the name of this Color, if the Color is created from
  a predefined color by using either the FromName(String) method or the
  FromKnownColor(KnownColor) method; otherwise, a string that consists
  of the ARGB component names and their values.

If this is just for learning Regex you could change

string colorText = color.ToString();

to

string colorText = Color.FromArgb(color.R, color.G, color.B).ToString();

otherwise it would be better to just write

static string SimpleColorOutput(Color color)
{
    return $"R = {color.R}, G = {color.G}, B = {color.B}";
}


Loading, parsing, and displaying a JSON file I recently have been working on loading a local JSON file parsing it and displaying it using Javascript and I think I got it to where I really like it. I would like to know if anyone knows if there is a "purer" way of doing this using just Javascript without libraries.

<!DOCTYPE html>
<meta charset="UTF-8" />
<input type="file" id="files" name="files" accept=".json"/>
<output id="list"></output>

handleFileSelect is an Impure Function
handleFileSelect has in your code snippet a dependence to fr which is decleraded outside of the function. To make the function pure you can pass fr as argument.
// impure..
function handleFileSelect (evt) {
    fr.readAsText(evt.target.files[0])
}

// pure
function handleFileSelect (fr, evt) {
    fr.readAsText(evt.target.files[0])
}

Put Side Effects Into a Wrapper

A pure function returns for the same input always the same output.

The function writeInfo contains side effects because we access the DOM. We could wrap the DOM functions in to a wrapper called IO which will always return a function - always the same function.

const IO = x => ({
    map: (fn) => IO(x(fn)),
    run: () => x()
})

// more about compose and the benefits on
// https://medium.com/javascript-scene/composing-software-an-introduction-27b72500d6ea
const compose = (f, g) => x => f(g(x))

const getElementById = id => IO(() => document.getElementById(id))
const writeInnerHTML = value => element => IO(() => element.run().innerHTML = value)

const selectAndWrite = value => compose(
  writeInnerHTML(value),
  getElementById
)

const writeInfo = data => IO(() => {
  selectAndWrite('Traveler: ' + data.traveler_num)('traveler_num').run()
  selectAndWrite('First Name: ' + data.first_name)('first_name').run()
})

// ONLY this call is impure
writeInfo({
  traveler_num: 500,
  first_name: 'Steven'
}).run()

Either You Can Parse to a JSON or Not..
In the following I want to introduce a functor calles Either which is split into to functors Left and Right. If we can parse a string succesfully we will return a Right with the parsed value otherwise we will return Left with an error message.

const Left = x => ({
  map: (fn) => Left(x),
  get: () => x
})

const Right = x => ({
  map: (fn) => Right(fn(x)),
  get: () => x
})

const tryFunction = (errorMessage, fn) => x => {
  try {
    return Right(fn(x))
  } catch (error) {
    return Left(errorMessage)
  }
}

const parseToJson = tryFunction('Can\'t pase to JSON', JSON.parse)

console.log('should be {"name": "Random Name"}:', parseToJson('{"name": "Random Name"}').get())
console.log('should be "Can\'t pase to JSON":', parseToJson('{name: Random Name}').get())

References
The free online book mostly-adequate-guide is very good
The series composing-software is a good way to start


Adding more responsibility to this endgame-analyzer class I have written a Tic-Tac-Toe game.  I have a class called GameEndAnalyzer that will check if the game has already ended or not.

Would it violate the Single Responsibility Principle if I will add more responsibilities to this class to not only decide if someone has won the game, but who has won (X or O)?  What is the best way to add responsibility to this?

public interface IGameEndAnalyzer
{
    bool IsGameEnd(IBoard board);
}
public class GameEndAnalyzer : IGameEndAnalyzer
{
    public bool IsGameEnd(IBoard board)
    {
        return IsAnyRowHasAllSameValue(board) ||
               IsAnyColumnHasAllSameValue(board) ||
               IsLeftDiagonalHasAllSameValue(board) ||
               IsRightDiagonalHasAllSameValue(board);
    }

    private bool IsAnyRowHasAllSameValue(IBoard board)
    {
        for (int rowIndex = Board.Row.Top; rowIndex <= Board.Row.Bottom; rowIndex++)
        {
            var isSameValue = board
                       .Cells
                       .WhereInRow(rowIndex)
                       .AllCellIs_O_Or_X();

            if (isSameValue)
            {
                return true;
            }
        }

        return false;
    }

    private bool IsAnyColumnHasAllSameValue(IBoard board)
    {
        for (int columnIndex = Board.Column.Left; columnIndex <= Board.Column.Right; columnIndex++)
        {
            var isSameValue = board
                       .Cells
                       .WhereInRow(columnIndex)
                       .AllCellIs_O_Or_X();

            if (isSameValue)
            {
                return true;
            }
        }

        return false;
    }

    private bool IsLeftDiagonalHasAllSameValue(IBoard board)
    {
        var isSameValue = board
                          .Cells
                          .WhereInDiagonalLeft()
                          .AllCellIs_O_Or_X();

        return isSameValue;
    }

    private bool IsRightDiagonalHasAllSameValue(IBoard board)
    {
        var isSameValue = board
                          .Cells
                          .WhereInDiagonalRight()
                          .AllCellIs_O_Or_X();

        return isSameValue;
    }
}

You're using extension methods for cells; why do use for loops for rows? It strikes me as weird.  Also, there's plenty of repetition in your analyzer; you can do better by abstracting some things away (more on that below).

To answer your main question, instead of returning bool for whether somebody won, you can return Nullable<Player> where Player is an enum with X and O values. This would not in any way violate single responsibility principle because your class' responsibility is to analyze the game, and determine the winner. There is no point in finding out if somebody has won, if you don't know who that somebody is.

That being said, I think you could still do better with the implementation. Your GameEndAnalyzer analyzes each outcome manually, whereas you could've made it more generic by introducing a concept of “lines” and a few extension methods (somewhat less than you seem to have).

I re-wrote my earlier toy Tic Tac Toe implementation specifically to address your question, with the following separation:

Player is a simple enum:

enum Player {
    X,
    O
}

IBoard represents a board with an indexer by row and column and Size.

interface IBoard
{
    int Size { get; }
    Player? this [int row, int column] { get; set; }
}

(really, you don't need anything else in IBoard)
The implementation is trivial:

class Board : IBoard
{
    Player? [,] _cells;

    public int Size {
        get { return _cells.GetLength (0); }
    }

    public Board (int size)
    {
        _cells = new Player? [size, size];
    }

    public Player? this [int row, int column] {
        get {
            return _cells [row, column];
        } set {
            if (_cells [row, column].HasValue)
                throw new InvalidOperationException ("The cell is already claimed.");

            _cells [row, column] = value;
        }
    }
}

IBoardAnalyzer has a single method that scans the board to find the winner, similarly to yours:

interface IBoardAnalyzer
{
    Player? DetermineWinner (IBoard board);
}

All the actual heavy-lifting for scanning the board lives in BoardExtensions that can enumerate board rows, columns and diagonals. There is also an über-method called SelectAllLines that returns a sequence of all “lines” (rows, columns and diagonals) on the board. Note that BoardExtensions does no analysis; it only provides convenience methods for extracting data out of Board.

static class BoardExtensions
{
    public enum DiagonalKind {
        Primary,
        Secondary
    }

    public static IEnumerable<IEnumerable<Player?>> SelectAllLines (this IBoard board)
    {
        return board.SelectAllDiagonals ()
            .Concat (board.SelectAllRows ())
            .Concat (board.SelectAllColumns ());
    }

    public static IEnumerable<IEnumerable<Player?>> SelectAllRows (this IBoard board)
    {
        return from row in board.SelectIndices ()
               select board.SelectRow (row);
    }

    public static IEnumerable<IEnumerable<Player?>> SelectAllColumns (this IBoard board)
    {
        return from column in board.SelectIndices ()
               select board.SelectColumn (column);
    }

    public static IEnumerable<IEnumerable<Player?>> SelectAllDiagonals (this IBoard board)
    {
        return from kind in new [] { DiagonalKind.Primary, DiagonalKind.Secondary }
               select board.SelectDiagonal (kind);
    }

    public static IEnumerable<Player?> SelectRow (this IBoard board, int row)
    {
        return from column in board.SelectIndices ()
               select board [row, column];
    }

    public static IEnumerable<Player?> SelectColumn (this IBoard board, int column)
    {
        return from row in board.SelectIndices ()
               select board [row, column];
    }

    public static IEnumerable<Player?> SelectDiagonal (this IBoard board, DiagonalKind kind)
    {
        return from index in board.SelectIndices ()
               let row = index
               let column = (kind == DiagonalKind.Primary)
                   ? index
                   : board.Size - 1 - index
               select board [row, column];
    }

    public static IEnumerable<int> SelectIndices (this IBoard board)
    {
        return Enumerable.Range (0, board.Size);
    }
}

The implementation for BoardAnalyzer uses BoardExtensions to find the winner, but in itself is trivial:

class BoardAnalyzer : IBoardAnalyzer
{
    public Player? DetermineWinner (IBoard board)
    {
        return (
            from line in board.SelectAllLines ()
            let winner = DetermineLineWinner (line)
            where winner.HasValue
            select winner
        ).FirstOrDefault ();
    }

    static Player? DetermineLineWinner (IEnumerable<Player?> line)
    {
        try {
            return line.Distinct ().Single ();
        } catch (InvalidOperationException) {
            return null;
        }
    }
}

Finally, I implemented an IO which has a simple interface:

interface IGameIO
{
    Tuple<int, int> AskNextMove (Player player, IBoard board);
    void DisplayError (GameError error);
    void DisplayWinner (Player player);
}

And just as simple implementation:

class ConsoleGameIO : IGameIO
{
    public Tuple<int, int> AskNextMove (Player player, IBoard board)
    {
        Console.WriteLine ("\n\n");
        Console.WriteLine ("{0}, what is your move?\n", FormatPlayer (player));
        Console.WriteLine (FormatBoard (board));
        Console.Write ("\nType A1 to C3: ", FormatPlayer (player));

        return ParseMove (Console.ReadLine ().Trim ().ToUpperInvariant ());
    }

    public void DisplayError (GameError error)
    {
        switch (error) {
        case GameError.CellAlreadyOccupied:
            Console.WriteLine ("Bad move: cell is already occupied.");
            break;
        case GameError.CouldNotParseMove:
            Console.WriteLine ("Bad move. Valid moves are A1 to C3.");
            break;
        default:
            Console.WriteLine ("Something went wrong.");
            break;
        }
    }

    public void DisplayWinner (Player player)
    {
        Console.WriteLine ("Congatulations, {0}! You won.", FormatPlayer (player));
    }

    static string FormatBoard (IBoard board)
    {
        return string.Join ("\n", from row in board.SelectAllRows () select FormatRow (row));
    }

    static string FormatRow (IEnumerable<Player?> row)
    {
        return string.Join ("|", from cell in row select FormatCell (cell));
    }

    static string FormatPlayer (Player player)
    {
        return FormatCell (player);
    }

    static string FormatCell (Player? cell)
    {
        return cell.HasValue ? cell.Value.ToString () : "_";
    }

    static Tuple<int, int> ParseMove (string input)
    {
        return Tuple.Create (
            input [0] - 'A',
            input [1] - '1'
        );
    }
}

Finally, there goes the Game class with the run loop and Main method:

class Game
{
    IBoard board;
    IBoardAnalyzer analyzer;
    IGameIO io;

    public Game (IBoard board, IBoardAnalyzer analyzer, IGameIO io)
    {
        this.board = board;
        this.analyzer = analyzer;
        this.io = io;
    }

    public void Run ()
    {
        Player player = Player.X;
        Player? winner = null;

        do {
            Tuple<int, int> move;
            try {
                move = io.AskNextMove (player, board);
            } catch {
                io.DisplayError (GameError.CouldNotParseMove);
                continue;
            }

            if (!ValidateMove (move)) {
                io.DisplayError (GameError.CouldNotParseMove);
                continue;
            }

            try {
                board [move.Item1, move.Item2] = player;
            } catch (InvalidOperationException) {
                io.DisplayError (GameError.CellAlreadyOccupied);
                continue;
            }

            player = GetNextPlayer (player);
            winner = analyzer.DetermineWinner (board);

        } while (!winner.HasValue);

        io.DisplayWinner (winner.Value);
    }

    bool ValidateMove (Tuple<int, int> move)
    {
        return (move.Item1 >= 0 && move.Item1 < board.Size)
            && (move.Item2 >= 0 && move.Item2 < board.Size);
    }

    static Player GetNextPlayer (Player player)
    {
        switch (player) {
        case Player.X:
            return Player.O;
        case Player.O:
            return Player.X;
        default:
            throw new NotImplementedException ();
        }
    }

    public static void Main (string[] args)
    {
        var game = new Game (new Board (3), new BoardAnalyzer (), new ConsoleGameIO ());
        game.Run ();
    }
}

Check it out on Github.


Sort a scrambled itinerary (Google apac test problem) This is my first time writing ruby - and I was mainly hoping to get some feedback and stylistic pointers.

The algorithm is pretty straight-forward, since Google guarantees airports aren't repeated I store all the tickets in a hash table and then sort from random insertion points, which should be worst case \$O(n \log n)\$.

Original problem description

  Once upon a day, Mary bought a one-way ticket from somewhere to somewhere with some flight transfers. For example: SFO->DFW DFW->JFK JFK->MIA MIA->ORD.

  Unfortunately, after she received the tickets, she messed up the tickets and she forgot the order of the tickets.

  Help Mary rearrange the tickets to make the tickets in correct order.

def merge_ordered_flights(ordered_flights)
    while ordered_flights.size > 1
        flights = ordered_flights.pop
        ordered_flights.each do |a|
            if a[0] == flights[-1]
                a.insert(0, *flights)
                break
            elsif a[-1] == flights[0]
                a.insert(-1, *flights)
                break
            end
        end
    end
    return ordered_flights[0] #flatten
end

def order_flights(tickets)
    ordered_flights = []
    iter = 0
    while true
        #Get an arbitrary arrival, destination flight pair
        arv, dest = tickets.shift
        if arv == nil
            break #Done if we are out of tickets
        end
        ordered_flights[iter] = []
        ordered_flights[iter].push(arv, dest)
        #And append follow-up destinations until we can't find one
        #(It's the final destination, or the other tickets have already been sorted)
        while true
            arv = dest
            dest = tickets.delete(dest)
            if dest == nil
                break #End of current chain
            end
            ordered_flights[iter].push(arv, dest)
        end
        iter += 1
    end
    #Now combine the ordered flight arrays into a single array
    return merge_ordered_flights(ordered_flights)
end

def print_flights(case_num, ordered_flights)
    flights = (0...ordered_flights.size).step(2).map {|i| ordered_flights[i] + '-' + ordered_flights[i+1] }.join ' '
    puts "CASE ##{case_num}: #{flights}"
end

while not $stdin.eof?
    num_cases = $stdin.readline.strip.to_i
    (1..num_cases).each {
        |num|
        tickets = {}
        num_flights = $stdin.readline.strip.to_i
        (0...num_flights).each {
            tickets[$stdin.readline.strip] = $stdin.readline.strip
        }
        ordered_flights = order_flights(tickets)
        print_flights(num, ordered_flights)
    }
end

Example input:

  The first line contains the number of test cases T, after which T cases follow.
  For each case, it starts with an integer N. There are N flight tickets follow.
  Each of the next 2 lines contains the source and destination of a flight ticket.

2
1
SFO
DFW
4
MIA
ORD
DFW
JFK
SFO
DFW
JFK
MIA

Expected output:

Case #1: SFO-DFW
Case #2: SFO-DFW DFW-JFK JFK-MIA MIA-ORD

Style notes:

The Ruby convention is 2 spaces of indentation
Use braces for single-line blocks, but do...end for multi-line blocks.
Keep the block parameters on the starting line; i.e. don't do this

(1..num_cases).each {
    |num|

Use #nil? rather than == nil, empty? when checking array lengths, #first/#last when accessing the start/end of an array, etc..
Postfix conditions for single-line, single-branch conditionals. E.g. this:

if arv == nil
    break #Done if we are out of tickets
end

becomes:

break if arv.nil? # Done if we are out of tickets

Instead of iterating through ranges like (0...n) you can use n.times instead. E.g. n.times { |i| puts i } will produce the same as (0...n).each { |i| puts i }
You missed a single pair of parentheses in a method call: .join ' ' :)
Granted, you can write Ruby without parentheses (Seattle style Ruby). Personally though, I like 'em (except for puts and print), but there are arguments for and against.

Code notes:

Instead of $stdin.readline.strip, you can use the more common gets.chomp. #gets is just a shortcut for $stdin.readline, and #chomp chops off the linebreak, but leaves other whitespace intact.

When possible use methods like #map and #reduce and their ilk instead of modifying a closed-over object from inside a block. E.g. this:

tickets = {}
# ...
(0...num_flights).each {
    tickets[$stdin.readline.strip] = $stdin.readline.strip
}

can be written in a few different ways - #each_with_object is probably the cleanest:

tickets = num_flights.times.each_with_object({}) do |_, tickets|
  tickets[gets.chomp] = gets.chomp
end

Alternatively, you could read the lines, and chunk them up afterward:

lines = (num_flights * 2).times { gets.chomp }
tickets = Hash[ lines.each_slice(2).to_a ]

You can do something similar in #print_flights instead of using step(2):

def print_flights(case_num, ordered_flights)
  tickets = ordered_flights.each_slice(2).map { |pair| pair.join("-") }
  puts "CASE ##{case_num}: #{tickets.join(" ")}"
end

Typically, Ruby coders avoid "raw" array indices and index arithmetic like array[i+1] in favor of more expressive code.

Lastly, #order_flights has side-effects: It deletes entries from the tickets hash you pass to it. Sure, it doesn't matter for this particular exercise, but side-effects are side-effects and should be avoided.

The algorithm itself looks fine to me. I might write it like so:

def read_tickets
  count = gets.chomp.to_i
  count.times.map do
    [gets.chomp, gets.chomp]
  end
end

def sort_tickets(tickets)
  flights = tickets.dup # make a copy to modify
  stops = flights.shift
  until flights.empty?
    from, to = flights.shift
    if from == stops.last
      stops.push(to) # could also use <<, but #push keeps "symmetry" with #unshift
    elsif to == stops.first
      stops.unshift(from)
    else
      flights.push [from, to]
    end
  end
  stops.each_cons(2).to_a
end

case_count = gets.chomp.to_i
case_count.times do |t|
  tickets = read_tickets
  ordered = sort_tickets(tickets).map { |ticket| ticket.join("-") }
  puts "Case ##{t+1}: #{ordered.join(" ")}"
end

Pretty much the same algorithm, just written differently. Notice however that I never actually use hashes, just nested arrays. Unfortunately, it's sloooow (the else branch and #each_cons are probably to blame, but I haven't checked). Still, style-wise it might give you some ideas.

An alternative, which does use hashes, would be to scan the tickets to find the first leg of the journey. After that, a single loop through should put them in order.

def read_tickets
  count = gets.chomp.to_i
  count.times.each_with_object({}) do |_, tickets|
    tickets[gets.chomp] = gets.chomp
  end
end

def print_sorted_tickets(tickets)
  inverted = tickets.invert
  from, to = tickets.detect { |from, to| inverted[from].nil? } # find first
  print " #{from}-#{to}"
  while tickets[to]
    from, to = [to, tickets[to]]
    print " #{from}-#{to}"
  end
  print "\n"
end

case_count = gets.chomp.to_i
case_count.times do |t|
  print "Case ##{t+1}:"
  tickets = read_tickets
  print_sorted_tickets(tickets)
end

I've combined sorting and printing only because I can. I'd prefer sorting and then printing, like above, or at least returning a string. The code can easily be changed to support that, but I figured I'd try combining the two. And this runs about as fast as your solution (somewhat surprisingly - I figured #invert would be more expensive).

By the way, one could "manually" create the inverse hash immediately when reading the input, instead of using #invert. But I don't think it'll make that much difference. Either case would be trading memory for simplicity.