Searching for data in a table How can I optimize this code to increase the performance of searching data?

public class TableRowInfo : IEquatable<GridRowInfo>
{
    #region Ctor

    public TableRowInfo(int rowIndex, object rowData, NodeEntry nodeEntry, bool isNewRow = false)
    {
        this.RowData = rowData;
        this.NodeEntry = nodeEntry;
        this.RowIndex = rowIndex;
        this.IsNewRow = isNewRow;
    }

    public TableRowInfo(int rowIndex, bool isNewRow)
    {
        this.RowData = null;
        this.NodeEntry = null;
        this.RowIndex = rowIndex;
        this.IsNewRow = isNewRow;
    }

    #endregion

    #region Properties

    public object RowData { get; private set; }

    public bool IsDataRow
    {
        get { return RowData != null; }
    }

    public bool IsNewRow
    {
        get;
        set;
    }

    public int RowIndex { get; internal set; }

    internal NodeEntry NodeEntry { get; private set; }

    #endregion

    public override int GetHashCode()
    {
        int rowDataHashCode = this.RowData == null ? 0 : this.RowData.GetHashCode();
        return rowDataHashCode ^ this.RowIndex;
    }

    #region IEquatable<TableRowInfo> Members

    public bool Equals(GridRowInfo other)
    {
        if (Object.ReferenceEquals(other, null)) return false;

        if (Object.ReferenceEquals(this, other)) return true;

        if (!this.IsDataRow == other.IsDataRow)
            return false;
        else if (this.IsDataRow)
            return this.RowData == other.RowData;
        else
            return this.RowIndex == other.RowIndex;
    }

    #endregion
}

public class TableSelectedRowsCollection : List<TableRowInfo>
{
    #region Ctor

    public TableSelectedRowsCollection()
    {
    }

    #endregion

    #region IList Members

    public bool Contains(TableRowInfo rowInfo)
    {
        if (rowInfo.IsNewRow)
            return Find(rowInfo.RowIndex) != null;
        else
            return Find(rowInfo.NodeEntry) != null;
    }

    public bool Contains(int rowIndex)
    {
        return Find(rowIndex) != null;
    }

    public bool Contains(object rowData)
    {
        return Find(rowData) != null;
    }

    public bool Contains(NodeEntry nodeEntry)
    {
        return Find(nodeEntry) != null;
    }

    #endregion

    #region Helper Methods

    internal TableRowInfo Find(NodeEntry nodeEntry)
    {
        if (nodeEntry != null && this.Count > 0)
        {
            return this.FirstOrDefault(rowInfo => rowInfo.NodeEntry == nodeEntry);
        }
        return null;
    }

    internal TableRowInfo Find(int rowIndex)
    {
        if (rowIndex > 0 && this.Count > 0)
        {
            return this.FirstOrDefault(rowInfo => rowInfo.RowIndex == rowIndex);
        }
        return null;
    }

    internal TableRowInfo Find(object rowData)
    {
        if (rowData != null && this.Count > 0)
        {
            return this.FirstOrDefault(rowInfo => rowInfo.RowData == rowData);
        }
        return null;
    }

    internal TableRowInfo Find(TableRowInfo rowInfo)
    {
        if (rowInfo != null && this.Count > 0)
        {
            return this.FirstOrDefault(item => item.NodeEntry == rowInfo.NodeEntry);
        }
        return null;
    }

    internal List<int> GetRowIndexes()
    {
        var rowIndexes = new List<int>();
        if (this.Count > 0)
        {
            this.ForEach(rowInfo => rowIndexes.Add(rowInfo.RowIndex));
        }
        return rowIndexes;
    }

    #endregion
}

For faster searching and since you're searching by different properties, one way would be a different dictionary for each property you want to search for. The indexing property is the key and the value can be the tablerowinfo object or if indexes may be duplicated a list of tablerowinfoinfo objects.

Dictionary<NodeEntry, TableRowInfo> nodeEntryIndex;
Dictionary<int, TableRowInfo> rowNumberIndex;
Dictionary<object, TableRowInfo> rowDataIndex;

Assuming you've implemented Add and Remove methods, simply add/remove the appropriate entries to the dictionaries, in those methods.

The Find method would look something like this:

internal TableRowInfo Find(NodeEntry nodeEntry)
{
    if (nodeEntryIndex.ContainsKey(nodeEntry))
    {
        return nodeEntryIndex[nodeEntry];
    }
    return null;
}

One suggestion.  When you inherit from a List like this, you expose all the members you've inherited from the list, outside the class.  If you're going to provide your own version for all of them that's fine.  However if you're only going to implement a subset of them then you might want to consider not inheriting but having a List as a private property to store the collection.  This gives you much more flexibility in choosing what gets exposed outside the class.

Another suggestion.  Instead of using object as the rowData type, the DataRow Class would probably be better suited for that.  This increase the robustness of your class.


Code deletes unique entity that reports data later than 4 quarters, runs too slow I have a worksheet which is filled with data from Column A to G. Each row is a unique entity and Column G represents how long ago (in quarters) the data was reported. The purpose of the code is to check if the value is more than 4 (that means the reported date was more than 4 quarters from today), and if so, the code would delete that particular row.

Currently my code takes about 3 minutes to run, and I was wondering if there's anything else that I could do/restructure my code for it to run faster.

I have about 5000++ rows as of now.

Sub Two_Keep3Quarters()
    Dim Firstrow As Long
    Dim Lastrow As Long
    Dim lRow As Long
    Dim Tbl As ListObject
    Dim rng As Range
    Dim QuarterValue As Long

    With Application
        .Calculation = xlCalculationManual
        .ScreenUpdating = False
    End With

    With Sheets("Filtered Data")
        .DisplayPageBreaks = False

        'Set the first and last row to loop through
        Firstrow = 3
        Lastrow = .Cells(.Rows.Count, "A").End(xlUp).Row

        'We loop from Lastrow to Firstrow (bottom to top)
        For lRow = Lastrow To Firstrow Step -1

        QuarterValue = .Range("G" & lRow).Value

            'We check the values in the Column G
            With .Cells(lRow, "G")

                If Not IsError(QuarterValue) Then

                    If QuarterValue > 4 Then .EntireRow.Delete
                    'This will delete each row with value of more than 4 quarters

                End If

            End With

        Next lRow

    End With

    Range("F1").Value = "Quarters"
    Range("G1").Value = "No. of Quarters"

    On Error Resume Next
    Set rng = Range(Range("A1"), Range("G1").End(xlDown)).SpecialCells(xlCellTypeBlanks)
    rng.Rows.Delete Shift:=xlShiftUp

         For Each Tbl In Sheets("Filtered Data").ListObjects
         Tbl.Unlist

    Next

        Set Tbl = ActiveSheet.ListObjects.Add(xlSrcRange, Range(Range("A1"), Range("G1").End(xlDown)), , xlYes)

    With Tbl
    .Name = "DataTable"
    .TableStyle = "TableStyleLight10"
    End With

Application.ScreenUpdating = True

End Sub

This uses an AutoFilter and performs the delete operation once (it should take less than a sec)

Option Explicit

Public Sub Two_Keep3Quarters()
    Const WS_NAME = "Filtered Data"

    Dim ws As Worksheet, tbl As ListObject, tblUR As Range, t As Double

    t = Timer
    Set ws = ThisWorkbook.Worksheets(WS_NAME)
    Set tbl = ws.ListObjects("DataTable")
    Set tblUR = tbl.Range.Offset(2).Resize(tbl.ListRows.Count - 1)

    Application.DisplayAlerts = False
    With tbl.Range
        tblUR.AutoFilter Field:=7, Criteria1:=">4", Operator:=xlAnd
        .Rows("1:2").Hidden = True

        On Error Resume Next    'For empty filter
        tblUR.SpecialCells(xlCellTypeVisible).EntireRow.Delete
        On Error GoTo 0

        .AutoFilter
        .Rows.EntireRow.Hidden = False
        .Cells(1).Activate
    End With
    Application.DisplayAlerts = True
    Debug.Print "Time: " & Format(Timer - t, "0.000") & " sec"
End Sub

Some issues with your code:

Missing Option Explicit
The Sub should be declared Public explicitly
Naming conventions: what's the difference between Lastrow and lRow

All variable names should start with a lower case letter

You turn Calculation to xlCalculationManual but don't change it back at the end
Some ranges are not fully qualified (bellow Range("F1"))

When you know the worksheet used you shouldn't qualify ranges with ActiveSheet

You're performing an extra rng.Rows.Delete which is unneeded

When deleting rows make sure you use EntireRow - it's faster

Tables (ListObjects) are dynamic so there is no need to Unlist and resize them
Inconsistent indentation (at the end)


Injecting AutoMapper profiles I'd like to know if I'm doing profile configuration in the wrong place or in the wrong way.

I'm following the Onion Architecture, so that restricts the direction of my dependencies towards the center.

Core

My domain model and AutoMapper facade:

namespace Core.Domain
{
    public class MyModel
    {
        // model stuff
    }
}

namespace Core.Services
{
    public interface IMapper
    {
        object Map(object source, Type sourceType, Type destinationType);
    }
}

Infrastructure

AutoMapper facade implementation:

namespace Infrastructure.Mapping
{
    public class Mapper : IMapper
    {
        private readonly IMappingEngine _mappingEngine;

        public Mapper(IMappingEngine mappingEngine)
        {
            _mappingEngine = mappingEngine;
        }

        public object Map(object source, Type sourceType, Type destinationType)
        {
            return _mappingEngine.Map(source, sourceType, destinationType);
        }
    }
}

UI

This is my controller and view model. I'm using the AutoMapper via a filter, following this example.

namespace UI.Controllers
{
    public class HomeController : Controller
    {
        [AutoMap(typeof(MyModel), typeof(MyViewModel))]
        public ActionResult Index()
        {
            var myItem = _myRepository.GetById(0);
            return View(myItem);
        }
    }
}

namespace UI.ViewModels
{
    public class MyViewModel
    {
        // view stuff
    }
}

Dependency Resolution

This is where I have my doubts:

namespace DependencyResolution
{
    public class MappingModule : NinjectModule
    {
        public override void Load()
        {
            Mapper.Initialize(cfg => cfg.AddProfile(new MyProfile()));
            Bind<IMappingEngine>().ToMethod(ctx => Mapper.Engine);
            Bind<IMapper>().To<Mapping.Mapper>();

            Kernel.BindFilter<AutoMapFilter>(FilterScope.Controller, 0)
                  .WhenActionMethodHas<AutoMapAttribute>()
                  .WithConstructorArgumentFromActionAttribute<AutoMapAttribute>("sourceType", att => att.SourceType)
                  .WithConstructorArgumentFromActionAttribute<AutoMapAttribute>("destType", att => att.DestType);
        }
    }

    public class MyProfile : Profile
    {
        protected override void Configure()
        {
            Mapper.CreateMap<MyModel, MyViewModel>().ForMember(...);
        }
    }
}

Questions

Is the way I bind to AutoMapper wrong?
Is this the wrong place for the profile (keep in mind the dependency restriction)?

In the ideal world I would have placed

Mapper.CreateMap<MyModel, MyViewModel>().ForMember(...)

in Global.asax, but how do I expose CreateMap without referencing AutoMapper?

Is there anything else you have noticed?

What is the purpose of the IMapper interface and Mapper class?  It looks to me that they are just wrapping the IMappingEngine interface and MappingEngine class.  While this is a good method when you have a third party class that doesn't have an interface, I think it is overkill here.  Why don't you just use the IMappingEngine where you need that functionality?

If you are going to keep your Mapper class, I would rename it, having two Mapper classes is confusing.

As for where it is, I don't have a problem with doing it this way.  All the wire-up is done in one place, and its easy to find and add to as needed.


Filtering a list by comparing enums against a user choice I have a list of objects where one of the properties on the object class is an enum.

The program below simply loops through each item in the list, checks an int value, then checks the enum value for that item, then adds the item to another list when the condition is met.

I am using if-else statements, I attempted to use a switch statement but it doesn't seem possible to place 2 constants on a single case.

I'd like to know if there's a better / more elegant way to do this or is what I've done sufficient in this scenario?

Program Code

// User inputs number from 1 - 5
int SelectedOppType = Int32.Parse(Console.ReadLine());

foreach (Opportunities Opp in OpportunitiesList)
{
    if (SelectedOppType == 1 && Opp.OpportunityStatusID == OpportunityStatus.Active) // Opp.OpportunityStatusID == OpportunityStatus.Draft
    {
        FilterdOppsList.Add(Opp);
    }
    else if (SelectedOppType == 2 && Opp.OpportunityStatusID == OpportunityStatus.Draft)
    {
        FilterdOppsList.Add(Opp);
    }
    else if (SelectedOppType == 3 && Opp.OpportunityStatusID == OpportunityStatus.Closed)
    {
        FilterdOppsList.Add(Opp);
    }
    else if (SelectedOppType == 4 && (Opp.OpportunityStatusID == OpportunityStatus.Active || Opp.OpportunityStatusID == OpportunityStatus.Draft))
    {
        FilterdOppsList.Add(Opp);
    }
    else if (SelectedOppType == 5)
    {
        FilterdOppsList.Add(Opp);
    }

}

foreach (var OppItem in FilterdOppsList)
{
    Console.WriteLine(OppItem.OppText);
}

Console.ReadLine();

When SelectedOppType == 5, all values in OpportunitiesList will be added to FilterdOppsList, but only after evaluating many conditions unnecessarily. It would be better to handle this as a special case,
and add all values of OpportunitiesList in FilterdOppsList before ever entering the foreach loop.

Next, in the foreach loop, since selectedOppType doesn't change throughout the loop, regardless of the value of opp, it would make more sense to use conditionals on the value of opp.OpportunityStatusID instead of selectedOppType. You can rearrange the conditions to switch statement.

Putting it all together:

if(selectedOppType == 5)
{
    FilterdOppsList.AddRange(OpportunitiesList);
}
else
{
    foreach (Opportunities opp in OpportunitiesList)
    {
        bool shouldAdd = false;
        switch (opp.OpportunityStatusID)
        {
            case OpportunityStatus.Active:
                 shouldAdd = selectedOppType == 1 || selectedOppType == 4;
                 break;
            case OpportunityStatus.Draft:
                 shouldAdd = selectedOppType == 2 || selectedOppType == 4;
                 break;
            case OpportunityStatus.Closed:
                 shouldAdd = selectedOppType == 3;
                 break;
        }
        if (shouldAdd)
        {
            FilterdOppsList.Add(opp);
        }
    }
}


Interactive Discord bot for tabletop RPG I'm new to Python, but have experience in a variety of other languages. I've learnt that a code review early on would make sure I don't start with bad habits. Which are more difficult to get out the longer you keep them.

This is a Discord bot for one of my hobby projects.

import os
import random
import discord
from discord.ext import commands
from dotenv import load_dotenv

load_dotenv()
token = os.getenv('DISCORD_TOKEN')

bot = commands.Bot(command_prefix='!')

@bot.command(name='ability', aliases=['a', 'ab'], help='Roll an ability check. Parameters: ability [no. of heroic dice].')
async def ability(ctx, ability:int, heroic:int=0):
    if ability < -5 or ability > 5:
        await ctx.send('Invalid ability value "{}". Must be between -5 and +5'.format(ability))
        return
    if heroic < 0 or heroic > 5:
        await ctx.send('Invalid heroic dice amount "{}". Must be between 0 and 5 (3 + 2 ritual dice)'.format(heroic))
        return

    dice = 2 + abs(ability) + heroic
    keep_dice = 2 + heroic
    if (ability < 0):
        keep = 'lowest'
    else:
        keep = 'highest'

    roll = []
    for x in range(0, dice):
        roll.append(roll_die())

    result = sorted(roll)
    if keep == 'highest':
        result.reverse()
    outcome = result[:keep_dice]

    # check for critical success (a pair) or failure (all 1s)
    last = -1
    critical = False
    fail = True
    for d in outcome:
        if d != 1:
            fail = False
        if d == last and d != 1:
            critical = True
        last = d

    if critical:
        # critical success - add the next highest/lowest die as well
        if keep_dice == dice:
            # roll an additional die
            outcome.append(roll_die())
            outcome.sort()
        else:
            outcome = result[:keep_dice+1]
    elif fail:
        outcome = []
        for x in range(0, keep_dice):
            outcome.append(0)

    # now sum up all our dice for the total
    sum = 0
    for d in outcome:
        sum += d

    embed = discord.Embed(title='**Dragon Eye** Ability Test for {}'.format(ctx.author.name),
                          description='Rolling {}d10, keep {} {}'.format(dice, keep, keep_dice),
                          color=0x22a7f0)
    embed.add_field(name='Roll', value=format(roll), inline=False)
    embed.add_field(name='Outcome', value=format(outcome), inline=True)
    embed.add_field(name='Total', value=format(sum), inline=True)
    if critical:
        embed.add_field(name='Additional Info', value='**Critical Success**', inline=True)
    elif fail:
        embed.add_field(name='Additional Info', value='**Critical Fail**', inline=True)

    await ctx.send(embed=embed)

def roll_die(sides=10):
    die = random.randint(1, sides)
    return die

bot.run(token)

List Comprehension

Instead of

roll = []
for x in range(0, dice):
    roll.append(roll_die())

do this

roll = [roll_die() for _ in range(0, dice)]

The result is the same, it requires less code, and more pythonic. This also utilizes the _. The underscore is meant to show that the variable in the loop isn't used. This applies to other places in your code as well.

Ternary Operator

Instead of

if (ability < 0):
    keep = 'lowest'
else:
    keep = 'highest'

do this

keep = 'lowest' if ability < 0 else 'highest'

Using a ternary operator can keep you from writing more code, and looks a lot nicer. In this case, it converts four lines into one, which is a big plus.

Direct Returns

You don't have to assign a value inside a function before returning it. In this case, you don't need a dice variable inside roll_die(). You can simply return the call to random.randint. Take a look:

def roll_die(sides=10):
    return random.randint(1, sides)

Using sum

Instead of

sum = 0
for d in outcome:
    sum += d

use pythons built-in sum function

dice_sum = sum(outcome)

The sum functions returns the sum of all the numbers in an iterable. So, you can pass the list used in the loop to that function. It does the same thing you do, but it's already written.


PHP MySQL grouping and ordering I have a tv show I created a MySQL database for. I am trying to get my code to group and order correctly the seasons and episodes within their respective season.

Current Code

if (!($result = mysql_query("SELECT season_num, sub_season_num, esp_num, title, descrip FROM season ORDER BY season_num, sub_season_num", $link))) showerror();
$season_one = 1;
$season_two = 2;
echo "Season 1
";
while ($row = mysql_fetch_array($result))
{
if(season_num !== $season_one){
echo "Episode {$row["esp_num"]}
";
echo "Title {$row["title"]}
";
echo "Description {$row["descrip"]}
";
}}
echo "Season 2
";
while ($row = mysql_fetch_array($result))
{
if(season_num !== $season_two){
echo "Episode {$row["esp_num"]}
";
echo "Title {$row["title"]}
";
echo "Description {$row["descrip"]}
";
}}
?>

Example
Season 1
Eps 1
Eps 2
Eps 3
Season 2
Eps 1
Eps 2
Ect..

Any code help would be greatly appreciated.

You didn't need to hard-code season values like 1 and 2. Just use the nicely sorted repeating values from the rows and detect when the value changes from one row to the next.  With such a generic test, as shown below, you can use one loop and avoid repeated code.

Also note that you seemed to be using the wrong operator, !== the not-identical operator to do an equality test.

if (!($result = mysql_query("SELECT season_num, sub_season_num, esp_num, title, descrip FROM season ORDER BY season_num, sub_season_num", $link))) showerror();
$previous_season = 0; // start with nonsense value to force a heading
while ($row = mysql_fetch_array($result))
{
    $season = $row["season_num"]
    // Ignoring subseason in this version.
    // Decide whether to match both season and sub_season and
    // print both in a new header, complicating this `if` statement
    // OR have a separate, similar `if` statement and separate (sub)header
    // for subseasons. In the latter case, be sure to
    // reset previous_subseason to a nonsense value whenever season changes
    // Otherwise, a subseason header will be missing if a season happens to end
    // on the same subseason number (like 1) that the next season begins with.
    if ($season != $previous_season) {
        $echo "Season $season
";
        $previous_season = $season;
    }
    echo "Episode {$row["esp_num"]}
";
    echo "Title {$row["title"]}
";
    echo "Description {$row["descrip"]}
";
}


2vs2 Poker Hand One Pair Winner Algorithm I wrote this algorithm to determine the winning hand of two players going head to head with a hand of One Pair.  Note that the hand is sorted once it is given out, and the algorithm has to compare the two hands one pair, if one has a higher one pair such as AA vs 33, there is an obvious winner.  If the hands have matching one pairs, such as 44 & 44, the high card must be compared to determine the winner.

Please take a look at let me know how it looks.  It is my first real algorithm taking into account so many factors.  I am looking more for opinions, not answers.  If you feel it is no good and needs improvement, please tell me so.  It does work 100% as I have ran a lot of tests using different one pair hands of different values.

         // Arrays to hold matching pair and high card
         PlayingCard[] pPairHigh = new PlayingCard[2];
         PlayingCard[] oPairHigh = new PlayingCard[2];
         for(int i = 0; i < POKER_HAND_SIZE - 1 ; i++) {

             if(this.hand[i].compareTo(this.hand[i+1]) == 0) {
                 pPairHigh[0] = this.hand[i];

                 if(i == 3)
                    pPairHigh[1] = this.hand[i - 1];
                 else
                    pPairHigh[1] = this.hand[POKER_HAND_SIZE - 1];
             }

             if(otherHand.hand[i].compareTo(otherHand.hand[i+1]) == 0) {
                 oPairHigh[0] = otherHand.hand[i];

                 if(i == 3)
                    oPairHigh[1] = otherHand.hand[i - 1];
                 else
                    oPairHigh[1] = otherHand.hand[POKER_HAND_SIZE -

         if(pPairHigh[0].compareTo(oPairHigh[0]) == 0){
             if(pPairHigh[1].compareTo(oPairHigh[1]) < 0)
                     return -1;
             else if(pPairHigh[1].compareTo(oPairHigh[1]) > 0)
                 return 1;
             else
                 return 0;
         }

         if(pPairHigh[0].compareTo(oPairHigh[0]) < 0)
             return -1;
         else
             return 1;

     }

I'm not really any good at algorithms as such so my main suggestion would be to remove duplicate code.  That is primarily in the creation of your pPairHigh and oPairHigh blocks of code.

I admit they do share a common loop which in my implementation means this will be occurring twice.  If this is an issue in performance it could be broken out so the pairs are created like you have done in a single loop.  But I would still call a method to do the actual creation.  However I'm working on the assumption performance is not really an issue, here is my suggestions.

// Please not I made a guess around your method name here
private int ComparePairs() {
    PlayingCard[] myHand = getPokerHand(this.hand);
    PlayingCard[] otherPlayersHand = getPokerHand(otherHand);

    return compareMyHandAgainstOthers(myHand, otherPlayersHands)
 }

 private int compareMyHandAgainstOthers(PlayingCard[] myHand, PlayingCard[] otherHand) {
    if(pPairHigh[0].compareTo(oPairHigh[0]) == 0) {
        if(pPairHigh[1].compareTo(oPairHigh[1]) < 0)
           return -1;
        else if(pPairHigh[1].compareTo(oPairHigh[1]) > 0)
           return 1;
        else
           return 0;
    }

    if(pPairHigh[0].compareTo(oPairHigh[0]) < 0)
       return -1;
    else
       return 1;
 }

 private PlayingCard[] getPokerHand(Array hand) {

   PlayingCard[] pair = new PlayingCard[2];
   for(int i = 0; i < POKER_HAND_SIZE - 1 ; i++) {
        if(this.hand[i].compareTo(this.hand[i+1]) == 0) {
            pair[0] = this.hand[i];

            if(i == 3)
               pair[1] = this.hand[i - 1];
            else
               pair[1] = this.hand[POKER_HAND_SIZE - 1];

        }
    }

    // throw exception here as no possible pair found???
     return pair;
 }

Please note there may be java specific code as I haven't programmed in Java for a while.


Create new columns depending on the columns in a table I have code in the this example:

legacy_id phone_type phone_number
     1        f      1234567890
     1        b      1233854100
     1        f      4110256565
     2        f      0707070770
     2        b      7895120044

I want the data to end up like the following:

legacy_id  f_phone_number_1 b_phone_number_1 f_phone_number_2
     1      1234567890        1233854100        4110256565
     2      0707070770        7895120044

My initial approach works but I was hoping there is a more efficient what of doing this.

Select fill.legacy_id, max(fill.f_phone_number_1),max(fill.b_phone_number_1),max(fill.f_phone_number_2)
from
(
  Select
       a.legacy_id as legacy_id, a.phone_type as phone_type,
       case
           when a.phone_type = 'F' then a.phone_number and
           dense_rank() over (partition by a.legacy_id, a.phone_type order by a.legacy_id, a.phone_type, a.phone_number) = 1
           else null
       end as f_phone_number_1,
       case
           when a.phone_type = 'F' then a.phone_number and
           dense_rank() over (partition by a.legacy_id, a.phone_type order by a.legacy_id, a.phone_type, a.phone_number) = 2
           else null
       end as f_phone_number_2,
       case
           when a.phone_type = 'b' then a.phone_number and
           dense_rank() over (partition by a.legacy_id, a.phone_type order by a.legacy_id, a.phone_type, a.phone_number) = 1
           else null
       end as b_phone_number_1
  from table a
  group by a.legacy_id, a.phone_type, a.phone_number
) fill
group by fill.legacy_id

Seems like there are up to two rows for f and one for b.

In that case you can simplify it to a single GROUP BY using conditional aggregation:

SELECT legacy_id,
   -- first number 'f'
   Min(CASE WHEN phone_type = 'f' THEN phone_number END) AS f_phone_number_1,
   -- first number 'b'
   Min(CASE WHEN phone_type = 'b' THEN phone_number END) AS b_phone_number_1,
   -- when MIN and MAX are different return the 2nd number
   NullIf(Max(CASE WHEN phone_type = 'f' THEN phone_number END)
         ,Min(CASE WHEN phone_type = 'f' THEN phone_number END)) AS f_phone_number_2
FROM table
GROUP BY 1

Edit:

Of yourse you can't use the MAX/MIN approach when there's more than two rows per type.

But you can reduce the ranks down to one and move the case to the outer select:

SELECT legacy_id,
   Max(CASE WHEN phone_type = 'f' AND rn = 1 THEN phone_number END) AS f_phone_number_1,
   Max(CASE WHEN phone_type = 'f' AND rn = 2 THEN phone_number END) AS f_phone_number_2,
   Max(CASE WHEN phone_type = 'f' AND rn = 3 THEN phone_number END) AS f_phone_number_3,
   Max(CASE WHEN phone_type = 'f' AND rn = 4 THEN phone_number END) AS f_phone_number_4,
   Max(CASE WHEN phone_type = 'b' AND rn = 1 THEN phone_number END) AS b_phone_number_1,
   Max(CASE WHEN phone_type = 'b' AND rn = 2 THEN phone_number END) AS b_phone_number_2,
   Max(CASE WHEN phone_type = 'b' AND rn = 3 THEN phone_number END) AS b_phone_number_3,
   Max(CASE WHEN phone_type = 'b' AND rn = 4 THEN phone_number END) AS b_phone_number_4
FROM
 (
   SELECT legacy_id,
      phone_type,
      phone_number,
      Dense_Rank()
      Over (PARTITION BY a.legacy_id, a.phone_type
            ORDER BY a.phone_number) AS rn
   FROM tab
 ) AS dt
GROUP BY 1


Should I create an empty string and append through a foreach loop or should I use StringBuilder? Here's the current code:

    static string GetResources(string header, string filter, string resourceTemplate)
    {
        string themeName;
        if (!TryGetHeaderValue(header, "theme", out themeName)) return "";

        var resources = "";
        var path = Path.Combine(HtmlPreview.BaseDirectory, themeName);

        foreach (var resource in Directory.GetFiles(path, filter))
        {
            resources += String.Format(
                resourceTemplate,
                themeName,
                Path.GetFileName(resource));
        }

        return resources;
    }

I was wondering if I should use string builder instead:

static string GetResources(string header, string filter, string resourceTemplate)
    {
        string themeName;
        if (!TryGetHeaderValue(header, "theme", out themeName)) return "";

        var resourcesStringBuilder = new StringBuilder();
        var path = Path.Combine(HtmlPreview.BaseDirectory, themeName);

        foreach (var resource in Directory.GetFiles(path, filter))
        {
            resourcesStringBuilder.Append(String.Format(
                resourceTemplate,
                themeName,
                Path.GetFileName(resource)));
        }

        return resourcesStringBuilder.ToString();
    }

Resharper simply suggests it is possible to replace the original code with a LINQ statement:

    static string GetResources(string header, string filter, string resourceTemplate)
    {
        string themeName;
        if (!TryGetHeaderValue(header, "theme", out themeName)) return "";

        var path = Path.Combine(HtmlPreview.BaseDirectory, themeName);

        return Directory.GetFiles(path, filter)
            .Aggregate("",
            (current, resource) => current +
                String.Format(resourceTemplate,
                themeName,
                Path.GetFileName(resource)));
    }

Thank you! The original code with context is also on github.

Strings in C# are immutable which means that this:

 resouces += string.Format(...);

will create 2 strings: one from string.Format and then the new string created by appending it to the existing string.

A StringBuilder will just keep a list of all the strings to append and create one result string at the end when you call ToString(). So instead of 2*n strings you only create n+1 strings. The Aggregate has the same problem.

There are some optimizations in the C# compiler which can transform string + string + string + ... sequences into Append calls on a StringBuilder but you should not really rely on this.

In the end it doesn't matter too much if your strings a short. However when you hit the 85kb limit then the strings will be placed on the large object heap which can cause problems because it doesn't get compacted and you can get holes. This can lead to weird out of memory situations (you can find more with google).


PHP Table generation I have been using the following function (in the kohana html class in a table) to generate tables.

function table($columns,$reverse=false,$fill=false,array $attr=null){
    $c = '';
    //ugly hack, looking for fix
    $max=sizeof(max($columns));
    for($i=0;$i<sizeof($columns);$i++){
            $column = $columns[$i];

        if($fill==true){
            $l = ($max-sizeof($column));
            for($ii=0;$ii<$l;$ii++){
                array_push($column,'');
            }
        }
        if($reverse==true){
            $columns[$i] = array_reverse($column);
        }else{
            $columns[$i] = $column;
        }
    }
    for($i=0,$l=sizeof($columns);$i<$l;$i++){
        $column = $columns[$i];
        $c.="
";
        for($ii=0,$ll=sizeof($column);$ii<$ll;$ii++){
            $c.="".$column[$ii]."	";
        }
        $c.="
";
    }
    return "
".$c."
";
}

And then I have been calling it like so:

echo html::table(array(
    array('colunm 1 row 1','colunm 2 row 1'),
));

And a reversed table that auto fills the columns.

echo html::table(array(
    array('colunm 1 row 1','colunm 2 row 1'),
    array('column 1 row 2','column 2 row 2')
),true,true);

Didn't test, but I would write it something like this:

function table($rows, $reverse = false, $fill = false, array $attr = null) {
    $c = '';
    $max_cols = sizeof(max($columns));

    foreach ($rows as $columns) {
        if ($fill && sizeof($columns) < $max_cols) {
            $columns = array_merge($columns, array_fill(0, $max_cols - sizeof($columns), ''));
        }
        if ($reverse) {
            $columns = array_reverse($columns);
        }
        $c .= '
' . implode('	', $columns) . '
';
    }

    return '
' . $c . '
';
}


Getting the largest element in an array using recursion Any suggestions on how to make this code more efficient?

import java.util.Scanner;
public class RecursionLargestInArray
{
public static void main (String[] args)
{
    int max = -999;
    Scanner scan = new Scanner (System.in);
    System.out.print("Enter the size of the array: ");
    int arraySize = scan.nextInt();
    int[] myArray = new int[arraySize];
    System.out.print("Enter the " + arraySize + " values of the array: ");
    for (int i = 0; i < myArray.length; i++)
        myArray[i] = scan.nextInt();
    for (int i = 0; i < myArray.length; i++)
        System.out.println(myArray[i]);
    System.out.println("In the array entered, the larget value is "
                        + getLargest(myArray, max) + ".");
}

public static int getLargest(int[] myArray, int max)
{
    int i = 0, j = 0, tempmax = 0;
    if (myArray.length == 1)
    {
        return myArray[0] > max ? myArray[0] : max;
    }
    else if (max < myArray[i])
    {
        max = myArray[i];
        int[] tempArray = new int[myArray.length-1];
        for (i = 1; i < myArray.length; i++)
        {
            tempArray[j] = myArray[i];
            j++;
        }
        tempmax = getLargest(tempArray, max);
        return tempmax;
    }
    else
    {
        int[] tempArray = new int[myArray.length-1];
        for (i = 1; i < myArray.length; i++)
        {
            tempArray[j] = myArray[i];
            j++;
        }
        tempmax = getLargest(tempArray, max);
        return tempmax;
    }
}
}

I would use a helper function, and I would avoid copying the array (using indices instead):

public static int getLargest(int ... myArray) {
    return getLargest(myArray, 0, myArray.length);
}

private static int getLargest(int[] myArray, int from, int to) {
    if(from == to) {
        throw new IllegalArgumentException("empty array");
    } else if (from + 1 == to) {
        return myArray[from];
    } else {
        int middle = (from + to)  / 2;
        return Math.max(getLargest(myArray, from, middle),
                        getLargest(myArray, middle, to));
    }
}

Using varargs int ... instead of an array int[] for the argument makes it more flexible, e.g. better testable.

In case you really want to copy the array, use System.arraycopy, which is much faster than looping through.


Python making call list for C++ executable I have a text file which contains configuration values that are intended to be arguments to a C++ executable (to be read in as a vector in C++). I am using a Python script to read in the text file and then call the C++ executable as a subprocess. The reason I'm doing it via Python is because I do not want to modify the C++ executable for unrelated reasons.

Here is my code snippet to achieve this. I was wondering if there's a better way to do this.

call_list = [executable, "--directory", directory, "--filePrefix", file_prefix, "--configList"]

# read config file
config_file = directory + file_prefix + ".txt"
config_list = [line.strip() for line in open(config_file)]

config_list_split = []
for config in config_list:
    config_list_split.extend(config.split())

call_list.extend(config_list_split)

subprocess.check_call(call_list)

Putting an open() function inside a list comprehension seems a bit iffy to me, especially since you don't seem to close it anywhere.

List comprehensions are generally better than for loops.

The config_file name is a bit misleading, as it's the filename, not the file object.

So alternative code would be:

config_file_name = directory + file_prefix + ".txt"
with  open(config_file_name) as config_file:
    config_list_split = [config_item for config in config_file for config_item in config.split()]

This is less transparent, but performance-wise I would expect it to be better than a for-loop.


Is this the right way to use TcpListener in the background? I have a server application using TcpListener in BackgroundWorker, where my application is doing some work, and in the background listening to a IP:PORT.

The DoWork of the BackgroundWorker:

private void m_oWorker_DoWork(object sender, DoWorkEventArgs e)
{
    while (true) //infinit loop : listen
    {
        var clientSocket = _serverSocket.AcceptTcpClient();
        e.Result = clientSocket;
        m_oWorker.ReportProgress(1, clientSocket);
    }
}

At the progress change event, I do notify the form (launcher of the BackgroundWorker) that a new TcpClient is available:

The ProgressChanged of the BackgroundWorker:

private void m_oWorker_ProgressChanged(object sender, ProgressChangedEventArgs e)
{
    _luncher.NotifyFormNewTcpClient((TcpClient)e.UserState);
    MessageBox.Show("new TcpClient Connected...");
}

Now for each form that wants to use that background worker, I need to add the method NotifyFormNewTcpClient, and in that method, I have a list of TcpListening, and I added the returned TcpClient to that list:

public void NotifyFormNewTcpClient(TcpClient tcpClient)
{
    TcpClientsList.Add(tcpClient);
    lNumberOfClient.Text = TcpClientsList.Count.ToString()  " Clients";
}

Question:

What I am doing works until now, but I wanted to know if I am doing it the right way.  I want my application to keep doing some work, while it's listening in a LAN for clients that request it, and have a list of the clients (TcpClient) at any moment.  Any ideas are welcome.

Your code looks OK, though I think there is some redundant code - e.Result = clientSocket; is never used, since work is never completed...

You might also want to consider a different approach to BackgroundWorker - TcpListener.AcceptTcpClientAsync. Instead of creating a worker thread you can do this:

private Task<TcpClient> listen() {

   return _serverSocket.AcceptTcpClientAsync().ContinueWith(clientSocket => {
       _launcher.NotifyFormNewTcpClient(clientSocket);
       MessageBox.Show("new TcpClient Connected...");
       return listen();
   });


Method that concatenates strings, some of which may be null, with several different separators I'd like to refactor a Java method I've created, making it cleaner and more beautiful. The method concatenates some strings, which may be empty or not. I'm new on java so i'm not sure how to do that, especially because between the strings I may have separators.

Here is my method:

private String createString(String modeloVotacao, String tipoProposicao, String ordemNumero, String ordemAno) {

    String modeloDescricao = "";
    if(modeloVotacao != null) {
        modeloDescricao = modeloVotacao + " - ";
    }

    if(tipoProposicao != null) {
        modeloDescricao += tipoProposicao + " ";
    }
    modeloDescricao += ordemNumero != null?ordemNumero:"";

    if(ordemNumero != null && ordemAno != null) {
        modeloDescricao += "/"+ordemAno;
    } else {
        if(ordemAno != null) {
            modeloDescricao += ordemAno;
        }
    }

    return modeloDescricao;
}

Here is some results that this method returns:

All filled: 1ª DISCUSSÃO - PROJETO DE LEI COMPLEMENTAR Nº 16/2018
modeloVotacao = null: PROJETO DE LEI COMPLEMENTAR Nº 16/2018
tipoProposicao = null: 1ª DISCUSSÃO - Nº 16/2018
ordemNumero = null: 1ª DISCUSSÃO - PROJETO DE LEI COMPLEMENTAR 2018
ordemAno = null: 1ª DISCUSSÃO - PROJETO DE LEI COMPLEMENTAR Nº 16
all null: empty string
modeloVotacao  and tipoProposicao  = null: Nº 16/2018
ordemNumero and ordemAno  = null: 1ª DISCUSSÃO - PROJETO DE LEI COMPLEMENTAR

The repeated task here is that you want to conditionally insert a separator between two strings if both are non-null.  So, write a helper function that does that:

private static String join(String a, String sep, String b) {
    return (a == null) ? b :
           (b == null) ? a : a + sep + b;
}

Then, the code becomes simple:

private String createString(String modeloVotacao, String tipoProposicao, String ordemNumero, String ordemAno) {
    String description = join(modeloVotacao, " - ",
                             join(tipoProposicao, " ",
                                 join(ordemNumero, "/", ordemAno)));
    return (description == null) ? "" : description;
}

Note that in your final example, where ordemNumero and ordemAno are both null, the output ends with a space.  That does not happen in my solution; I consider that to be an improvement.

Please avoid mixing Portuguese with English identifiers (such as createString()).  Stick with one or the other (preferably English).


Formatting date depending on current date I have two screens where I show a message creation time and date. Initially, there was only one screen and I should always show time and show date differently depending on difference between today and the message creation day.

Here is my method:

private final static String TIME_TEMPLATE_TODAY = "HH:mm";

public static String getFormattedDate(Date msgDate, Date currentTime) {
        long daysDiff = Utils.getDaysDiff(currentTime, msgDate);

        String format;
        if (currentTime.getYear() != msgDate.getYear()) {
            format = "dd.MM.yyyy' at '" + TIME_TEMPLATE_TODAY;
        } else if (daysDiff == 0) {
            format = TIME_TEMPLATE_TODAY;
        } else if (daysDiff == 1) {
            format = "'yesterday at " + TIME_TEMPLATE_TODAY;
        } else if (daysDiff > 1) {
            format = "dd.MM' at '" + TIME_TEMPLATE_TODAY;
        } else {
            format = "dd.MM.yyyy";
            Timber.e("Unexpected time diff: %d", daysDiff);
        }

        return new SimpleDateFormat(format, Locale.CANADA).format(msgDate);
    }

And now I should add cases for the second screen: show time only if it is today, otherwise, show only date (or "yesterday") without time.

I came up with solution below:

public static String getFormattedDate(Date msgDate, Date currentTime, boolean alwaysShowTime) {
        long daysDiff = Utils.getDaysDiff(currentTime, msgDate);
        StringBuilder format = new StringBuilder();

        // Add date representation
        if (currentTime.getYear() != msgDate.getYear()) {
            format.append("dd.MM.yyyy'");
        } else if (daysDiff == 1) {
            format.append("'вчера'");
        } else if (daysDiff > 1) {
            format.append("dd.MM");
        } else {
            format.append("dd.MM.yyyy");
            Timber.e("Unexpected time diff: %d", daysDiff);
        }

        // Add time
        if (daysDiff == 0) {
            format.append(TIME_TEMPLATE_TODAY);
        } else if (alwaysShowTime) {
            format.append("' at '")
                    .append(TIME_TEMPLATE_TODAY);
        }

        return new SimpleDateFormat(format.toString(), Locale.CANADA).format(msgDate);
    }

Disadvantages that I see are:

I changed method contract (but it used in 1-2 places, so it is not a big deal in this case)
I need to always pass this third parameter to the method.
I can solve both points creating another method with a previous contract and pass some default value for alwaysShowTime
If some third option appears, I would have to change the contract again, changing Boolean to enum, for example.

What problems or possible optimizations do you see here?
Should I just create a new method and extract some common logic to a private method instead changing existing one?

It's neatly formatted and follows standard naming-conventions. I found it easy to follow the logic.

I'm not sure why you switched from a String to a StringBuilder. It doesn't buy you anything, and makes the code more verbose. (If you did it as an optimization, the effort is misplaced. These days, Java compilers turn simple String concatenations into StringBuilder appends behind the scenes. In any case, even if you were doing this trillions of times in a tight loop, the possible savings from StringBuilder would be negligible compared to the time to format dates and whatever I/O you're doing).

Rather than using a static method, you could create an interface (e.g. DateRenderer) and one or more implementations. This would be a bit more object-oriented and more future proof. You wouldn't have to add the alwaysShowTime flag (or whatever future formatting options might be needed) to the interface, but have that knowledge in a specific implementation.

What is the foreign character string when daysDiff == 1? If it's not a typo, it might be worth writing a comment to explain, for people who can't read that language.

I'm not sure that TIME_TEMPLATE_TODAY is the right name, nor whether it is necessary. You don't use a constant for the other date/time formats in the method.


Holding a reference of the active Android Activity I recently had to work in a tricky piece of work for a project I am working on. You can find its code on GitHub and I'd like to have a feedback about some specific parts of it.

The project is a library and if you look at the README you will see how it needs to be integrated with an android application. I report it here:

public class YOUR_APP_NAMEApplication extends Application{
    private RateMyApp rateMyApp;
    @Override
    public void onCreate(){
        RateMyAppBuilder builder = new RateMyAppBuilder();
        builder.setLaunchesBeforeAlert(3);
        builder.setDaysBeforeAlert(7);
        builder.setEmailAddress("mariosangiorgio@gmail.com");
        PreferencesManager preferencesManager = SharedPreferencesManager.buildFromContext(this);
        rateMyApp = builder.build(preferencesManager);
    }

    public void activityCreated(Activity activity){
        if(rateMyApp != null)
            rateMyApp.appLaunched(activity);
    }

    public void activityDestroyed(){
        if(rateMyApp != null)
            rateMyApp.activityDestroyed();
    }
}

public class MainActivity extends Activity {
    @Override
    public void onCreate(Bundle savedInstanceState) {
        setContentView(R.layout.activity_main);
        super.onCreate(savedInstanceState);

        FutsalCoachApplication application = (FutsalCoachApplication)     getApplicationContext();
        application.activityCreated(this);
    }

    @Override
    public void onDestroy(){
        FutsalCoachApplication application = (FutsalCoachApplication) getApplicationContext();
        application.activityDestroyed();
        super.onDestroy();
    }
}

The first thing I'd like to ask is if it acceptable to require the creation of a new class extending Application just to hold the main component of my library.

I'd also like to get a feedback about the fact that my library needs to create instances of DialogFragment and to start new activities. Both these tasks require an active Activity and this is a source of a bit of trickery.

I like the fact that RateMyApp class has the dependency on the Activity instance isolated in the RateMyApp.appLaunched(Activity) method.

I don't really like how I get the class extending Application to get a reference of the current Activity. Needing to override Activity.onCreate(Bundle) and Activity.onDestroy() feels a bit dirty.

What do you think about this approach? Would you do it in an alternative (and possible cleaner) way?

First, I would recommend that you take a look at the null object pattern to avoid the != null check here:
if (rateMyApp != null)
    rateMyApp.appLaunched(activity);

By using a "null object", simply rateMyApp.appLaunched(activity); would be enough. Your null object is an instance of a special class that just doesn't do anything in the appLaunched method.

The first thing I'd like to ask is if it acceptable to require the creation of a new class extending Application just to hold the main component of my library.

No. Absolutely not. I don't really see why you are using the Application to do that. You could write a regular Java class for holding the main component of your library. Or rather, you could let the user of your library determine how to hold the main component of your library.
Instead of using the onCreate method of the Application, you could let the user initialize the RateMyApp object in the onCreate of a regular Activity.

I'd also like to get a feedback about the fact that my library needs to create instances of DialogFragment and to start new activities. Both these tasks require an active Activity and this is a source of a bit of trickery.

Technically, those tasks don't need an active Activity. They need a Context. However, "acquiring" an activity should not be that much of a problem. See below.

I don't really like how I get the class extending Application to get a reference of the current Activity.

Me neither. Why does it need a reference of the current activity?

Would you do it in an alternative (and possible cleaner) way?

Let the user whenever he likes create a RateMyApp object, and call the .appLaunched method on it (as you've done in the sample). There is no need for your RateMyApp to know which is the current showing activity, it only needs to know about an activity (or rather, Context) when it should do something - i.e. possibly show a dialog or read/write the preferences to keep track on how many times/days the application has been used. When the user of your library wants your library to possibly show the dialog, let it call a method on your library and pass along the current Activity as a parameter. That's it. No need to make things more complicated than necessary.
Summary
By extending Application as you've done in your question, you have only added a middle hand that doesn't add any value at all. All the things that are done there can, and in this case really should, be done in the MainActivity instead.


Project Euler Smallest Multiple w/ JavaScript Question 5 for Project Euler is finding the smallest multiple of 1 - 20. My solution is not even close to most of the solutions I've researched. Any feedback, good or bad, is greatly appreciated as I've been self teaching how to code on and off the past year and a half.

function smallestMultiple()
{
  var smallest = 0;
  var found = false;
  var i = 0;
  while(!found)
    {
      i++
      if(i % 11 === 0 && i % 12 === 0 && i % 13 === 0 && i % 14 === 0 && i % 15 === 0 && i % 16 === 0 && i % 17 === 0 && i % 18 === 0 && i % 19 === 0 && i % 20 === 0)
        {
          smallest = i;
          found = true;
        }
    }
  return smallest;
}

Since the problem calls for a positive number evenly divisible by numbers from 1 to 20, I would overlook the fact that the function isn't reusable and it's strictly for this use case. But in real world programming, you will want to strive for reuse. Expanding the algorithm to do more than just 1-20 would be a good exercise.

The function name is misleading. The function is single-purpose but the name... is not. smallestMultiple doesn't even inform readers that it's simply just for the numbers 1-20. A name change will do.

I see you're putting opening braces in a new line and independently. I'm not really a fan of this formatting since it takes up too much space. I would suggest the collapsed "egyptian" brackets and 2-space indent because in JS, you easily run off to the right. You will realize this when you work with callbacks and nested functions.

while(...)
  {
    // stuff
  }

while(...){
  // stuff
}

Instead of a very long list of hard-coded conditions, consider using an array of factors and array.every. array.every executes a callback for each item in the array. If all items cause the callback to return true, array.every returns true and false if otherwise. This allows you to have a dynamic number of values to check against.

function smallestMultiple(){
  var factors = [11, 12, 13, 14, 15, 16, 17, 18, 19, 20];
  var found = false;

  for(var number = 1; !found; number++)
    found = factors.every(factor => number % factor === 0);

  return number;
}

Now with the code a bit cleaner and the factors are not anymore hardcoded, you can now move factors as an argument to the function. This makes it accept any number of values. This solves the very first concern mentioned.

function smallestMultiple(factors){
  var found = false;

  for(var number = 1; !found; number++)
    found = factors.every(factor => number % factor === 0);

  return number;
}

smallestMultiple([11, 12, 13, 14, 15, 16, 17, 18, 19, 20]);

Taking it a bit further, instead of a loop, we can use recursion. Much shorter but blows away the stack when the numbers become huge.

function smallestMultiple(factors, number = 1){
  return factors.every(factor => number % factor === 0) ? number : smallestMultiple(factors, number + 1);
}

smallestMultiple([11, 12, 13, 14, 15, 16, 17, 18, 19, 20]);

Note that I am using ES6 syntax for the most part because it's convenient to write. However, browsers don't support all of the syntax yet. You can convert it to ES5 using transpilers like Buble or Babel.


Complexity for two approaches of finding next biggest palindrome of a number I have come up with a problem online about the next biggest palindrome of a number and i have solved the problem by two different approaches in python. The first one is,

t = long(raw_input())

for i in range(t):
    a = (raw_input())
    a = str(int(a) + 1)
    palin = ""
    oddOrEven = len(a) % 2

    if oddOrEven:
        size = len(a) / 2
        center = a[size]
    else:
        size = 0
        center = ''

    firsthalf = a[0 : len(a)/2]
    secondhalf = firsthalf[::-1]
    palin = firsthalf + center + secondhalf

    if (int(palin) < int(a)):
        if(size == 0):
            firsthalf = str(int(firsthalf) + 1)
            secondhalf = firsthalf[::-1]
            palin = firsthalf + secondhalf

        elif(size > 0):
            lastvalue = int(center) + 1

            if (lastvalue == 10):
                firsthalf = str(int(firsthalf) + 1)
                secondhalf = firsthalf[::-1]
                palin = firsthalf + "0" + secondhalf

            else:
                palin = firsthalf + str(lastvalue) + secondhalf
    print palin

and the another one is,

def inc(left):
    leftlist=list(left)
    last = len(left)-1
    while leftlist[last]=='9':
        leftlist[last]='0'
        last-=1

    leftlist[last] = str(int(leftlist[last])+1)
    return "".join(leftlist)

def palin(number):
    size=len(number)
    odd=size%2
    if odd:
        center=number[size/2]
    else:
        center=''
    print center
    left=number[:size/2]
    right = left[::-1]
    pdrome = left + center + right
    if pdrome > number:
        print pdrome
    else:
        if center:
            if center<'9':
                center = str(int(center)+1)
                print left + center + right
                return
            else:
                center = '0'
        if left == len(left)*'9':
            print '1' + (len(number)-1)*'0' + '1'
        else:
            left = inc(left)
            print left + center + left[::-1]

if __name__=='__main__':
    t = long (raw_input())
    while t:
        palin(raw_input())
        t-=1

As a computer science perspective, what is the complexity of both of this algorithms? which one is more efficient to use?

I did some digging on this and i think the 2nd one is more efficient. I can not determine the complexity though but i know why the first one will take more time than the 2nd one.

In the first i approach, i did some arithmatic addition on the given input directly like here a = str(int(a) + 1) or here firsthalf = str(int(firsthalf) + 1) rather than just doing arithmatic on a single number like in the 2nd approach center = str(int(center)+1). So apparently a being a huge number and doing arithmatic on it will take a considerable amount of time.

This is the easiest explanation i can come up with. If anyone can do a more elaborate explanation with the given complexity for both the algorithms please feel free to do so.


Simple basic calculator php class I wrote this basic calculator class using php.It will only execute the basic arithmetics operations so any suggestion for new functions add and improvements will be accepted. Thank you.

<?php

class Calculator{

public $x , $y , $z;

    public function __construct($x,$y,$z){
        $this->x = $x;
        $this->y = $y;
        $this->z = $z;
    }

    public function calc(){
        try{
            if($this->x != 0 && $this->z != 0){

                switch($this->y){
                case '+':
                return ($this->x + $this->z);
                break;

                case '-':
                return ($this->x - $this->z);
                break;

                case '/':
                return ($this->x / $this->z);
                break;

                case '*':
                return ($this->x * $this->z);
                break;

                }
            } else {

                throw new Exception('Operations with 0 not alloewd!');

            }
        }
        catch(Exception $e){
            return $e->getMessage();
        }
    }
}
?>

Im not going to comment on the implementation of the calculator (there are more well versed people out there) but I will comment on the coding style;

Return early (or in this case throw early)

I much prefer (I think most people do) to return or throw early to avoid more brackets and indentation.

So I would change the body of calc() to look like;

if($this->x == 0 && $this->z == 0){
    throw new \Exception('Operations with 0 not allowed!');
}

switch($this->y){
    case '+':
    return ($this->x + $this->z);
    break;

    case '-':
    return ($this->x - $this->z);
    break;

    case '/':
    return ($this->x / $this->z);
    break;

    case '*':
    return ($this->x * $this->z);
    break;

    default:
    throw new \Exception('Unknown operator');
    break;

}

Let Exceptions bubble up

If this is a calculator the excepted return is a number, not a string so doing the whole try catch and returning the $e->getMessage() is probably not what the developer expects to happen.

Variables In The Constructor

There is a lot of resources out there that suggest this is the correct way to pass data to a class.

To me variables passed to a constructor should be other classes this class depends on, the reason is so we don't have to add more manual definitions when using dependency injection (this is not true in every instance).

but here are some arguments why in this particular instance I wouldn't pass data to the constructor

For dependency injection you are going to have to define where those variables are
You will have to instantiate a new Calculator(x, y, z) every time you want to use it which seems wasteful as you return the values and not store them for another class to use


Simple Javascript Widget without cluttering global namespace? I've written some Javascript code with jQuery to display a dialog box on a web page that "floats" in the corder of the page.

(1) It has the following features: the dialog follows as the user scrolls the page.

(2) If the user holds down the [Ctrl] key, the dialog is hidden so that it doesn't obscure content.

I know that I tend to intuitively treat Javascript as if it were C code because it looks like C code and this is a habit I've been working to break.

Also, I'm working to write this code to be reusable and not litter the global namespace with various objects and functions.

I refer to the dialog as the "Little Black Book".

Bearing that in mind, please critique my code:

function getNewLittleBlackBook(blackBookId, blackBookWidth, blackBookHeight) {
    var jQueryBlackBook = $('#' + blackBookId);
    if (jQueryBlackBook.length == 0)
        throw "Could not locate the element specified by blackBookId";
    if (!blackBookWidth > 0 || !blackBookHeight > 0)
        throw "Invalid width or height specified for Little Black Book";

    var LittleBlackBook = {
        id: blackBookId,
        width: blackBookWidth,
        height: blackBookHeight,
        jQueryObj: jQueryBlackBook,

        // Functions to show and hide the little black book
        hide: function () { jQueryBlackBook.hide(); },
        show: function () { jQueryBlackBook.show(); },

        // This setPosition function determines the position based on the document scrolling.
        setPosition: function () {
            var windowHeight = $(self).height();
            var windowWidth = $(self).width();
            var scrollPosition = $(self).scrollTop();
            var newModalTop = windowHeight - this.height + scrollPosition - 40;
            var newModalLeft = windowWidth - this.width - 40;
            this.jQueryObj.css('top', newModalTop + 'px');
            this.jQueryObj.css('left', newModalLeft + 'px');
            this.jQueryObj.css('height', this.height);
            this.jQueryObj.css('width', this.width);
        }, // end setPosition

        handlers: {
            ExternalScroll: function () {
                LittleBlackBook.setPosition();
            },
            // These next two handlers serve to show/hide the Little Black Book when the user holds down the [Ctrl] key.
            ExternalKeyDown: function (e) {
                if (e.keyCode == 17)
                    LittleBlackBook.hide();
            },
            ExternalKeyUp: function (e) {
                if (e.keyCode == 17)
                    LittleBlackBook.show();
            }
        } // end handlers
    };     // end LittleBlackBook object

    // Attach event handlers.
    $(self).scroll(LittleBlackBook.handlers.ExternalScroll);
    $(document).keydown(LittleBlackBook.handlers.ExternalKeyDown);
    $(document).keyup(LittleBlackBook.handlers.ExternalKeyUp);

    LittleBlackBook.setPosition();
    LittleBlackBook.show();

    // Return the object
    return LittleBlackBook;
}

You should throw Error objects rather than strings:

throw new Error("Invalid width or height specified for Little Black Book");

This provides a stack trace.


Selection and Bubble sorts Just to enjoy, I want to implement selection and bubble sorts algorithms without looking any implementation by my own efforts . Is there any redundant/lack part(s)? After improved their implementations, I think to try to write them down as recursive, can I ?, any hint?

class Sorts {

    public static void main(String[] args) {
        int[] arrSelection = { 8, 6, 3, 1 };
        selectionSort(arrSelection);
        for (int i : arrSelection ) {
            System.out.print(i + "  ");
        }

        System.out.print("\n");

        int[] arrBubble = { 8, 6, 3, 1 };
        bubbleSort(arrBubble);
        for (int i : arrBubble ) {
            System.out.print(i + "  ");
        }

    }

    // swap helper for java
    private static int returnFirst(int x, int y) {
        return x;
    }

    public static void selectionSort(int[] arr) {
        int lengthOfInputArray = arr.length;
        int min;
        int min_index;

        for (int i = 0; i < lengthOfInputArray - 1; i++) {
            min = arr[i];
            min_index = i;
            for (int j = i+1; j < lengthOfInputArray; j++) {
                if (min > arr[j]) {
                    min = arr[j];
                    min_index = j;
                }
            }
            if (min_index != i) {
                // swap(arr[i], arr[min_index])
                arr[i] = returnFirst(arr[min_index], arr[min_index] = arr[i]);
            }
        }
    }

    public static void bubbleSort(int[] arr) {
        int lengthOfInputArray = arr.length;

        for (int i = 0; i < lengthOfInputArray; i++)
            for (int j = 0; j < lengthOfInputArray - 1; j++) {
                if (arr[j] > arr[j + 1]) {
                    // swap(arr[j], arr[j + 1])
                    arr[j] = returnFirst(arr[j + 1], arr[j + 1] = arr[j]);
                }
            }
    }

}

In addition to the other answers, there are a couple common bubble sort optimizations that you can employ.

First, your inner loop steps through the entire array each time. This is unnecessary because each pass through the array is guaranteed to sink the largest value to the bottom, so the tail of the array is always sorted. You can avoid processing the sorted bits of the array by using

for (int j = 0; j < lengthOfInputArray - (i + 1); j++) {
    ...
}

for the inner loop.

Second, you can shortcut processing by ending when the inner loop encounters no swaps.

public static void bubbleSort(int[] arr) {
    boolean swapped = false;

    for (int i = 0; i < arr.length; i++) {
        swapped = false;
        for (int j = 0; j < arr.length - (i + 1); j++) {
            if (arr[j] > arr[j + 1]) {
                if (swap(arr, j, j + 1)) {
                    swapped = true;
                }
            }
        }
        if (!swapped) {
            break;
        }
    }
}

This only happens when the array is fully sorted. So if you start out with a sorted array, then you only have to run through the array once.

Third, the outer loop does not have to be processed once for each element in the array. Take a five element array for example. It is iterated in the following manner:

value of i      max j required
----------      --------------
    0                 3
    1                 2
    2                 1
    3                 0

So the outer loop only needs to be processed arr.length - 1 times. The final optimized bubble sort would look something like this:

public static void bubbleSort(int[] arr) {
    boolean swapped = false;

    for (int i = 0; i < arr.length - 1; i++) {
        swapped = false;
        for (int j = 0; j < arr.length - (i + 1); j++) {
            if (arr[j] > arr[j + 1]) {
                if (swap(arr, j, j + 1)) {
                    swapped = true;
                }
            }
        }
        if (!swapped) {
            break;
        }
    }
}


OrdinalIgnoreCase helper Could be useful?

OrdinalIgnoreCase s1 = "a";
OrdinalIgnoreCase s2 = "A";
Assert.AreEqual(s1, s2);

Where:

public class OrdinalIgnoreCase : IEquatable<OrdinalIgnoreCase>
{
    public static StringComparer Comparer { get; } = StringComparer.OrdinalIgnoreCase;

    public static implicit operator OrdinalIgnoreCase(string s) => new OrdinalIgnoreCase(s);
    public static implicit operator string(OrdinalIgnoreCase s) => s.Text;

    public OrdinalIgnoreCase(string text) => Text = text;
    public string Text { get; }

    public override int GetHashCode() => Comparer.GetHashCode(Text);
    public override bool Equals(object obj) => Equals(obj as OrdinalIgnoreCase);
    public virtual bool Equals(OrdinalIgnoreCase other) => Comparer.Equals(Text, other.Text);

    public static bool operator ==(OrdinalIgnoreCase left, OrdinalIgnoreCase right) =>
        Equals(left, right);

    public static bool operator !=(OrdinalIgnoreCase left, OrdinalIgnoreCase right) =>
        !Equals(left, right);

    public override string ToString() => Text;
}

Review

I see no reason not to seal this class. No need for virtual methods.
You prefer readability over argument checks: Comparer.Equals(Text, other.Text)
I would make != the opposite of == as blackbox:  !(left == right);
If you want to be a bit more resilient to unicode shenanigans, you should do text.Normalize(); (á is not always á, unless you normalize)


Reimplementation of a Hashtable and hash function package com.raja.util;

import java.util.Arrays;
import java.util.HashSet;
import java.util.Random;
import java.util.Set;

public class Hashtable {
    private Object[] slots;
    private int capacity;

    Hashtable(int size) {
        capacity = size;
        slots = new String[size];
        Arrays.fill(slots, "-1");
    }

    // Simple hash function that puts values in the randomly generated index that generated within the limits of the given Hashtable size
    public void myHashFunction(Object[] elements, Object[] slots) {
        Random random = new Random();
        for (int n = 0; n < elements.length; n++) {
            int randomIndex = randomIndex(0, capacity, random);
            if(randomIndex < capacity){
                while((String)slots[randomIndex] != "-1" && randomIndex != capacity){
                    randomIndex = randomIndex(0, capacity, random);
                }
                slots[randomIndex] = elements[n];
            }
        }
    }

    //Generates a random number in the limits of begin and end
    private static int randomIndex(int begin, int end, Random aRandom){
        if (begin > end) {
          throw new IllegalArgumentException("Start cannot exceed End.");
        }
        long range = (long)end - (long)begin + 1;
        long fraction = (long)(range * aRandom.nextDouble());
        int randomNumber =  (int)(fraction + begin);
        if(randomNumber == end)
            randomNumber--;
        return randomNumber;
      }

    // Returns the value stored in the Hash slots
    public Object getValue(int key) {
        if(getKeys().contains(key)){
            return slots[key];
        } else return null;
    }

    @Override
    public String toString() {
        StringBuilder sb = new StringBuilder();
        sb.append("{ ");
        for(int i = 0; i < slots.length; i++){
            if(slots[i] != "-1")
                sb.append(i+"->" + slots[i] + ", ");
        }
        sb.append("}");
        int lastIndex = sb.lastIndexOf(",");
        sb.replace(lastIndex, lastIndex + 1, "");
        return sb.toString();
    }

    // Returns the key indexes where the elements got stored.
    public Set<Integer> getKeys(){
        Set<Integer> keys = new HashSet<Integer>();
        if(slots != null){
            for(int index = 0; index < slots.length; index++){
                if(slots[index] != "-1"){
                    keys.add(index);
                }
            }
        }
        return keys;
    }

    //Testing : Storing A-Z alphabets in Hashtable
    public static void main(String[] args) {
        Object[] elements = {"A", "B", "C", "D", "E", "F", "G", "H", "I", "J", "K", "L", "M", "N", "O", "P", "Q", "R", "S", "T", "U", "V", "W", "X", "Y", "Z"};
        Hashtable myHashtable = new Hashtable(1000);  //Big size (i.e. 1000) is used to illustrate the random hash keys
        myHashtable.myHashFunction(elements, myHashtable.slots);
        System.out.println("Keys = "+ myHashtable.getKeys());
        if(myHashtable.getKeys().size() != elements.length) {
            System.out.println("Oops!, some element(s) were not stored in your hashtable");
        }
        System.out.println("Your Hashtable = " + myHashtable);
    }
}

The output of this program would be something like:

Keys = [611, 954, 199, 406, 892, 25, 214, 633, 265, 391, 759, 102, 307, 236, 990, 36, 432, 918, 433, 40, 435, 522, 591, 775, 250, 726]
Your Hashtable = { 25->O, 36->S, 40->I, 102->H, 199->D, 214->N, 236->X, 250->Y, 265->K, 307->B, 391->F, 406->A, 432->M, 433->Q, 435->C, 522->V, 591->U, 611->G, 633->P, 726->J, 759->W, 775->R, 892->Z, 918->E, 954->T, 990->L }

Your container isn't what most people would call a hash table.  It doesn't use a deterministic hash function of its contents, and it doesn't give the expected performance.  That said, here are some other comments on your "container".

myHashtable.myHashFunction(elements, myHashtable.slots);

This is a strange name for the function that adds elements to your container.

private Object[] slots;
slots = new String[size];
public void myHashFunction(Object[] elements, Object[] slots)

Your class acts like it can work on any Object, but it will fail if given anything other than a String.

Arrays.fill(slots, "-1")

This is a strange string to use as your empty sentinel.  What happens if someone wants to add "-1" to your container?  What's wrong with null as an empty marker?


Avoiding instanceOf when finding index of an interface within a list I am working on some code and I have come across the following:

private int getPosition() {
     List<IDescriptor> fragmentDescriptors = new ArrayList<IDescriptor>(mDescriptors);

     int position = -1;

     for (IDescriptor fragmentDescriptor : fragmentDescriptors) {
         if (fragmentDescriptor instanceof VideoDescriptor) {
                position = fragmentDescriptors.indexOf(fragmentDescriptor);
         }
     }

     return position;
}

In a situation like this, how can one find an index of a specific implementation within a list without using instanceOf?

Update:
Few points: mDesciptors is a set. The point of the question was: The set of IDescriptor is injected into the class, the class has no idea what the set will contain, it does however know there maybe a IDescriptor that is meant for video.. the problem is the current method relies on VideoDescriptor, therefore if that implementation changes, the position will no longer be found.

I'd like a general review of this.  The method was really just to demonstrate the dependency on VideoDescriptor.

I totally agree with Uri and have some further comments:

  the class has no idea what the set will contain, it does however know there maybe a IDescriptor that is meant for video..

I'm sorry, but I have to ask: If you're not sure about exactly what the Set contains, why are you storing it in a set in the first place?

Consider instead something like Map<KEY_TYPE, List<? extends IDescriptor>> descriptors; where KEY_TYPE can be a Class<? extends IDescriptor>, a String, or something else that can tell you what type it is.

When adding items to this map you could use:

List<? extends IDescriptor> list = descriptors.get(descriptor.getClass());
if (list == null) {
    list = new ArrayList<>();
    descriptors.put(descriptor.getClass(), list);
}
list.add(descriptor);

And then all you would need to do is to use descriptors.get(VideoDescriptor.class) (assuming VideoDescriptor is not subclassed, otherwise you could loop through the supertypes as well when adding to the list).

Another thought is that you can use a enum to describe what kind of IDescriptor it is, and the IDescriptor type can have a getDescriptorType method. Then you could check if (fragmentDescriptor.getDescriptorType() == DescriptorType.VIDEO)

As I only see a portion of your code and not the big picture, I can't really tell what would be best for you.


Queue resizing array implementation After learning about linked list implementations of a queue I was asked to try a resizing array implementation. I'm looking for constructive criticism.

import java.util.NoSuchElementException;

/**
 * ResizingQueue - Resizing Array Implementation.
 *
 * @author Rocky
 * @version 2016-05-31
 */
public class ResizingQueue<Item> {

    private Item[] queue;
    private int size;
    private int index;

    public ResizingQueue() {
        queue = (Item[]) new Object[1];
        size = 0;
        index = 0;
    }

    /**
     * Returns the size of the queue
     *
     * @return Integer representing the number of items in the queue
     */
    public int size() {
        return this.size;
    }

    /**
     * Returns whether or not the queue is empty
     *
     * @return True if the queue is empty
     */
    public boolean isEmpty(){
        return size == 0;
    }

    /**
     * Adds an item onto the end of the queue. If the size of the queue reaches
     * the length of the array, the length of the array is doubled.
     *
     * @param item Item to be added. Must not be null
     * @throws NullPointerException if the parameter item is null
     */
    public void enqueue(Item item) {
        if (item == null) {
            throw new NullPointerException("Item must not be null");
        }
        if (size == queue.length) {
            resize(2 * queue.length);
        }
        queue[size + index] = item;
        size++;
    }

    /**
     * Removes and returns the item at the front of the queue. If the size of
     * the queue reaches 25% of the length of the array, the length of the array
     * is halved.
     *
     * @return Item at the front of the queue
     * @throws NoSuchElementException if dequeue() is called while the queue is
     * empty
     */
     public Item dequeue() {
        if (isEmpty()) {
            throw new NoSuchElementException("There is nothing in the queue");
        }
        if (size < queue.length / 4) {
            resize(queue.length / 2);
            index = 0;
        }
        Item item = queue[index];
        queue[index++] = null;
        size--;
        return item;
    }

    /**
     * Resizes the array when capacity reaches 100% or 25%
     *
     * @param capacity New capacity for the array
     */
    private void resize(int capacity) {
        Item[] copy = (Item[]) new Object[capacity];
        for (int i = 0; i < size; i++) {
            copy[i] = queue[i + index];
        }
        queue = copy;
    }

}

Your code looks good, but can be improved.

/**
 * Resizes the array when capacity reaches 100% or 25%
 *
 * @param capacity New capacity for the array
 */
private void resize(int capacity) {
    Item[] copy = (Item[]) new Object[capacity];
    for (int i = 0; i < size; i++) {
        copy[i] = queue[i + index];
    }
    queue = copy;
}

The javadoc states that this method would resize the array when the capacity reaches a specific value which isn't correct. This method resizes the underlaying buffer no matter what capacity it has.

This javadoc is lying. After removing the lying part only Resizes the array is left as description which basically doesn't add any value because it is obvious.

I would remove that javadoc completely and rename the method to resizeInternalBuffer.

enqueue()

/**
 * Adds an item onto the end of the queue. If the size of the queue reaches
 * the length of the array, the length of the array is doubled.
 *
 * @param item Item to be added. Must not be null
 * @throws NullPointerException if the parameter item is null
 */

Without reading the code I see that this method (and the dequeue() method as well) is doing too much. I would extract the checking for the size to separate methods like so

private void ensureCapacityMax() {
    if (size == queue.length) {
        resize(2 * queue.length);
    }
}

private void ensureCapacityMin() {
    if (size < queue.length / 4) {
        resize(queue.length / 2);
    }
}

which are then called like

public void enqueue(Item item) {
    if (item == null) {
        throw new NullPointerException("Item must not be null");
    }

    ensureCapacityMax();

    queue[size + index] = item;
    size++;
}

By using lambda expressions you could simplify this to one method.

I don't really like the usage of index for enqueue() and dequeue(). A much cleaner way would be to use head and tail like so

private int head = 0;
private int tail = 0;

public void enqueue(Item item) {
    if (item == null) {
        throw new NullPointerException("Item must not be null");
    }

    ensureCapacityMax()

    queue[tail] = item;
    tail = (tail + 1) % queue.length;
    size++;
}

public Item dequeue() {
    if (isEmpty()) {
        throw new NoSuchElementException("There is nothing in the queue");
    }

    ensureCapacityMin()

    Item item = queue[head];
    queue[head] = null;
    head = (head + 1) % queue.length;
    size--;
    return item;
}

now we need to adjust the resize() method like so by the usage of System.arraycopy()

private void resize(int capacity) {
    Item[] copy = (Item[]) new Object[capacity];

    if (size > 0) {
        if (head < tail) {
            System.arraycopy(queue, head, copy, 0, size);
        } else {
            System.arraycopy(queue, head, copy, 0, queue.length - head);
            System.arraycopy(queue, 0, copy, queue.length - head, tail);
        }
    }

    queue = copy;
    head = 0;
    tail = (size == capacity) ? 0 : size;
}

I didn't test this implementation but it should work well.


Compare words in one file I have started learning programming in ObjC from
"Objective-C Programming: The Big Nerd Ranch Guide"

In a task to exercise the learned stuff I've had to write a program what compares the the names in the file /usr/share/dict/words if there are same proper names (capitalized) and normal words (uncapitalized).

This is my program:

#import <Foundation/Foundation.h>

int main(int argc, const char * argv[])
{

@autoreleasepool {

    // Zähler für die Anzahl der Suchergebnisse
    int counter = 0;

    // Hier die Suchbegriffe eingeben
    NSString *search1 = @"Da";
    NSString *search2 = @"d";

    // Prüfung ob search1 und search2 identisch sind, wenn nicht eine Warnmeldung ausgeben.
    if ([search1 caseInsensitiveCompare:search2] != NSOrderedSame) {

        NSLog(@"Wenn du unterschiedliche Wörter suchst, wird die Suche lange dauern und zu keinem Ergebnis führen.");
        NSLog(@"Suche ist gestartet...");
        sleep(2);

    }   else {NSLog(@"Suche wird gestartet...");
        sleep(2);

    // Liest Datei als großen String ein (mögliche Fehler werden ausgeblendet
    NSString *namestring = [NSString stringWithContentsOfFile:@"/usr/share/dict/words"
                                                    encoding:NSUncachedRead
                                                        error:NULL];

    // Zerlegt Datei in ein Array mit Strings
    NSArray *names = [namestring componentsSeparatedByString:@"\n"];

    // Suchalgorithmus
    // Geht das Array in der 1. Dimension einen String nach dem anderen durch
    for (NSString *n in names) {

        // Sucht nach dem String search1
        NSRange r = [n rangeOfString:search1 options:NSAnchoredSearch];

        // Wurde für search1 etwas gefunden?
        if (r.location != NSNotFound) {

            // Geht das Array in der 2. Dimension einen String nach dem anderen durch
            for (NSString *m in names) {

                // Sucht nach dem String search2
                NSRange q = [m rangeOfString:search2 options:NSAnchoredSearch];

                // Wurde für search2 etwas gefunden?
                if (q.location != NSNotFound) {
                    if ([n caseInsensitiveCompare:m] == NSOrderedSame) {
                        NSLog(@"%@ is equal to %@", n, m);

                        counter++;
                    }
                }
            }
        }
    }
    NSLog(@"Es wurden %i Suchergebnisse gefunden", counter);
    }
    NSLog(@"Es wurden %i Suchergebnisse gefunden", counter);
}
return 0;
}

As you can see, I'm German.

Is there a way to do the same but more elegantly?

It's not necessary to tell me the complete code. It's better to give me some tips on which way would be better.

The first most glaring problem I see is sleep(2).  We really don't want to be doing this, particularly with Objective-C programs.  The main reason I add emphasis on Objective-C is because virtually every program written in Objective-C is either an OSX or iOS app with a GUI.  And in these cases, the main thread is the thread that handles the GUI.  You'll also be writing the bulk of your code on the main thread--certainly until you start figuring out multithreading.  Everything defaults on the main thread.

And a call to sleep(someNum) on the main thread means that the user only has one way of interacting with our program for someNum amount of time--by closing it out.

There are other ways to delay actions in our code without putting the entire thread to sleep.  In the case of this simple code, there's no interaction anyways, but in the future, you will run into this problem.

For the most simple example of how to point you in the right direction, Objective-C offers performSelector:withObject:afterDelay: (though this requires the use of objects & methods).

But there's also a non-OOP way of executing code after delays in Objective-C (it's a lot messier than performSelector:withObject:afterDelay:, and I had to look up the exact syntax...).  It makes use of code blocks.

A code block is similar to a C-style function, but we have a variable that is a pointer to it (yes, I know C and other languages have "function pointers"--we call them blocks in Objective-C).  Anyway, the syntax for executing code in a block after a delay looks like this:

int delay = dispatch_time(DISPATCH_TIME_NOW, 2 * NSEC_PER_SEC);

dispatch_after(delay, dispatch_get_main_queue(), ^{
    // any code you want executed after delay
    // in this case, delay is 2 seconds
});

Where 2 is the number of seconds you want to delay, and everything else is built-in C or Objective-C constants/functions.


Electrocardiography simulator I am tring to learn coding in clear way. I developed easy application to simulate EKG. I would like to ask some experts, if the code is clean and if they have any suggestion, what could I improve. Possible improvment is not to avoid first loop (no counter of data is set to -1, to avoid first read), but I do not have an idea how to make it.

EkgDataModel

struct ekgDataModel {
    int amountOfData;
    int frequency;
    float newValue;
    float previousValue;
};

void initEKGData(struct ekgDataModel *ekgData) {
    // -1 to avoid first loop
    ekgData->amountOfData = -1;
    ekgData->frequency = 0;
    ekgData->newValue = 0;
    ekgData->previousValue = 0;
}

EkgFunctionality.c

void runElectrocardiography() {
    struct ekgDataModel ekgData;
    initEKGData(&ekgData);

    while (ekgData.newValue != END_VALUE) {
        analyzeNewValue(&ekgData);
        ekgData.previousValue = ekgData.newValue;
        ekgData.newValue = getNewInput();
    }
}

void analyzeNewValue(struct ekgDataModel* ekgData) {
    if (ekgData->newValue >= MIN_INPUT_RANGE && ekgData->newValue <= MAX_INPUT_RANGE) {
        ++(ekgData->amountOfData);
        ekgData->frequency += checkCrossing(ekgData);
        checkIfFrequencyValid(ekgData);
    }
}

void checkIfFrequencyValid(struct ekgDataModel* ekgData) {
    if (ekgData->amountOfData == MAX_AMOUNT_OF_DATA
        && (ekgData->frequency < MIN_FREQUENCY_ALLOWED
                || ekgData->frequency > MAX_FREQUENCY_ALLOWED)) {
        displayWrongFrequencyMessage(&ekgData->frequency);
        ekgData->frequency = 0;
        ekgData->amountOfData = 0;
    }
}

int checkCrossing(struct ekgDataModel* ekgData) {
    if (ekgData->newValue * ekgData->previousValue < 0)
        return 1;
    else
        return 0;
}

EkgInputOutputOperations.c

float getNewInput() {
    float newValue;
    scanf("%f", &newValue);
    return newValue;
}

void displayWrongFrequencyMessage(int *frequency) {
    printf("Bad frequency result: %d\n", *frequency);
}

I see a number of things that might help you improve your code.

Arrange functions from simple to compound

Within your source code files, try to arrange your functions such that simpler functions, such as checkCrossing precede compound functions such as analyzeNewValue that call other simple functions.  Doing so allows you to avoid having to write function declarations that would otherwise be required.

Use const where practical

The checkCrossing function is passed a pointer to an ekgDataModel and the function does not alter that structure.  This should be made explicit by declaring the parameter const as in:

int checkCrossing(const struct ekgDataModel* ekgData) {

Carefully consider your data

The checkCrossing routine is looking for zero-crossings and uses this expression:

if (ekgData->newValue * ekgData->previousValue < 0) { /* ... */ }

However, consider the possibility that one of the samples is exactly 0.  For example, a sequence might be -0.15, 0.0, +0.20.  For (-0.15, 0.0) the expression is false and for (0.0, +0.20) the expression is again false.  The effect is that the function has just missed an obvious zero-crossing. An alternative approach would be to use the signbit macro:

if (signbit(ekgData->newValue) != signbit(ekgData->previousValue)) { /* ... */ }

I'd rewrite the function using stdbool.h as a bool function:

bool isZeroCrossing(float prev, float curr) {
    return (bool)signbit(curr) != (bool)signbit(prev);
}

Prefer >= to == to prevent "runaway data"

In the checkIfFrequencyValid function, the program currently does this:

if (ekgData->amountOfData == MAX_AMOUNT_OF_DATA
    && (ekgData->frequency < MIN_FREQUENCY_ALLOWED
            || ekgData->frequency > MAX_FREQUENCY_ALLOWED)) {
     /* stuff */
     ekgData->amountOfData = 0;
}

There is a potential problem here in that the analyzeNewValue function only increments amountOfData, so if it ever gets to MAX_AMOUNT_OF_DATA and the frequency is within range, subsequent readings will increase amountOfData beyond MAX_AMOUNT_OF_DATA which would make for a difficult-to-debug error.  Instead, use the >= operator for the first clause to make sure that you don't have "runaway data" problems.

Use better names

Function names such as displayWrongFrequencyMessage and initEKGData are good because they give a very good idea of what the functions are doing just by the name.  However functions like checkCrossing and checkIfFrequencyValid are not so good.  In one case, essentially a boolean value is returned indicating that a zero crossing has occurred, but in the other, nothing is returned at all.  Better for boolean functions is to name them to suggest what the return value might mean.  For example isZeroCrossing would return true if it detected a zero crossing.  Very clear and doesn't require much further explanation.

Check return values of standard functions

Many standard functions, including scanf can fail and return values that can help in detecting those failures.  To write robust software, you should get into the habit of checking the return values and dealing intelligently with the potential failures.

Avoid scanf if you can

There are so many well known problems with scanf that you're usually better off avoiding it.  The usual approach is to read in user input into a string and then parse from there, in this case using something like atof().

Think carefully about signed versus unsigned

Would it ever be possible that amountOfData or frequency would be negative numbers?  If so, what would that mean?  Better would be to declare them both as unsigned so that all possible values have an interpretable meaning.

Prefer pass-by-value for simple data

Generally, unless there is a need to modify the underlying variable, it's better to pass a value rather than a pointer.  For instance, this function:

void displayWrongFrequencyMessage(int *frequency) {

Could instead be written like this:

void displayWrongFrequencyMessage(int frequency) {

That way there's no way that the function could alter the original frequency variable and it's likely to be efficiently passed in a register.

Write code for humans to read it

The runElectrocardiography function is not bad, but it's not easy to understand.  I'd recommend doing something like this instead:

void runElectrocardiography() {
    unsigned dataCount = 0;  // number of data points so far
    unsigned crossingCount = 0; // number of zero-crossings
    float curr; // the most recent EKG voltage value
    float prev = 0; // the previous EKG voltage value

    for (curr = getNewValue(); curr != END_VALUE; curr = getNewValue()) {
        if (isWithinValidRange(curr)) {
            ++dataCount;
            if (isZeroCrossing(prev, curr)) {
                ++crossingCount;
            }
            if (!isFrequencyValid(dataCount, crossingCount)) {
                displayWrongFrequencyMessage(crossingCount);
                dataCount = 0;
                crossingCount = 0;
            }
            prev = curr;
        }
    }
}

Note that the ekgDataModel structure is no longer needed or used.


Correct way to implement a Singleton field I have a helper class that does HTTP operations on Android. Every method uses a client field. I think it's a good idea to make this client a singleton since every method in the class uses it.

The problem is that I don't know 100% what's the best way to implement this singleton since it's only a field in my helper class.

public class HttpHelper {
    private static OkHttpClient client;

    private HttpHelper() { }

    private static OkHttpClient getClient() {
        if(client == null) {
            client = new OkHttpClient();
            client.setConnectTimeout(TIMEOUT, TimeUnit.SECONDS);
        }
       return client;
    }

    // method that uses client:
    public static String getHTML(String cookie, String URL) throws IOException {

        Request request = new Request.Builder().url(URL).header("Cookie", cookie).build();
        Response response = getClient().newCall(request).execute(); // using client
        if (!response.isSuccessful()) throw new IOException("Unexpected code " + response);

        return readStream(response.body().byteStream());
    }
}

I am using methods like getHTML() (yes, the class has more) all over my project.

Is it OK that I use a lot of static methods like getHTML()?
Should I be implementing the HttpHelper class as singleton and use non static methods like HttpHelper.getInstance().getHTML() (where the client is created in the getInstance() method of the HttpHelper?

IMHO, it's ok to use static methods if the object (HTTPHelper in your case) doesn't store data and can't have multiple states.

A little suggestion:

    private static final OkHttpClient client;
    static {
        client = new OkHttpClient();
        client.setConnectTimeout(TIMEOUT, TimeUnit.SECONDS);
    }

if you initailize your client in this way, you won't need getClient method at all!

Actually, what concerns me in your code is whether it must be used by multiple threads or not. What will happen of one thread call getHTML while client is used in another thread pending from answer?

Addition:
what does

private HttpHelper() { }

serve for? Do you want to forbid to create an instance of your class? If so, your class may be extended and instantiated, or your constructor may be accessed via reflection. Try this:

private HttpHelper() {
    throw new NonInstantiableException()
}


Variations of the Repository Pattern I've been using this variation of the Repository pattern for over a year now:

 public interface IReadOnlyRepository<T, in TId>
     where T : AbstractEntity<TId>
    {
        T Get( TId id );
        IEnumerable<T> GetAll();
    }

/// <summary>
/// Defines a generic repository interface for
/// classes solely in charge of getting and processing data from a data source
/// </summary>
/// <typeparam name="T"></typeparam>
/// <typeparam name="TId">The type of the id.</typeparam>
public interface IRepository<T, in TId> : IReadOnlyRepository<T, TId> where T : AbstractEntity<TId>
{
    /// <summary>
    /// Determines whether the specified entity has duplicates.
    /// </summary>
    /// <param name="entity">The entity.</param>
    /// <returns>
    ///   <c>true</c> if the specified entity has duplicates; otherwise, <c>false</c>.
    /// </returns>
    bool HasDuplicates(T entity);

    /// <summary>
    /// Inserts the specified entity.
    /// </summary>
    /// <param name="entity">The entity.</param>
    void Save( T entity );
    /// <summary>
    /// Inserts the entity or updates it if it already exists.
    /// </summary>
    /// <param name="entity">The entity.</param>
    T SaveOrUpdate( T entity );

    /// <summary>
    /// Updates the specified entity.
    /// </summary>
    /// <param name="entity">The entity.</param>
    /// <returns></returns>
    T Update(T entity);

    /// <summary>
    /// Deletes the specified entity from the data source.
    /// </summary>
    /// <param name="entity">The entity.</param>
    void Delete(T entity);

    /// <summary>
    /// Deletes the entity with the specified id.
    /// </summary>
    /// <param name="id">The id.</param>
    void Delete(TId id);
}

but recently, after rereading some books on Design Patterns, I've had this seemingly amazing idea to apply some patterns to my repositories.

public interface IRepository<T, in TId> : IReadOnlyRepository<T, TId> where T : AbstractEntity<TId>
{
    void Execute(IRepositoryCommand command);
    void Execute(IBatchRepositoryCommand command);
}

public interface IRepositoryCommand<T>
{
    void Execute(T entity);
}

public interface IBatchRepositoryCommand<T>
{
    void Execute(IEnumerable<T> entities);
}

public SaveCommand<T> : IRepositoryCommand<T>
{
    public void Execute(T entity)
    {
        // Logic for saving goes here
    }
}

public BatchSaveCommand<T> : IRepositoryCommand<T>
{
    public void Execute(IEnumerable<T> entities)
    {
        // Logic for batch saves go here
    }
}

which would then be called like this:

_myRepository.Execute(new SaveCommand());

My reasoning is that placing logic for the common data access operations (e.g. saving, deleting) gets to be so repetitive that right now I'm relying on a T4 template to recreate those everytime I have a new entity enter the playing field. This way I just define the most commonly used Data Access operations and then have any of my callers execute whatever action they need to execute.

Can you critique my work? I do have the tendency to overthink and overengineer things.

Although the command pattern you brought in makes it easy to create a 'flexible' Execute method. I wonder if it is really transparant to the ones who read your code.

Perhaps you can combine your two ideas, by implementing a certain ReadOnlyRepository using the command objects. So that the user of your interace keeps using:

repository->Update(SomeEntity);

While the repository implementation does:

repository.Execute(new UpdateCommand());

Consider, if you had to make a change to your later interface. Where would you need to update that in your code? In my suggestion it simply requires you to change it at one place. (since all the other code still uses the 'old' interface).

I hope my post makes sense :)